Merge pull request #13765 from gitbutlerapp/merge-changes-into-tree

editor: merge commit changes into tree

Author: estib-vega

crates/but-core/src/commit/mod.rs

@@ -6,6 +6,7 @@ use anyhow::{Context as _, bail};
 use bstr::{BStr, BString, ByteSlice};
 use but_error::Code;
 use gix::objs::WriteTo;
+use gix::objs::tree::EntryKind;
 use gix::prelude::ObjectIdExt;
 use serde::{Deserialize, Serialize};
 
@@ -698,8 +699,149 @@ impl ConflictEntries {
     }
 }
 
+/// Derive GitButler conflict entry metadata from a `gix` tree-merge outcome.
+///
+/// This is the shared translation used when a merge is auto-resolved but still
+/// has unresolved conflicts that must be persisted in a conflicted commit/tree.
+/// It first asks `gix` to materialize conflict stages into an index view of the
+/// merged tree. If that yields no staged conflict paths, it falls back to the
+/// unresolved conflict list from the merge outcome itself, which is important
+/// for force-resolved merges where Git would otherwise omit index stages.
+///
+/// - `repo` - is the repository that owns `merged_tree_id` and provides the index
+///   machinery used to derive stage entries from the merge result.
+///
+/// - `merged_tree_id` - is the tree written from the merge result's auto-resolved
+///   output, which is reloaded so conflict stages can be inspected.
+///
+/// - `merge_result` - is the original `gix` merge outcome whose unresolved
+///   conflicts are translated into GitButler conflict-entry metadata.
+///
+/// - `treat_as_unresolved` - decides which conflicts should still be treated as
+///   unresolved when scanning both staged entries and fallback conflict records.
+pub fn conflict_entries_from_merge_outcome(
+    repo: &gix::Repository,
+    merged_tree_id: gix::ObjectId,
+    merge_result: &gix::merge::tree::Outcome<'_>,
+    treat_as_unresolved: gix::merge::tree::TreatAsUnresolved,
+) -> anyhow::Result<ConflictEntries> {
+    use gix::index::entry::Stage;
+
+    let mut index = repo.index_from_tree(&merged_tree_id.attach(repo))?;
+    merge_result.index_changed_after_applying_conflicts(
+        &mut index,
+        treat_as_unresolved,
+        gix::merge::tree::apply_index_entries::RemovalMode::Mark,
+    );
+
+    let (mut ancestor_entries, mut our_entries, mut their_entries) =
+        (Vec::new(), Vec::new(), Vec::new());
+    for entry in index.entries() {
+        let storage = match entry.stage() {
+            Stage::Unconflicted => continue,
+            Stage::Base => &mut ancestor_entries,
+            Stage::Ours => &mut our_entries,
+            Stage::Theirs => &mut their_entries,
+        };
+        storage.push(gix::path::from_bstr(entry.path(&index)).into_owned());
+    }
+
+    let mut out = ConflictEntries {
+        ancestor_entries,
+        our_entries,
+        their_entries,
+    };
+
+    if !out.has_entries() {
+        fn push_unique(v: &mut Vec<PathBuf>, change: &gix::diff::tree_with_rewrites::Change) {
+            let path = gix::path::from_bstr(change.location()).into_owned();
+            if !v.contains(&path) {
+                v.push(path);
+            }
+        }
+
+        for conflict in merge_result
+            .conflicts
+            .iter()
+            .filter(|c| c.is_unresolved(treat_as_unresolved))
+        {
+            let (ours, theirs) = conflict.changes_in_resolution();
+            push_unique(&mut out.our_entries, ours);
+            push_unique(&mut out.their_entries, theirs);
+        }
+    }
+
+    assert_eq!(
+        out.has_entries(),
+        merge_result.has_unresolved_conflicts(treat_as_unresolved),
+        "Must have entries to indicate conflicting files, or bad things will happen later: {:#?}",
+        merge_result.conflicts
+    );
+
+    Ok(out)
+}
+
 mod conflict;
 pub use conflict::{
     add_conflict_markers, is_conflicted, message_is_conflicted,
     rewrite_conflict_markers_on_message_change, strip_conflict_markers,
 };
+
+/// Write a GitButler conflicted tree that wraps `resolved_tree_id` together
+/// with the conflict side trees and conflict file list.
+///
+/// - `repo` - is the repository that owns all tree objects involved and receives
+///   the newly written conflicted tree.
+///
+/// - `resolved_tree_id` - is the visible auto-resolved tree that callers want to
+///   present as the conflicted commit's main tree.
+///
+/// - `base_tree_id` - is the merge-base tree used for the conflicted merge step.
+///
+/// - `ours_tree_id` - is the accumulated tree on the "ours" side of the conflict.
+///
+/// - `theirs_tree_id` - is the incoming tree on the "theirs" side of the conflict.
+///
+/// - `conflict_entries` - lists the paths that should be recorded as conflicted in
+///   the synthetic GitButler tree metadata.
+pub fn write_conflicted_tree(
+    repo: &gix::Repository,
+    resolved_tree_id: gix::ObjectId,
+    base_tree_id: gix::ObjectId,
+    ours_tree_id: gix::ObjectId,
+    theirs_tree_id: gix::ObjectId,
+    conflict_entries: &ConflictEntries,
+) -> anyhow::Result<gix::ObjectId> {
+    let conflicted_files_string = toml::to_string(conflict_entries)?;
+    let conflicted_files_blob = repo.write_blob(conflicted_files_string.as_bytes())?;
+
+    let mut tree = repo.find_tree(resolved_tree_id)?.edit()?;
+    tree.upsert(
+        TreeKind::Ours.as_tree_entry_name(),
+        EntryKind::Tree,
+        ours_tree_id,
+    )?;
+    tree.upsert(
+        TreeKind::Theirs.as_tree_entry_name(),
+        EntryKind::Tree,
+        theirs_tree_id,
+    )?;
+    tree.upsert(
+        TreeKind::Base.as_tree_entry_name(),
+        EntryKind::Tree,
+        base_tree_id,
+    )?;
+    tree.upsert(
+        TreeKind::AutoResolution.as_tree_entry_name(),
+        EntryKind::Tree,
+        resolved_tree_id,
+    )?;
+    tree.upsert(
+        TreeKind::ConflictFiles.as_tree_entry_name(),
+        EntryKind::Blob,
+        conflicted_files_blob,
+    )?;
+    tree.write()
+        .context("failed to write conflicted tree")
+        .map(|tree_id| tree_id.detach())
+}

crates/but-rebase/docs/merge_commit_changes.md

@@ -0,0 +1,67 @@
+# Merge Commit Changes
+
+This module contains the editor helpers for combining selected commit changes
+into a single tree and for planning those changes as mergeable `base..commit`
+ranges.
+
+## `merge_commit_changes_to_tree`
+
+`Editor::merge_commit_changes_to_tree(target, subjects, merge_options)` builds a
+tree by:
+
+1. using the target commit's full visible tree as the baseline,
+2. asking the planner to normalize the selected subject commits into mergeable
+   change ranges,
+3. merging those planned `base_tree_id -> commit^{tree}` ranges into the
+   accumulated tree in planner-defined order, and
+4. stopping at the first unresolved merge conflict while returning the
+   auto-resolved tree plus conflict metadata.
+
+The target contributes its whole tree. Subject commits contribute only their
+selected change ranges.
+
+## `plan_commit_changes_for_merge`
+
+`Editor::plan_commit_changes_for_merge(target, subjects)` turns a set of
+selected subject commits into emitted `PlannedCommitChange` entries.
+
+The planner uses a single traversal of the editor graph, rooted from the same
+child-most entrypoints used by commit parentage ordering. During that traversal
+it derives:
+
+- canonical subject order from editor parentage,
+- selected first-parent relationships for contiguous-chain collapse, and
+- the target ancestry cone for pruning.
+
+The planner then emits only non-pruned selected-chain tips.
+
+Even though traversal and pruning are driven by the editor step graph, the
+planner still takes first-parent and tree semantics from the commit objects in
+the editor's in-memory repository. In other words:
+
+- the in-memory repository is the source of truth for commit parents, trees,
+  and SHAs,
+- the step graph is used only to walk the selected region deterministically and
+  to discover the target ancestry cone quickly, and
+- callers are expected to keep those two views aligned before using these
+  helpers.
+
+## Design Decisions
+
+- The planner is the sole source of truth for subject ordering. Call-sites
+  should pass raw selected subject IDs and not pre-order them.
+- Subject ordering is canonicalized from the editor graph, not from caller
+  input order.
+- Duplicate subject IDs are deduplicated by commit ID only.
+- Pruning uses full target ancestry across any parent edge.
+- Collapse uses only selected first-parent chains.
+- Pruned commits are never emitted as merge picks.
+- A pruned selected first parent may still define the `base_tree_id` boundary
+  for a surviving descendant, so a surviving tip can emit `B..C` even when `B`
+  is pruned.
+- The editor step graph is expected to represent the same topology as the
+  editor's in-memory repository. These helpers do not treat step-graph rewiring
+  as a new source of commit parent truth.
+- The editor is assumed to already be normalized and up to date before calling
+  these helpers. Callers chaining earlier editor mutations should normalize via
+  `editor.rebase()?.into_editor()` first.

crates/but-rebase/src/cherry_pick.rs

@@ -22,14 +22,13 @@ pub enum EmptyCommit {
     UsePrevious,
 }
 
-use std::path::PathBuf;
-
 use anyhow::{Context as _, bail};
 use bstr::BString;
-use but_core::commit::{HEADERS_CONFLICTED_FIELD, Headers, SignCommit, TreeKind};
+use but_core::commit::{
+    HEADERS_CONFLICTED_FIELD, Headers, SignCommit, TreeKind, conflict_entries_from_merge_outcome,
+};
 use but_error::bail_precondition;
 use gix::{object::tree::EntryKind, prelude::ObjectIdExt};
-use serde::Serialize;
 
 use crate::commit::DateMode;
 
@@ -197,8 +196,12 @@ fn commit_from_conflicted_tree<'repo>(
 ) -> anyhow::Result<gix::Id<'repo>> {
     let repo = resolved_tree_id.repo;
 
-    let conflicted_files =
-        extract_conflicted_files(resolved_tree_id, cherry_pick, treat_as_unresolved)?;
+    let conflicted_files = conflict_entries_from_merge_outcome(
+        repo,
+        resolved_tree_id.detach(),
+        &cherry_pick,
+        treat_as_unresolved,
+    )?;
 
     // convert files into a string and save as a blob
     let conflicted_files_string = toml::to_string(&conflicted_files)?;
@@ -250,85 +253,3 @@ fn commit_from_conflicted_tree<'repo>(
     )?
     .attach(repo))
 }
-
-fn extract_conflicted_files(
-    merged_tree_id: gix::Id<'_>,
-    merge_result: gix::merge::tree::Outcome<'_>,
-    treat_as_unresolved: gix::merge::tree::TreatAsUnresolved,
-) -> anyhow::Result<ConflictEntries> {
-    use gix::index::entry::Stage;
-    let repo = merged_tree_id.repo;
-    let mut index = repo.index_from_tree(&merged_tree_id)?;
-    merge_result.index_changed_after_applying_conflicts(
-        &mut index,
-        treat_as_unresolved,
-        gix::merge::tree::apply_index_entries::RemovalMode::Mark,
-    );
-    let (mut ancestor_entries, mut our_entries, mut their_entries) =
-        (Vec::new(), Vec::new(), Vec::new());
-    for entry in index.entries() {
-        let stage = entry.stage();
-        let storage = match stage {
-            Stage::Unconflicted => {
-                continue;
-            }
-            Stage::Base => &mut ancestor_entries,
-            Stage::Ours => &mut our_entries,
-            Stage::Theirs => &mut their_entries,
-        };
-
-        let path = entry.path(&index);
-        storage.push(gix::path::from_bstr(path).into_owned());
-    }
-    let mut out = ConflictEntries {
-        ancestor_entries,
-        our_entries,
-        their_entries,
-    };
-
-    // Since we typically auto-resolve with 'ours', it maybe that conflicting entries don't have an
-    // unconflicting counterpart anymore, so they are not applied (which is also what Git does).
-    // So to have something to show for - we *must* produce a conflict, extract paths manually.
-    // TODO(ST): instead of doing this, don't pre-record the paths. Instead redo the merge without
-    //           merge-strategy so that the index entries can be used instead.
-    if !out.has_entries() {
-        fn push_unique(v: &mut Vec<PathBuf>, change: &gix::diff::tree_with_rewrites::Change) {
-            let path = gix::path::from_bstr(change.location()).into_owned();
-            if !v.contains(&path) {
-                v.push(path);
-            }
-        }
-        for conflict in merge_result
-            .conflicts
-            .iter()
-            .filter(|c| c.is_unresolved(treat_as_unresolved))
-        {
-            let (ours, theirs) = conflict.changes_in_resolution();
-            push_unique(&mut out.our_entries, ours);
-            push_unique(&mut out.their_entries, theirs);
-        }
-    }
-    assert_eq!(
-        out.has_entries(),
-        merge_result.has_unresolved_conflicts(treat_as_unresolved),
-        "Must have entries to indicate conflicting files, or bad things will happen later: {:#?}",
-        merge_result.conflicts
-    );
-    Ok(out)
-}
-
-#[derive(Default, Debug, Clone, Serialize, PartialEq)]
-#[serde(rename_all = "camelCase")]
-pub(crate) struct ConflictEntries {
-    pub(crate) ancestor_entries: Vec<PathBuf>,
-    pub(crate) our_entries: Vec<PathBuf>,
-    pub(crate) their_entries: Vec<PathBuf>,
-}
-
-impl ConflictEntries {
-    pub(crate) fn has_entries(&self) -> bool {
-        !self.ancestor_entries.is_empty()
-            || !self.our_entries.is_empty()
-            || !self.their_entries.is_empty()
-    }
-}