editor: merge commit changes into tree#13765
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new low-level repository helpers in but-core to (1) plan and merge selected commits’ change ranges into a single tree (avoiding unselected-parent state leakage), and (2) persist GitButler-style conflicted trees with side trees + conflict metadata. This is intended as foundational support for squash-style operations that combine only chosen commit changes.
Changes:
- Introduces
RepositoryExt::merge_commit_changes_to_tree()plus planning/output types (PlannedCommitChange,MergeCommitChangesOutcome,MergeCommitChangesConflict). - Adds
commit::write_conflicted_tree()for writing GitButler conflicted wrapper trees. - Adds multiple scripted fixture scenarios and integration tests covering octopus merges, non-contiguous selections, unselected-parent exclusion, and fail-fast-on-conflict behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/but-core/src/repo_ext.rs | Implements merge planning + merge execution into a single tree; adds conflict metadata extraction utilities. |
| crates/but-core/src/lib.rs | Re-exports new merge/planning types and helpers from repo_ext. |
| crates/but-core/src/commit/mod.rs | Adds write_conflicted_tree() to persist GitButler conflicted trees. |
| crates/but-core/tests/core/repo_ext.rs | Adds integration tests validating merge planning and resulting trees across multiple graphs. |
| crates/but-core/tests/core/main.rs | Registers the new repo_ext integration test module. |
| crates/but-core/tests/fixtures/scenario/three-branches-three-commits.sh | New fixture repo with three branches and three commits each for planner behavior tests. |
| crates/but-core/tests/fixtures/scenario/octopus-merge-with-redundant-input.sh | New fixture to compare “merge selected changes” vs a clean octopus merge result. |
| crates/but-core/tests/fixtures/scenario/merge-commits-preserve-noncontiguous-selected-changes.sh | New fixture to validate skipping unselected middle commits while keeping later selected changes. |
| crates/but-core/tests/fixtures/scenario/merge-commits-excludes-unselected-parent.sh | New fixture to validate excluding unselected parent changes when merging selected descendants. |
| crates/but-core/tests/fixtures/scenario/merge-commit-changes-fail-fast-after-conflict.sh | New fixture to validate fail-fast folding when an earlier merge step conflicts. |
0d1fc30 to
f4b2636
Compare
There was a problem hiding this comment.
This is interesting!
As it's implemented on an immutable &gix::Repository, it's naturally indifferent to whether the merge is in-memory or not, letting the caller decide. Thus it works nicely in dry-run mode.
This PR will contain CommitId based merge-base helpers that you could already use here (related to the comment below).
| } | ||
|
|
||
| let merge_base = self | ||
| .merge_base_octopus(merge_plan.iter().map(|change| change.commit_id)) |
There was a problem hiding this comment.
While I wouldn't push for this just yet, I think once but-graph is more obviously commit oriented and Segemnts went away (which really are just logical structuring without functional impact), then it would be natural to the graph to find the merge-base.
The octopus-merge already exists here.
gitbutler/crates/but-graph/src/projection/workspace/init.rs
Lines 148 to 151 in 6ea4ec0
If that was a method on but-graph, it could be used today, and creating one is no problem either.
The inconvenience is just the translation from CommitId to SegmentId, for which helpers could also exist or maybe a mapping even. The benefit of this is just performance in the form of "not doing work twice".
So this is just a note, a heads-up, and a note to self that getting rid of all gix::Repository based merge-base computations is a worthwhile pursuit.
CC @Caleb-T-Owens this is what would motivate me to restructure but-graph to support this, segments or not. Just needs a mapping. And nowadays, while talking about it, it can actually be done :D.
There was a problem hiding this comment.
Just to keep this up-to-date:
- the octopus-merge-like behaviour above actually has a bug that comes to light when there are disjoint histories. Rare, but there now is a real octopus merge implementation, and this one as some tests seem to rely on that behaviour.
- Helpers for using the graph with commit-ids as input instead of segment-ids now exist. The expectation is that everything that is related to the workspace is contained in the graph, everything else is a bug.
I wrote
While I wouldn't push for this just yet, I think once but-graph is more obviously commit oriented and Segemnts went away (which really are just logical structuring without functional impact), then it would be natural to the graph to find the merge-base.
Since then there was a clarification of invariants and additional validation to be sure these are met. This makes clear that Segments are fine and useful, even though they also add complexity. That complexity is mostly tamed and if not, it should be. So not having segments isn't anything I will be working on. Instead, I see them as a way to compress a graph so traversals are faster, most of which we won't need anymore as each Commit is classified via flags:
- only on remote
- in workspace
- integrated
- in workspace and integrated
2d99db0 to
9a9d19e
Compare
9a9d19e to
d30a60c
Compare
Moved the function to the editorI opted for moving the function to the editor. The editor has access to the repo and to the workspace graph which I need in order to do the merging of trees while using the new WS functions. We need to recreate the WS from an overlayThis also means that doing this, needs to re-traverse the the graph and build the WS before doing the merge. Let me know what you think |
d30a60c to
c2adc8b
Compare
c2adc8b to
7dbbe8c
Compare
There was a problem hiding this comment.
HELL YEAH!
I think the additions to but-core are Ok, but wonder where the newly added shell scripts are used. It's like I am blind and am missing something, but also am just looking at GH.
Otherwise it's good to merge from my side, but leave everything else to @Caleb-T-Owens .
PS: For an extra haleluja, the function docs could mention every parameter in backticks.
🙌
For nothing, they are remnants now. Will remove them.
Will dooooooo 👍 |
7dbbe8c to
2ac95fb
Compare
2ac95fb to
cc90fcf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
crates/but-rebase/src/cherry_pick.rs:235
- Now that
but_core::commit::write_conflicted_tree()exists, this function’s manual conflicted-tree construction (writing the conflict-files blob + upserting the conflict side trees) is duplicated in multiple places. Consider delegating the tree-editing portion towrite_conflicted_treeto keep the conflicted-tree layout consistent and reduce maintenance burden (leaving this function to handle header/message updates and parent wiring).
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)?;
let conflicted_files_blob = repo.write_blob(conflicted_files_string.as_bytes())?;
let mut tree = repo.find_tree(resolved_tree_id)?.edit()?;
// save the state of the conflict, so we can recreate it later
let (base_tree_id, ours_tree_id, theirs_tree_id) = find_cherry_pick_trees(&head, &to_rebase)?;
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(".conflict-files", EntryKind::Blob, conflicted_files_blob)?;
crates/but-rebase/src/graph_rebase/cherry_pick.rs:417
- This function still manually writes the conflicted tree (conflict side subtrees +
.conflict-filesblob). Sincebut_core::commit::write_conflicted_tree()was introduced in this PR, consider using it here as well to avoid duplicated conflicted-tree construction logic and ensure all call sites stay in sync if the conflict-tree format changes.
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)?;
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(".conflict-files", EntryKind::Blob, conflicted_files_blob)?;
26cf8ca to
e550b59
Compare
Add a function to the editor to merge the changes in from different commits into a tree that can be used for e.g. squashing commits.
Add code to write the GitButler conflicted tree out.
Deduplicate the code related to the extraction of GitButler conflict metadata out of gix merge outcomes. Share it across rebase, and gitbutler-repo.
e550b59 to
3be8ea6
Compare
|
For the preservation of context: The way the function performs the merging of trees and figuring out of merge-bases is documented in the code and in a MD file I added as module docs. In summary, we traverse the workspace and figure out the order of the subject commits, whether they are parents to each other, and wether they are already part of the ancestry of the target commit. Based on that information, we fold in the changes into the target's tree. |
Add abut-corerepository helper for combining the changes introduced by selected commits into a single tree, along with support for writing GitButler conflicted trees.Add a Editor helper to merge the changes from different commits into a tree that can be used for a new commit.
This introduces the low-level building blocks needed for squash-style operations that should merge only the selected commit changes, without accidentally pulling in unrelated parent state.
How is this done?
The first relevant function is
plan_commit_changes_for_merge.This function iterates over the selected commits, and figures out which base to use for the merge ops.
At this step, we simplify the merges down to only the necessary ones.
The second (and main function) is
merge_commit_changes_to_tree.This function calls
plan_commit_changes_for_merge, and executes the merge ops in order.Starting with the common merge base of all commits to be merged.
If there is a conflict encountered, we stop and return where we stopped
Why
This enables us to bypass having to sort the subject commits of a squash around the target, and just grabbing the actual changes to produce the tree of the squash commit.
This is part 1 of 3 in a stack made with GitButler: