Detect and reject individual no-effect changes after cherry-pick#13665
Open
Detect and reject individual no-effect changes after cherry-pick#13665
Conversation
134fd40 to
6231351
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves commit creation when applying worktree changes onto a different target tree (cherry-pick/merge-trees step), by detecting and rejecting individual change requests that become no-ops after the cherry-pick—rather than only rejecting when the overall resulting tree is unchanged.
Changes:
- Add post–cherry-pick per-path checks to flag individual
DiffSpecs asNoEffectiveChangeswhen the target and result tree entries match. - Add a new fixture with two stacks where one stack introduces an extra file.
- Add a regression test ensuring a deletion of a file that only exists in another stack is rejected while other valid changes still create a commit.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/but-core/src/tree/mod.rs | Adds per-change post–cherry-pick no-op detection and an early-exit when all changes are rejected (without cherry-pick conflicts). |
| crates/but-workspace/tests/workspace/commit_engine/new_commit.rs | Adds regression test for per-change rejection of a no-op deletion after cherry-pick. |
| crates/but-workspace/tests/fixtures/scenario/two-stacks-with-extra-file.sh | Adds fixture scenario with two stacks where only one stack contains an extra file. |
Comment on lines
+168
to
+172
| let target_entry = target.lookup_entry(spec.path.as_bstr().split_str("/"))?; | ||
| let result_entry = result.lookup_entry(spec.path.as_bstr().split_str("/"))?; | ||
| if target_entry.map(|e| e.object_id()) == result_entry.map(|e| e.object_id()) { | ||
| into_err_spec(possible_change, RejectionReason::NoEffectiveChanges); | ||
| } |
Problem:
When committing a mix of changes to a branch, some changes could be
silently dropped — the commit succeeded, but the caller received no
rejection feedback for the omitted changes.
Example: deleting a file that only exists in another stack's workspace
tree. The deletion has no effect on this branch's tree, but was not
reported as rejected.
Root cause:
The post-cherry-pick check only compared entire trees:
if tree_with_changes == target_tree { reject all }
When at least one change was effective the trees differed, so
individual no-ops slipped through undetected.
Fix:
After the cherry-pick, check each change individually via an
`entries_match` helper that compares both mode and object_id at a
given path between the target and result trees:
for each change:
if entries_match(target, result, change.path)
&& entries_match(target, result, change.previous_path):
reject as NoEffectiveChanges
Comparing mode (not just object_id) catches mode-only changes like
executable bit toggles. Checking previous_path catches renames where
only the source removal was effective.
Test:
A new `two-stacks-with-extra-file` fixture verifies that committing a
file deletion from another stack alongside a valid modification:
- rejects the deletion as `NoEffectiveChanges`
- still produces a commit for the modification
6231351 to
10de214
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When committing a mix of changes to a branch, some changes could be silently dropped — the commit succeeded, but the caller received no rejection feedback for the omitted changes.
Example: deleting a file that only exists in another stack's workspace tree. The deletion has no effect on this branch's tree, but was not reported as rejected.
Root cause
The post-cherry-pick check only compared entire trees:
When at least one change was effective the trees differed, so individual no-ops slipped through undetected.
Fix
After the cherry-pick, check each change individually via an
entries_matchhelper that compares both mode and object_id at a given path between the target and result trees:Comparing mode (not just object_id) catches mode-only changes like executable bit toggles. Checking
previous_pathcatches renames where only the source removal was effective.Test
A new
two-stacks-with-extra-filefixture verifies that committing a file deletion from another stack alongside a valid modification:NoEffectiveChanges