Merge pull request #6180 from gitbutlerapp/kv-branch-3

Reduce reliance on change id

crates/but-rebase/src/lib.rs

@@ -72,6 +72,7 @@ pub struct Rebase<'repo> {
     base: Option<gix::ObjectId>,
     base_substitute: Option<gix::ObjectId>,
     steps: Vec<RebaseStep>,
+    rebase_noops: bool,
 }
 
 impl<'repo> Rebase<'repo> {
@@ -99,6 +100,7 @@ impl<'repo> Rebase<'repo> {
             base,
             base_substitute,
             steps: Vec::new(),
+            rebase_noops: true, // default to always rebasing
         })
     }
 
@@ -113,6 +115,14 @@ impl<'repo> Rebase<'repo> {
         Ok(self)
     }
 
+    /// Configures whether the noop steps should be rebased regardless.
+    /// If set to true, commits that dont really change will have their timestamps and ids updated.
+    /// Default is `true`
+    pub fn rebase_noops(&mut self, value: bool) -> &mut Self {
+        self.rebase_noops = value;
+        self
+    }
+
     /// Performs a rebase on top of a given base, according to the provided steps, or fails if no step was provided.
     /// It does not actually create new git references nor does it update existing ones, it only deals with
     /// altering commits and providing the information needed to update refs.
@@ -135,6 +145,7 @@ impl<'repo> Rebase<'repo> {
             self.base,
             self.base_substitute,
             std::mem::take(&mut self.steps),
+            self.rebase_noops,
         )
     }
 }
@@ -211,6 +222,7 @@ fn rebase(
     base: Option<gix::ObjectId>,
     base_substitute: Option<gix::ObjectId>,
     steps: Vec<RebaseStep>,
+    rebase_noops: bool,
 ) -> Result<RebaseOutput> {
     let git2_repo = git2::Repository::open(repo.path())?;
     let (mut references, mut commit_mapping) = (
@@ -258,7 +270,7 @@ fn rebase(
                                 &git2_repo,
                                 cursor.to_git2(),
                                 &[commit_id.to_git2()],
-                                true,
+                                rebase_noops,
                                 true,
                             )?
                             .to_gix();

crates/gitbutler-branch-actions/Cargo.toml

@@ -33,6 +33,8 @@ gitbutler-stack.workspace = true
 gitbutler-hunk-dependency.workspace = true
 gitbutler-workspace.workspace = true
 but-workspace.workspace = true
+but-rebase.workspace = true
+but-core.workspace = true
 serde = { workspace = true, features = ["std"] }
 serde-error = "0.1.3"
 bstr.workspace = true

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

@@ -381,7 +381,8 @@ pub fn reorder_stack(
         SnapshotDetails::new(OperationKind::ReorderCommit),
         guard.write_permission(),
     );
-    reorder::reorder_stack(ctx, stack_id, stack_order, guard.write_permission())
+    reorder::reorder_stack(ctx, stack_id, stack_order, guard.write_permission())?;
+    Ok(())
 }
 
 pub fn reset_virtual_branch(

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

@@ -1,10 +1,11 @@
 use std::collections::HashMap;
 
 use anyhow::{bail, Context, Result};
+use but_rebase::{RebaseOutput, RebaseStep};
 use git2::{Commit, Oid};
 use gitbutler_command_context::CommandContext;
+use gitbutler_oxidize::{ObjectIdExt, OidExt};
 use gitbutler_project::access::WorktreeWritePermission;
-use gitbutler_repo::rebase::cherry_rebase_group;
 use gitbutler_stack::{
     stack_context::{CommandContextExt, StackContext},
     Stack, StackId,
@@ -34,7 +35,7 @@ pub fn reorder_stack(
     stack_id: StackId,
     new_order: StackOrder,
     perm: &mut WorktreeWritePermission,
-) -> Result<()> {
+) -> Result<RebaseOutput> {
     let state = ctx.project().virtual_branches();
     let repo = ctx.repo();
     let mut stack = state.get_stack(stack_id)?;
@@ -48,13 +49,26 @@ pub fn reorder_stack(
     let old_head = repo.find_commit(stack.head())?;
     let merge_base = repo.merge_base(default_target_commit.id(), stack.head())?;
 
-    let ids_to_rebase = new_order
-        .series
-        .iter()
-        .flat_map(|s| s.commit_ids.iter())
-        .cloned()
-        .collect_vec();
-    let new_head = cherry_rebase_group(repo, merge_base, &ids_to_rebase, false, false)?;
+    let mut steps: Vec<RebaseStep> = Vec::new();
+    for series in new_order.series.iter().rev() {
+        for oid in series.commit_ids.iter().rev() {
+            steps.push(RebaseStep::Pick {
+                commit_id: oid.to_gix(),
+                new_message: None,
+            });
+        }
+        steps.push(RebaseStep::Reference(but_core::Reference::Virtual(
+            series.name.clone(),
+        )));
+    }
+    let gix_repo = ctx.gix_repository()?;
+    let mut builder = but_rebase::Rebase::new(&gix_repo, merge_base.to_gix(), None)?;
+    let builder = builder.steps(steps)?;
+    builder.rebase_noops(false);
+    let output = builder.rebase()?;
+
+    let new_head = output.top_commit.to_git2();
+
     // Calculate the new head and tree
     let BranchHeadAndTree {
         head: new_head_oid,
@@ -65,15 +79,11 @@ pub fn reorder_stack(
     stack.set_stack_head(ctx, new_head_oid, Some(new_tree_oid))?;
 
     let mut new_heads: HashMap<String, Commit<'_>> = HashMap::new();
-    let mut previous = merge_base;
-    for series in new_order.series.iter().rev() {
-        let commit = if let Some(commit_id) = series.commit_ids.first() {
-            repo.find_commit(*commit_id)?
-        } else {
-            repo.find_commit(previous)?
-        };
-        previous = commit.id();
-        new_heads.insert(series.name.clone(), commit);
+    for reference in &output.references {
+        let commit = repo.find_commit(reference.commit_id.to_git2())?;
+        if let but_core::Reference::Virtual(name) = &reference.reference {
+            new_heads.insert(name.clone(), commit);
+        }
     }
     // Set the series heads accordingly in one go
     stack.set_all_heads(ctx, new_heads)?;
@@ -82,7 +92,7 @@ pub fn reorder_stack(
     crate::integration::update_workspace_commit(&state, ctx)
         .context("failed to update gitbutler workspace")?;
 
-    Ok(())
+    Ok(output)
 }
 
 /// Represents the order of series (branches) and changes (commits) in a stack.

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

@@ -1,4 +1,7 @@
+use std::collections::HashMap;
+
 use anyhow::{bail, Context, Ok, Result};
+use but_rebase::RebaseStep;
 use gitbutler_command_context::CommandContext;
 use gitbutler_commit::{commit_ext::CommitExt, commit_headers::HasCommitHeaders};
 use gitbutler_oplog::{
@@ -9,7 +12,6 @@ use gitbutler_oxidize::{GixRepositoryExt, ObjectIdExt, OidExt};
 use gitbutler_project::access::WorktreeWritePermission;
 use gitbutler_repo::{
     logging::{LogUntil, RepositoryExt},
-    rebase::cherry_rebase_group,
     RepositoryExt as _,
 };
 use gitbutler_stack::{stack_context::CommandContextExt, StackId};
@@ -45,7 +47,7 @@ pub(crate) fn squash_commits(
 fn do_squash_commits(
     ctx: &CommandContext,
     stack_id: StackId,
-    source_ids: Vec<git2::Oid>,
+    mut source_ids: Vec<git2::Oid>,
     desitnation_id: git2::Oid,
     perm: &mut WorktreeWritePermission,
 ) -> Result<()> {
@@ -72,9 +74,25 @@ fn do_squash_commits(
             branch.commit_ids.splice(pos..pos, source_ids.clone());
         }
     }
-    if order != updated_order {
-        reorder_stack(ctx, stack_id, updated_order, perm)?;
-    }
+    let mapping = if order != updated_order {
+        Some(reorder_stack(ctx, stack_id, updated_order, perm)?.commit_mapping)
+    } else {
+        None
+    };
+
+    // update source ids from the mapping if present
+    if let Some(mapping) = mapping {
+        for (_, old, new) in mapping.iter() {
+            // if source_ids contains old, replace it with new
+            if source_ids.contains(&old.to_git2()) {
+                let index = source_ids
+                    .iter()
+                    .position(|id| id == &old.to_git2())
+                    .unwrap();
+                source_ids[index] = new.to_git2();
+            }
+        }
+    };
 
     // =========== Step 2: Squash
 
@@ -141,23 +159,38 @@ fn do_squash_commits(
         )
         .context("Failed to create a squash commit")?;
 
-    // ids_to_rebase is the list the original list of commit ids (branch_commit_ids) with the source commits removed and the
-    // destination commit replaced with the new commit
-    let ids_to_rebase = branch_commit_oids
-        .iter()
-        .filter(|id| !source_ids.contains(id))
-        .map(|id| {
-            if *id == destination_commit.id() {
-                new_commit_oid
-            } else {
-                *id
-            }
-        })
-        .collect::<Vec<_>>();
+    let mut steps: Vec<RebaseStep> = Vec::new();
+
+    for head in stack.heads_by_commit(ctx.repo().find_commit(merge_base)?) {
+        steps.push(RebaseStep::Reference(but_core::Reference::Virtual(head)));
+    }
+    for oid in branch_commit_oids.iter().rev() {
+        let commit = ctx.repo().find_commit(*oid)?;
+        if source_ids.contains(oid) {
+            // noop - skipping this
+        } else if destination_commit.id() == *oid {
+            steps.push(RebaseStep::Pick {
+                commit_id: new_commit_oid.to_gix(),
+                new_message: None,
+            });
+        } else {
+            steps.push(RebaseStep::Pick {
+                commit_id: oid.to_gix(),
+                new_message: None,
+            });
+        }
+        for head in stack.heads_by_commit(commit) {
+            steps.push(RebaseStep::Reference(but_core::Reference::Virtual(head)));
+        }
+    }
 
-    // Rebase the commits in the stack so that the source commits are removed and the destination commit
-    // is replaces with a new commit with the final tree that is the result of the merge
-    let new_stack_head = cherry_rebase_group(ctx.repo(), merge_base, &ids_to_rebase, false, false)?;
+    let gix_repo = ctx.gix_repository()?;
+    let mut builder = but_rebase::Rebase::new(&gix_repo, merge_base.to_gix(), None)?;
+    let builder = builder.steps(steps)?;
+    builder.rebase_noops(false);
+    let output = builder.rebase()?;
+
+    let new_stack_head = output.top_commit.to_git2();
 
     let BranchHeadAndTree {
         head: new_head_oid,
@@ -170,18 +203,15 @@ fn do_squash_commits(
     crate::integration::update_workspace_commit(&vb_state, ctx)
         .context("failed to update gitbutler workspace")?;
 
-    // Finally, update branch heads in the stack if present
-    for source_commit in &source_commits {
-        // Find the next eligible ancestor commit that is not in the source commits
-        let mut ancestor = source_commit.parent(0)?;
-        while source_commits.iter().any(|c| c.id() == ancestor.id()) {
-            if ancestor.id() == merge_base {
-                break; // Don's search past the merge base
-            }
-            ancestor = ancestor.parent(0)?;
+    let mut new_heads: HashMap<String, git2::Commit<'_>> = HashMap::new();
+    for reference in output.references {
+        let commit = ctx.repo().find_commit(reference.commit_id.to_git2())?;
+        if let but_core::Reference::Virtual(name) = reference.reference {
+            new_heads.insert(name, commit);
         }
-        stack.replace_head(ctx, source_commit, &ancestor)?;
     }
+    // Set the series heads accordingly in one go
+    stack.set_all_heads(ctx, new_heads)?;
     Ok(())
 }
 

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

@@ -183,11 +183,7 @@ pub fn push_stack(ctx: &CommandContext, stack_id: StackId, with_force: bool) ->
     let repo = ctx.repo();
     let default_target = state.get_default_target()?;
     let merge_base = repo.find_commit(repo.merge_base(stack.head(), default_target.sha)?)?;
-    let merge_base = if let Some(change_id) = merge_base.change_id() {
-        CommitOrChangeId::ChangeId(change_id)
-    } else {
-        CommitOrChangeId::CommitId(merge_base.id().to_string())
-    };
+    let merge_base: CommitOrChangeId = merge_base.into();
 
     // First fetch, because we dont want to push integrated series
     ctx.fetch(

crates/gitbutler-stack/src/heads.rs

@@ -145,15 +145,15 @@ mod test {
     #[test]
     fn add_head_with_archived_bottom_head() -> Result<()> {
         let head_1_archived = StackBranch {
-            head: CommitOrChangeId::ChangeId("328447a2-08aa-4c4d-a1bc-08d5cd82bcd4".to_string()),
+            head: CommitOrChangeId::CommitId("328447a2-08aa-4c4d-a1bc-08d5cd82bcd4".to_string()),
             name: "kv-branch-3".to_string(),
             description: None,
             pr_number: None,
             archived: true,
             review_id: None,
         };
         let head_2 = StackBranch {
-            head: CommitOrChangeId::ChangeId("11609175-039d-44ee-9d4a-6baa9ad2a750".to_string()),
+            head: CommitOrChangeId::CommitId("11609175-039d-44ee-9d4a-6baa9ad2a750".to_string()),
             name: "more-on-top".to_string(),
             description: None,
             pr_number: None,
@@ -162,16 +162,16 @@ mod test {
         };
         let existing_heads = vec![head_1_archived.clone(), head_2.clone()];
         let new_head = StackBranch {
-            head: CommitOrChangeId::ChangeId("11609175-039d-44ee-9d4a-6baa9ad2a750".to_string()),
+            head: CommitOrChangeId::CommitId("11609175-039d-44ee-9d4a-6baa9ad2a750".to_string()),
             name: "abcd".to_string(),
             description: None,
             pr_number: None,
             archived: false,
             review_id: None,
         };
         let patches = vec![
-            CommitOrChangeId::ChangeId("92a89ae608d77ff75c1ce52ea9dccc0bccd577e9".to_string()),
-            CommitOrChangeId::ChangeId("11609175-039d-44ee-9d4a-6baa9ad2a750".to_string()),
+            CommitOrChangeId::CommitId("92a89ae608d77ff75c1ce52ea9dccc0bccd577e9".to_string()),
+            CommitOrChangeId::CommitId("11609175-039d-44ee-9d4a-6baa9ad2a750".to_string()),
         ];
 
         let updated_heads = add_head(