Skip to content

Conversation

@Byron
Copy link
Collaborator

@Byron Byron commented Jun 4, 2025

There are still a lot of fatal flaws resulting in errors and even panics. These should be fixed and it should 'generally' be working.
Write critical tests only.

Follow-up on #8883.

Old Tasks

Before deciding to rethink the whole thing.

  • try normal workflow once, including the integration of a branch and its archival
    • figure out what should happen after merge (is that the archived flag?)
  • deal with archived flag
  • more tests for unapplied stacks and branch listings
    • apply and unapply testing)

Possible Tasks

  • a single branch can be applied multiple times and will then show up multiple times as parent to the workspace commit
  • A test for: commit in branch A has "foo" added, now try to create a commit with the removal of "foo" in branch B
  • V3 of get_branch_listing_details()

Notable changes to the Datamodel

  • Upstream commits can also be integrated. This would happen if the local tracking branch has not caught up to them, and all of them are integrated. This also means that everything local could be unintegrated, but upstream-only commits are integrated, something that can happen if everything is diverged. If both disagree, it's unclear what to do.

Notes for the Reviewer

  • This is a transitory PR that replaces VirtualBranchesToml with the RefMetadata trait to prepare the code to transition to other data stores.
  • The code is clearly transitory while StackId is still a thing - in the new world stacks or stack-segments are referred to by their reference name or by their index in the parent-list of the top-level merge commit (if there is one).
  • Ideally, one the UI will be able to make just one heads_info call, and can consume the data directly (even though it may have been processed to further facilitate consumption).
  • branch_details() already works on references
  • BranchDetails are probably the data structure of choice for the UI, and it's just the question if there is stack information or not.
  • There is no notion of using stacks in branch listings outside of what we are currently looking at, i.e. where HEAD is. Thus, for this we will probably keep using branch details of sort.

Next PR

  • intermediate V3 version of get_base_branch_data().

Follow Up Tasks

  • first non-workspace cases
    • merge commit
    • merge commit stacked
    • stacks and ambiguous stack references (i.e. lots of empty stack segments)

Next PRs

  • head-info, stacks and details API with key scenarios
  • graph-based integration checks with target and remote
  • use hide() in places where merge-commits are used as boundary.
    • This shouldn't be a major problem for simple topologies, but is certainly an issue for more complex ones.
  • integration checking with target
  • integration checking with remote tracking branch

Known Shortcomings (for now)

  • ⚠️ first_parent_only maybe a good simplification for display, but I wonder if there are side-effects like us not seeing commits that could participate in commit-status check.
  • ⚠️ Commit-classification is hacky and undertested right now⚠️
  • One probably wants to show all refs at a position (or indicate there are more)
  • ⚠️ rebase engine needs a way to know which parent to rewrite if there are two parents pointing to the same commit in a starting configuration with two stacks at the same commit. Alternatively, create_commit would have to create a workspace commit, which seems worse than adding a WS commit in the moment there are two stacks.
  • ⚠️ merge conflicts are created from either cherry-picks or normal merges (legacy?), but cherry-pick would need to know that to either choose the auto-resolution. That's OK as long as the 'old' merge-commit isn't run as it would create a semantically different tree-setup in the conflicted commit.
  • ⚠️ commit listing are ambiguous
    • Certain less common but possible branch configuration make ambiguous to assign commits to one branch or another. It won't be super trivial to make this work right.
  • ⚠️Even though Git does not list namespaced references, tig will happily list everything. set reference-format = hide:other fixes this though
  • empty commits aren't specifically highlighted when rebasing, or handled or prevented. The UI could detect that and show them.
  • Moving hunks or files will need multi-branch rebases with merge-commit handling. This can be added to the rebase engine as we know it today. This will also enable worktree-updates.
  • reordering in such a way that only heads a moved and nothing else happens (in terms of commit rewrites) probably wouldn't work yet in but-rebase.
  • Index adjustments (tree to disk index) lack a lot of tests, particularly those for automatic conflict removal.
  • if changes are added to the wrong spot, then they may apply cleanly and yield different results after merging, so the worktree differs from what's in Git. This is where hunk-dependencies/auto-hunk-splitting comes into play.
  • Right now sub-hunks can't be selected as they won't match when it tries to find exact matches. However, that check could be changed to a 'contains' or 'is-included' to allow sub-hunks. Maybe this doesn't naturally work though or do the right thing.
  • Workspace commits with a single branch will get signed if they were signed before. This shouldn't be a real problem, and ideally it will go away soon enough.

For follow-up PRs

In any order (probably)

  • Rebase-engine with insert empty commit (below and above, insert as first parent support)
  • What happens in Git if one rebases non-linear branches? Can it retain the structure?
    • Rebase engine probably has to learn to re-schedule picks (and remember the base, a feature useful for multi-stacks as well, which then wouldn't need a special case anymore)
  • move file out of commit into worktree (uncommit something)
  • per-hunk exclusion if hunk didn't match (right now it rejects the whole file)
  • run hooks on an index created from the tree that would be committed.
  • move hunks from one commit into another

Out of scope

  • hunk-dependencies
    • These should be added once the commit-engine is feature-complete, the idea is that the UI can function well enough without them as a baseline.
  • atomic object writes
    • In theory, new objects should only be written to disk if they actually end up in a tree. For instance, if a change is rejected, the object associated with it shouldn't be in the object database.
    • However, even though implementing this with the in-memory object feature is very possible, it feels like an optimization for another day. In general, I really think only objects that end up in a tree should actually be written, so that the implementation doesn't have to rely on git gc to cleanup.

@vercel
Copy link

vercel bot commented Jun 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gitbutler-components ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 5, 2025 11:56am

@vercel vercel bot temporarily deployed to Preview – gitbutler-components June 4, 2025 15:38 Inactive
@vercel
Copy link

vercel bot commented Jun 4, 2025

@Byron is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

@Byron
Copy link
Collaborator Author

Byron commented Jun 5, 2025

Just added another test that represents an interesting real-world problem, but without a fix as this would be superseded by the segmented graph anyway.

This PR should be re-created once the new graph-based implementation is ready, and needs to be further reality-proofed.

@Byron Byron merged commit 6766090 into gitbutlerapp:master Jun 5, 2025
19 of 20 checks passed
This was referenced Jun 5, 2025
This was referenced Jun 18, 2025
@Byron Byron mentioned this pull request Jun 30, 2025
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant