commit squash: use merge arithmetic

Use merge arithmetic instead of reordering the commits for a faster,
more reliable squash.

Author: estib-vega

crates/but-workspace/src/commit/squash_commits.rs

@@ -1,11 +1,13 @@
 //! An action to squash multiple commits into a target commit.
 
 use anyhow::{Result, bail};
-use but_core::RefMetadata;
+use but_core::{RefMetadata, RepositoryExt};
 use but_rebase::{
     commit::DateMode,
     graph_rebase::{
-        Editor, Selector, Step, SuccessfulRebase, ToCommitSelector, mutate::InsertSide,
+        Editor, LookupStep as _, Selector, Step, SuccessfulRebase, ToCommitSelector,
+        commit::MergeCommitChangesOutcome,
+        mutate::{SegmentDelimiter, SelectorSet},
     },
 };
 
@@ -43,73 +45,56 @@ fn push_message_with_spacing(combined: &mut Vec<u8>, message: &[u8]) {
     combined.extend_from_slice(message);
 }
 
-/// Reorder commits around `target_commit` so all selected commits become
-/// adjacent around the target in parentage order.
-///
-/// Returns the rewritten editor together with the original below/above anchor
-/// commit IDs used for remapping after the first rebase.
-fn reorder_commits_around_target<'ws, 'meta, M: RefMetadata>(
-    mut editor: Editor<'ws, 'meta, M>,
-    ordered_all_commits: &[Selector],
-    target_commit: Selector,
-) -> Result<(Editor<'ws, 'meta, M>, Selector, Selector)> {
-    let target_pos = ordered_all_commits
-        .iter()
-        .position(|id| *id == target_commit)
-        .expect("target commit must be in ordered commit list");
-
-    let (below_commits, target_and_above_commits) = ordered_all_commits.split_at(target_pos);
-
-    let mut below_anchor = target_commit;
-    for source_id in below_commits.iter().rev().copied() {
-        editor = crate::commit::move_commit_no_rebase(
-            editor,
-            source_id,
-            below_anchor,
-            InsertSide::Below,
-        )?;
-        below_anchor = source_id;
-    }
-
-    let mut above_anchor = target_commit;
-    for source_id in target_and_above_commits.iter().skip(1).copied() {
-        editor = crate::commit::move_commit_no_rebase(
-            editor,
-            source_id,
-            above_anchor,
-            InsertSide::Above,
-        )?;
-        above_anchor = source_id;
-    }
-
-    Ok((editor, below_anchor, above_anchor))
-}
-
-/// Build the squashed commit from the mapped top/bottom commits and replace the
-/// bottom selector with the newly created commit.
+/// Build the squashed commit and replace the target selector with the newly
+/// created commit.
 ///
 /// Returns the updated editor and the selector that now points to the squashed
 /// commit.
 fn construct_new_squashed_commit<'ws, 'meta, M: RefMetadata>(
     mut editor: Editor<'ws, 'meta, M>,
-    top_most_commit_id: Selector,
-    bottom_most_commit_id: Selector,
+    squashed_tree: MergeCommitChangesOutcome,
+    target_commit_id: Selector,
     combined_message: Vec<u8>,
 ) -> Result<(Editor<'ws, 'meta, M>, Selector)> {
-    let (_, top_most_commit) = editor.find_selectable_commit(top_most_commit_id)?;
-    let (bottom_most_selector, bottom_most_commit) =
-        editor.find_selectable_commit(bottom_most_commit_id)?;
+    let (target_selector, target_commit) = editor.find_selectable_commit(target_commit_id)?;
+    let target_parent_ids = parent_commit_ids(&editor, target_selector)?;
 
     let new_commit = {
-        let mut squashed_commit = bottom_most_commit.clone();
-        squashed_commit.tree = top_most_commit.tree;
+        let mut squashed_commit = target_commit.clone();
+        squashed_commit.inner.parents = target_parent_ids.into();
+        squashed_commit.tree = squashed_tree.tree_id;
         squashed_commit.message = combined_message.into();
         editor.new_commit(squashed_commit, DateMode::CommitterUpdateAuthorKeep)?
     };
 
-    editor.replace(bottom_most_selector, Step::new_pick(new_commit))?;
+    editor.replace(target_selector, Step::new_pick(new_commit))?;
 
-    Ok((editor, bottom_most_selector))
+    Ok((editor, target_selector))
+}
+
+fn parent_commit_ids<M: RefMetadata>(
+    editor: &Editor<'_, '_, M>,
+    selector: Selector,
+) -> Result<Vec<gix::ObjectId>> {
+    let mut parents = editor.direct_parents(selector)?;
+    parents.sort_by_key(|(_, order)| *order);
+
+    parents
+        .into_iter()
+        .map(|(parent_selector, _)| match editor.lookup_step(parent_selector)? {
+            Step::Pick(_) => {
+                let (_, commit) = editor.find_selectable_commit(parent_selector)?;
+                Ok(commit.id)
+            }
+            Step::Reference { .. } => {
+                let (_, commit) = editor.find_reference_target(parent_selector)?;
+                Ok(commit.id)
+            }
+            Step::None => bail!(
+                "BUG: expected parent selector {parent_selector:?} to point to a pick or reference"
+            ),
+        })
+        .collect()
 }
 
 /// How to combine messages of commits being squashed.
@@ -133,72 +118,67 @@ but_schemars::register_sdk_type!(MessageCombinationStrategy);
 
 /// Squash `subjects` into `target_commit`.
 ///
-/// `subjects` may be provided in any order. They are ordered by
-/// parentage internally together with `target_commit` before reordering and
-/// squashing.
-///
 /// The `target_commit` must not also appear in `subjects`.
 ///
-/// After reordering and squashing, the resulting squashed commit has:
-/// - The tree of the commit that is top-most after reordering.
+/// After squashing, the resulting squashed commit has:
+/// - The tree produced by merging the selected commits together.
 /// - A message determined by `how_to_combine_messages`:
 ///   - `KeepTarget`: target message only.
 ///   - `KeepSubject`: subject messages only.
 ///   - `KeepBoth`: target message followed by subject messages.
 ///
-/// Subject messages are appended in squash order (top-most first after
-/// reordering), with at least one blank line between non-empty message blocks.
+/// Subject messages are appended in the order they are provided, with at least
+/// one blank line between non-empty message blocks.
 ///
 pub fn squash_commits<'ws, 'meta, M: RefMetadata, S: ToCommitSelector, T: ToCommitSelector>(
     editor: Editor<'ws, 'meta, M>,
     subjects: Vec<S>,
     target_commit: T,
     how_to_combine_messages: MessageCombinationStrategy,
 ) -> Result<SquashCommitsOutcome<'ws, 'meta, M>> {
+    let mut seen_subjects = std::collections::HashSet::with_capacity(subjects.len());
+
     if subjects.is_empty() {
         bail!("Need at least 2 commits to squash")
     }
 
     let (target_commit_selector, target_commit_obj) =
         editor.find_selectable_commit(target_commit)?;
 
-    let mut all_commits = Vec::with_capacity(subjects.len() + 1);
-    all_commits.push(target_commit_selector);
+    let mut subject_selectors = Vec::with_capacity(subjects.len());
     for subject_commit in subjects {
         let (subject_commit_selector, _) = editor.find_selectable_commit(subject_commit)?;
         if subject_commit_selector == target_commit_selector {
             bail!("Cannot squash a commit into itself")
         }
-        all_commits.push(subject_commit_selector);
+        if !seen_subjects.insert(subject_commit_selector) {
+            continue;
+        }
+        subject_selectors.push(subject_commit_selector);
     }
 
-    let ordered_selectors = editor.order_commit_selectors_by_parentage(all_commits)?;
-
-    let (editor, below_anchor, above_anchor) =
-        reorder_commits_around_target(editor, &ordered_selectors, target_commit_selector)?;
-
-    let rebase = editor.rebase()?;
-    let editor = rebase.into_editor();
-
-    for commit_selector in &ordered_selectors {
-        let (_, commit) = editor.find_selectable_commit(*commit_selector)?;
-        if commit.clone().attach(editor.repo()).is_conflicted() {
-            bail!(
-                "Commit {} became conflicted after reordering. Can't continue with squash.",
-                commit.id
-            );
-        }
+    let mut commits_to_merge = Vec::with_capacity(subject_selectors.len() + 1);
+    commits_to_merge.push(target_commit_selector);
+    commits_to_merge.extend(subject_selectors.iter().copied());
+    let commits_to_merge = editor.order_commit_selectors_by_parentage(commits_to_merge)?;
+    let squashed_tree = editor.merge_commit_changes_to_tree(
+        commits_to_merge
+            .iter()
+            .map(|commit_selector| {
+                let (_, commit) = editor.find_selectable_commit(*commit_selector)?;
+                Ok(commit.id)
+            })
+            .collect::<Result<Vec<_>>>()?,
+        editor.repo().merge_options_force_ours()?,
+    )?;
+    if squashed_tree.conflict.is_some() {
+        bail!("Cannot squash commits that would result in merge conflicts");
     }
 
     let mut combined_message = Vec::new();
     match how_to_combine_messages {
         MessageCombinationStrategy::KeepSubject => {
-            for source_id in ordered_selectors
-                .iter()
-                .rev()
-                .copied()
-                .filter(|commit_selector| *commit_selector != target_commit_selector)
-            {
+            for source_id in subject_selectors.iter().copied() {
                 let (_, source_commit) = editor.find_selectable_commit(source_id)?;
                 push_message_with_spacing(&mut combined_message, source_commit.message.as_ref());
             }
@@ -208,37 +188,34 @@ pub fn squash_commits<'ws, 'meta, M: RefMetadata, S: ToCommitSelector, T: ToComm
         }
         MessageCombinationStrategy::KeepBoth => {
             push_message_with_spacing(&mut combined_message, target_commit_obj.message.as_ref());
-            for source_id in ordered_selectors
-                .iter()
-                .rev()
-                .copied()
-                .filter(|commit_selector| *commit_selector != target_commit_selector)
-            {
+            for source_id in subject_selectors.iter().copied() {
                 let (_, source_commit) = editor.find_selectable_commit(source_id)?;
                 push_message_with_spacing(&mut combined_message, source_commit.message.as_ref());
             }
         }
     }
 
-    let (editor, bottom_most_selector) =
-        construct_new_squashed_commit(editor, above_anchor, below_anchor, combined_message)?;
-
-    let rebase = editor.rebase()?;
-    let mut editor = rebase.into_editor();
-
-    for commit_selector in ordered_selectors {
-        if commit_selector == below_anchor {
-            continue;
-        }
-        let Ok((selector, _)) = editor.find_selectable_commit(commit_selector) else {
-            continue;
+    let mut editor = editor;
+    for commit_selector in subject_selectors {
+        let delimiter = SegmentDelimiter {
+            child: commit_selector,
+            parent: commit_selector,
         };
+        editor.disconnect_segment_from(delimiter, SelectorSet::All, SelectorSet::All, false)?;
+        let (selector, _) = editor.find_selectable_commit(commit_selector)?;
         editor.replace(selector, Step::None)?;
     }
 
+    let (editor, new_target_selector) = construct_new_squashed_commit(
+        editor,
+        squashed_tree,
+        target_commit_selector,
+        combined_message,
+    )?;
+
     Ok(SquashCommitsOutcome {
         rebase: editor.rebase()?,
-        commit_selector: bottom_most_selector,
+        commit_selector: new_target_selector,
     })
 }
 

crates/but-workspace/tests/fixtures/scenario/ws-ref-ws-commit-single-stack-double-stack-files.sh

@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+
+set -eu -o pipefail
+
+source "${BASH_SOURCE[0]%/*}/shared.sh"
+
+### General Description
+
+# A ws-ref points to a workspace commit, with two stacks inside:
+# - One stack `A` with file-a changes
+# - Another stack with `B` introducing file-b and `C` introducing file-c
+git init
+
+echo base >base
+git add base
+git commit -m M
+setup_target_to_match_main
+
+git branch B
+git checkout -b A
+  echo a >file-a
+  git add file-a
+  git commit -m A
+git checkout B
+  echo b >file-b
+  git add file-b
+  git commit -m B
+git checkout B -b C
+  echo c >file-c
+  git add file-c
+  git commit -m C
+create_workspace_commit_once A C