Skip to content

ref-info: more correctness #8815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 30, 2025
Merged

ref-info: more correctness #8815

merged 2 commits into from
May 30, 2025

Conversation

Byron
Copy link
Collaborator

@Byron Byron commented May 29, 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 #8803.

Tasks

  • try normal workflow once, including the integration of a branch and its archival
    • fix base-branch is considered empty if dependent branch is created on top
    • why is it now considered unpushed?
    • fix the "two dependent branches pushed and base doesn't detect that state" issue
  • deal with archived flag
  • more tests for unapplied stacks and branch listings.

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.

Copy link

vercel bot commented May 29, 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 May 30, 2025 6:41pm

@vercel vercel bot temporarily deployed to Preview – gitbutler-components May 29, 2025 03:08 Inactive
Copy link

vercel bot commented May 29, 2025

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

A member of the Team first needs to authorize it.

- empty dependendant branches don't empty the segment below.
@Byron Byron marked this pull request as ready for review May 30, 2025 18:46
@Byron Byron enabled auto-merge May 30, 2025 18:46
@Byron Byron merged commit 63e0158 into gitbutlerapp:master May 30, 2025
19 of 20 checks passed
@Byron Byron mentioned this pull request May 30, 2025
13 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