Merge pull request #8315 from Byron/ref-updates

Tame ref-updates

Author: Byron

crates/but-workspace/src/commit_engine/mod.rs

@@ -348,22 +348,57 @@ pub fn create_commit_and_update_refs(
         return Ok(out);
     };
 
-    let commit_to_find = match destination {
+    let (commit_to_find, is_amend) = match destination {
         Destination::NewCommit {
             parent_commit_id, ..
-        } => parent_commit_id,
-        Destination::AmendCommit(commit) => Some(commit),
+        } => (parent_commit_id, false),
+        Destination::AmendCommit(commit) => (Some(commit), true),
     };
 
     if let Some(commit_in_graph) = commit_to_find {
         let mut all_refs_by_id = gix::hashtable::HashMap::<_, Vec<_>>::default();
-        for (commit_id, git_reference) in repo
-            .references()?
-            .prefixed("refs/heads/")?
-            .chain(repo.references()?.prefixed("refs/gitbutler/")?)
-            .filter_map(Result::ok)
-            .filter_map(|r| r.try_id().map(|id| (id.detach(), r.inner.name)))
-        {
+        let mut checked_out_ref_name = None;
+        let checked_out_ref = repo.head_ref()?.and_then(|mut r| {
+            let id = r.peel_to_id_in_place().ok()?.detach();
+            checked_out_ref_name = Some(r.inner.name.clone());
+            Some((id, r.inner.name))
+        });
+        let (platform_storage, platform_storage_2);
+        let checked_out_and_gitbutler_refs =
+            checked_out_ref
+                .into_iter()
+                // TODO: remove this as `refs/gitbutler/` won't contain relevant refs anymore.
+                .chain({
+                    platform_storage_2 = repo.references()?;
+                    platform_storage_2
+                        .prefixed("refs/gitbutler/")?
+                        .filter_map(Result::ok)
+                        .filter_map(|r| r.try_id().map(|id| (id.detach(), r.inner.name)))
+                })
+                .chain(
+                    // When amending, we want to update all branches that pointed to the old commit to now point to the new commit.
+                    if is_amend {
+                        Box::new({
+                            platform_storage = repo.references()?;
+                            platform_storage
+                                .prefixed("refs/heads/")?
+                                .filter_map(Result::ok)
+                                .filter_map(|r| {
+                                    let is_checked_out = checked_out_ref_name.as_ref().is_some_and(
+                                        |checked_out_ref| checked_out_ref == &r.inner.name,
+                                    );
+                                    if is_checked_out {
+                                        None
+                                    } else {
+                                        r.try_id().map(|id| (id.detach(), r.inner.name))
+                                    }
+                                })
+                        }) as Box<dyn Iterator<Item = _>>
+                    } else {
+                        Box::new(std::iter::empty())
+                    },
+                );
+        for (commit_id, git_reference) in checked_out_and_gitbutler_refs {
             all_refs_by_id
                 .entry(commit_id)
                 .or_default()
@@ -435,7 +470,7 @@ pub fn create_commit_and_update_refs(
                         && !wsc.inner.parents.contains(&branch_tip) /* the branch tip we know isn't yet merged */
                         // but the tip is known to the workspace
                         && vb.branches.values().any(|s| {
-                        s.head(repo)
+                        s.head_oid(repo)
                             .is_ok_and(|head_id| head_id == branch_tip)
                     }) {
                         let mut stacks: Vec<_> = vb

crates/but-workspace/src/commit_engine/reference_frame.rs

@@ -47,12 +47,12 @@ impl ReferenceFrame {
                     .context("Didn't find stack - was it deleted just now?")?;
                 Ok(ReferenceFrame {
                     workspace_tip: Some(head_id.detach()),
-                    branch_tip: Some(stack.head(repo)?),
+                    branch_tip: Some(stack.head_oid(repo)?),
                 })
             }
             InferenceMode::CommitIdInStack(commit_id) => {
                 for stack in vb.branches.values() {
-                    let stack_tip = stack.head(repo)?;
+                    let stack_tip = stack.head_oid(repo)?;
                     if stack_tip
                         .attach(repo)
                         .ancestors()

crates/but-workspace/src/commit_engine/refs.rs

@@ -35,16 +35,8 @@ pub fn rewrite(
                     continue; // Dont rewrite refs for other stacks
                 }
             }
-            if stack.head(repo)? == old {
-                // Perhaps skip this - the head will be updated later in this call
-                // stack.set_stack_head_without_persisting(repo, new.to_git2(), None)?;
-                // Does it make sense to set stack tree in v3? I think not
-                // stack.tree = new
-                //     .attach(repo)
-                //     .object()?
-                //     .into_commit()
-                //     .tree_id()?
-                //     .to_git2();
+            if stack.head_oid(repo)? == old {
+                // The actual head will be updated later.
                 updated_refs.push(UpdatedReference {
                     old_commit_id: old,
                     new_commit_id: new,
@@ -65,15 +57,16 @@ pub fn rewrite(
                     });
             for (idx, branch) in stack.heads.iter_mut().rev().enumerate() {
                 let id = branch.head_oid(repo)?;
-                if id == old {
-                    if update_up_to_idx.is_some() && Some(idx) > update_up_to_idx {
-                        // Make sure the actual refs also don't update (later)
-                        already_updated_refs.push(format!("refs/heads/{}", branch.name()).into());
-                        continue;
-                    }
-                    if let Some(full_refname) = branch.set_head(new, repo)? {
-                        already_updated_refs.push(full_refname)
-                    }
+                if id != old {
+                    continue;
+                }
+                if update_up_to_idx.is_some() && Some(idx) > update_up_to_idx {
+                    // Make sure the actual refs also don't update (later)
+                    already_updated_refs.push(format!("refs/heads/{}", branch.name()).into());
+                    continue;
+                }
+                if let Some(full_refname) = branch.set_head(new, repo)? {
+                    already_updated_refs.push(full_refname);
                     updated_refs.push(UpdatedReference {
                         old_commit_id: old,
                         new_commit_id: new,

crates/but-workspace/src/lib.rs

@@ -140,7 +140,7 @@ impl StackEntry {
         Ok(StackEntry {
             id: stack.id,
             heads: stack_heads_info(stack, repo)?,
-            tip: stack.head(repo)?,
+            tip: stack.head_oid(repo)?,
         })
     }
 }

crates/but-workspace/tests/fixtures/scenario/three-commits-with-line-offset-and-workspace-commit.sh

@@ -10,6 +10,6 @@ git init
 seq 10 >file
 git add . && git commit -m init && git tag first-commit
 
-{ seq 4; seq 10; } >file && git commit -am "insert 5 lines to the top"
+{ seq 4; seq 10; } >file && git commit -am "insert 5 lines to the top" && git branch feat1
 
 git commit -m $'GitButler Workspace Commit\n\njust a fake - only the subject matters right now' --allow-empty

crates/but-workspace/tests/workspace/commit_engine/refs_update.rs

@@ -110,8 +110,8 @@ fn new_commits_to_tip_from_unborn_head() -> anyhow::Result<()> {
     // The HEAD reference was updated, along with all other tag-references that pointed to it.
     let new_commit = outcome.new_commit.expect("a new commit was created");
     insta::assert_snapshot!(visualize_commit_graph(&repo, new_commit)?, @r"
-    * 1c4bb33 (HEAD -> main, another-tip) third commit
-    * f1b87da (tag: tag-that-should-not-move) second commit
+    * 1c4bb33 (HEAD -> main) third commit
+    * f1b87da (tag: tag-that-should-not-move, another-tip) second commit
     * 0939187 initial commit
     ");
 
@@ -132,8 +132,8 @@ fn new_commits_to_tip_from_unborn_head() -> anyhow::Result<()> {
     assure_no_worktree_changes(&repo)?;
     // The top commit has a different hash now thanks to amending.
     insta::assert_snapshot!(graph_commit_outcome(&repo, &outcome)?, @r"
-    * 2abfa5c (HEAD -> main, another-tip) third commit
-    * f1b87da (tag: tag-that-should-not-move) second commit
+    * 2abfa5c (HEAD -> main) third commit
+    * f1b87da (tag: tag-that-should-not-move, another-tip) second commit
     * 0939187 initial commit
     ");
 
@@ -194,15 +194,6 @@ fn new_commits_to_tip_from_unborn_head() -> anyhow::Result<()> {
             Sha1(273aeca7ca98af0f7972af6e7859a3ae7fde497a),
         ),
         references: [
-            UpdatedReference {
-                reference: Git(
-                    FullName(
-                        "refs/heads/another-tip",
-                    ),
-                ),
-                old_commit_id: Sha1(2abfa5cc3c7c48b8b9eabbd10c21b88347801f15),
-                new_commit_id: Sha1(189ac82eb44ddb97677d7d7b1859cf6f2e33a473),
-            },
             UpdatedReference {
                 reference: Git(
                     FullName(
@@ -234,9 +225,9 @@ fn new_commits_to_tip_from_unborn_head() -> anyhow::Result<()> {
     write_vrbranches_to_refs(&vb, &repo)?;
     // It updates stack heads and stack branch heads.
     insta::assert_snapshot!(graph_commit_outcome(&repo, &outcome)?, @r"
-    * 189ac82 (HEAD -> main, s2-b/second, s1-b/second, another-tip) fourth commit
+    * 189ac82 (HEAD -> main, s2-b/second, s1-b/second) fourth commit
     * 2abfa5c third commit
-    * f1b87da (tag: tag-that-should-not-move, s2-b/first, s1-b/first) second commit
+    * f1b87da (tag: tag-that-should-not-move, s2-b/first, s1-b/first, another-tip) second commit
     * 0939187 (s2-b/init, s1-b/init) initial commit
     ");
     insta::assert_snapshot!(visualize_tree(&repo, &outcome)?, @r#"
@@ -258,7 +249,6 @@ fn new_commits_to_tip_from_unborn_head() -> anyhow::Result<()> {
 /// https://github.com/gitbutlerapp/gitbutler/pull/7596 may affect the solution here, but
 /// it's not yet ready.
 #[test]
-#[ignore = "needs fixes"]
 fn new_stack_receives_commit_and_adds_it_to_workspace_commit() -> anyhow::Result<()> {
     assure_stable_env();
 
@@ -270,7 +260,7 @@ fn new_stack_receives_commit_and_adds_it_to_workspace_commit() -> anyhow::Result
     let workspace_commit_id = repo.rev_parse_single("@")?.detach();
     insta::assert_snapshot!(visualize_commit_graph(&repo, workspace_commit_id)?, @r"
     * 47c9e16 (HEAD -> main) GitButler Workspace Commit
-    * b451685 insert 5 lines to the top
+    * b451685 (feat1) insert 5 lines to the top
     * d15b5ae (tag: first-commit) init
     ");
 
@@ -311,10 +301,10 @@ fn new_stack_receives_commit_and_adds_it_to_workspace_commit() -> anyhow::Result
     // head was updated to point to the new workspace commit.
     insta::assert_snapshot!(visualize_commit_graph(&repo, repo.head_id()?)?, @r"
     *   7051951 (HEAD -> main) GitButler Workspace Commit
-    |\
+    |\  
     | * 00fbfba (s2/top) new file with 15 lines
-    * | b451685 (s1/top) insert 5 lines to the top
-    |/
+    * | b451685 (s1/top, feat1) insert 5 lines to the top
+    |/  
     * d15b5ae (tag: first-commit) init
     ");
 
@@ -612,8 +602,8 @@ fn insert_commit_into_single_stack_with_signatures() -> anyhow::Result<()> {
     let rewritten_head_id = repo.head_id()?.detach();
     insta::assert_snapshot!(visualize_commit_graph(&repo, rewritten_head_id)?, @r"
     * 3d1262e (HEAD -> main) insert 10 lines to the top
-    * 3aec753 (s1-b/init, first-commit) between initial and former first
-    * ecd6722 (tag: first-commit) init
+    * 3aec753 (s1-b/init) between initial and former first
+    * ecd6722 (tag: first-commit, first-commit) init
     ");
     insta::assert_snapshot!(but_testsupport::visualize_tree(rewritten_head_id.attach(&repo)), @r#"
     5fdd313
@@ -640,15 +630,6 @@ fn insert_commit_into_single_stack_with_signatures() -> anyhow::Result<()> {
                 old_commit_id: Sha1(ecd67221705b069c4f46365a46c8f2cd8a97ec19),
                 new_commit_id: Sha1(3aec75308383b83d85a78a90308a618755a7b0f8),
             },
-            UpdatedReference {
-                reference: Git(
-                    FullName(
-                        "refs/heads/first-commit",
-                    ),
-                ),
-                old_commit_id: Sha1(ecd67221705b069c4f46365a46c8f2cd8a97ec19),
-                new_commit_id: Sha1(3aec75308383b83d85a78a90308a618755a7b0f8),
-            },
             UpdatedReference {
                 reference: Git(
                     FullName(
@@ -711,8 +692,8 @@ fn insert_commit_into_single_stack_with_signatures() -> anyhow::Result<()> {
     let rewritten_head_id = repo.head_id()?;
     insta::assert_snapshot!(visualize_commit_graph(&repo, rewritten_head_id)?, @r"
     * 4b649eb (HEAD -> main) insert 10 lines to the top
-    * d26d789 (s1-b/init, first-commit) between initial and former first
-    * ecd6722 (tag: first-commit) init
+    * d26d789 (s1-b/init) between initial and former first
+    * ecd6722 (tag: first-commit, first-commit) init
     ");
     insta::assert_snapshot!(but_testsupport::visualize_tree(rewritten_head_id), @r#"
     683b451
@@ -880,8 +861,8 @@ fn insert_commits_into_workspace() -> anyhow::Result<()> {
     *   a97960f (HEAD -> merge) Merge branch 'A' into merge
     |\  
     | * 3538622 (A) add 10 to the beginning
-    * | 46991ae (s1-b/init, B) add 10 more lines at end
-    * | e81b470 add 10 to the end
+    * | 46991ae (s1-b/init) add 10 more lines at end
+    * | e81b470 (B) add 10 to the end
     |/  
     * 9cf2979 (main) init
     ");
@@ -1007,8 +988,8 @@ fn merge_commit_remains_unsigned_in_remerge() -> anyhow::Result<()> {
     insta::assert_snapshot!(visualize_commit_graph(&repo, rewritten_head_id)?, @r"
     *   595a255 (HEAD -> merge) Merge branch 'A' into merge
     |\  
-    | * 057f154 (s1-b/top, A) remove 5 lines from beginning
-    | * eede47d add 10 to the beginning
+    | * 057f154 (s1-b/top) remove 5 lines from beginning
+    | * eede47d (A) add 10 to the beginning
     * | 16fe86e (B) add 10 to the end
     |/  
     * 6074509 (main) init
@@ -1228,8 +1209,8 @@ fn commit_on_top_of_branch_in_workspace() -> anyhow::Result<()> {
     insta::assert_snapshot!(visualize_commit_graph(&repo, rewritten_head_id)?, @r"
     *   09ac476 (HEAD -> merge) Merge branch 'A' into merge
     |\  
-    | * 99f3a1c (s1-b/top, A) remove 5 lines from beginning
-    | * 7f389ed (s1-b/below-top) add 10 to the beginning
+    | * 99f3a1c (s1-b/top) remove 5 lines from beginning
+    | * 7f389ed (s1-b/below-top, A) add 10 to the beginning
     * | 91ef6f6 (s2-b/top, s2-b/below-top, B) add 10 to the end
     |/  
     * ff045ef (main) init
@@ -1316,10 +1297,10 @@ fn commit_on_top_of_branch_in_workspace() -> anyhow::Result<()> {
     insta::assert_snapshot!(visualize_commit_graph(&repo, rewritten_head_id)?, @r"
     *   098effd (HEAD -> merge) Merge branch 'A' into merge
     |\  
-    | * 99f3a1c (s1-b/top, A) remove 5 lines from beginning
-    | * 7f389ed (s1-b/below-top) add 10 to the beginning
-    * | 03e9b6f (s2-b/top, B) remove 5 lines from the end
-    * | 91ef6f6 (s2-b/below-top) add 10 to the end
+    | * 99f3a1c (s1-b/top) remove 5 lines from beginning
+    | * 7f389ed (s1-b/below-top, A) add 10 to the beginning
+    * | 03e9b6f (s2-b/top) remove 5 lines from the end
+    * | 91ef6f6 (s2-b/below-top, B) add 10 to the end
     |/  
     * ff045ef (main) init
     ");

crates/gitbutler-branch-actions/src/actions.rs

@@ -111,7 +111,7 @@ pub fn create_virtual_branch(
     Ok(StackEntry {
         id: stack.id,
         heads: stack_heads_info(&stack, &repo)?,
-        tip: stack.head(&repo)?,
+        tip: stack.head_oid(&repo)?,
     })
 }
 

crates/gitbutler-branch-actions/src/base.rs

@@ -82,7 +82,7 @@ fn go_back_to_integration(ctx: &CommandContext, default_target: &Target) -> Resu
     for branch in &virtual_branches {
         // merge this branches tree with our tree
         let branch_tree_id = git2_to_gix_object_id(
-            repo.find_commit(branch.head(&gix_repo)?.to_git2())
+            repo.find_commit(branch.head_oid(&gix_repo)?.to_git2())
                 .context("failed to find branch head")?
                 .tree_id(),
         );

crates/gitbutler-branch-actions/src/branch.rs

@@ -298,7 +298,7 @@ fn branch_group_to_branch(
     // If there is a virtual branch let's get it's head. Alternatively, pick the first local branch and use it's head.
     // If there are no local branches, pick the first remote branch.
     let head_commit = if let Some(vbranch) = virtual_branch {
-        Some(vbranch.head(repo)?.attach(repo))
+        Some(vbranch.head_oid(repo)?.attach(repo))
     } else if let Some(mut branch) = local_branches.into_iter().next() {
         branch.peel_to_id_in_place_packed(packed).ok()
     } else if let Some(mut branch) = remote_branches.into_iter().next() {

crates/gitbutler-branch-actions/src/branch_manager/branch_creation.rs

@@ -289,11 +289,11 @@ impl BranchManager<'_> {
 
         let gix_repo = repo.to_gix()?;
         let merge_base = repo
-            .merge_base(default_target.sha, stack.head(&gix_repo)?.to_git2())
+            .merge_base(default_target.sha, stack.head_oid(&gix_repo)?.to_git2())
             .context(format!(
                 "failed to find merge base between {} and {}",
                 default_target.sha,
-                stack.head(&gix_repo)?
+                stack.head_oid(&gix_repo)?
             ))?;
 
         // Branch is out of date, merge or rebase it
@@ -331,7 +331,7 @@ impl BranchManager<'_> {
         // Do we need to rebase the branch on top of the default target?
 
         let has_change_id = repo
-            .find_commit(stack.head(&gix_repo)?.to_git2())?
+            .find_commit(stack.head_oid(&gix_repo)?.to_git2())?
             .change_id()
             .is_some();
         // If the branch has no change ID for the head commit, we want to rebase it even if the base is the same
@@ -351,7 +351,7 @@ impl BranchManager<'_> {
             } else {
                 gitbutler_merge_commits(
                     repo,
-                    repo.find_commit(stack.head(&gix_repo)?.to_git2())?,
+                    repo.find_commit(stack.head_oid(&gix_repo)?.to_git2())?,
                     repo.find_commit(default_target.sha)?,
                     &stack.name,
                     default_target.branch.branch(),
@@ -378,7 +378,8 @@ impl BranchManager<'_> {
 
         {
             if let Some(wip_commit_to_unapply) = &stack.not_in_workspace_wip_change_id {
-                let potential_wip_commit = repo.find_commit(stack.head(&gix_repo)?.to_git2())?;
+                let potential_wip_commit =
+                    repo.find_commit(stack.head_oid(&gix_repo)?.to_git2())?;
 
                 // Don't try to undo commit if its conflicted
                 if !potential_wip_commit.is_conflicted() {
@@ -387,7 +388,7 @@ impl BranchManager<'_> {
                             stack = crate::undo_commit::undo_commit(
                                 self.ctx,
                                 stack.id,
-                                stack.head(&gix_repo)?.to_git2(),
+                                stack.head_oid(&gix_repo)?.to_git2(),
                                 perm,
                             )?;
                         }