Skip to content

Commit 134fd40

Browse files
committed
Detect and reject individual no-effect changes after cherry-pick
When committing a mix of changes to a branch, some changes may have no effect on the target tree — for example, deleting a file that only exists in another stack's workspace tree but not in this branch. Previously, these no-op changes were silently dropped: the commit succeeded without the change, and the caller received no rejection feedback. The root cause was that the post-cherry-pick check only compared the entire result tree against the target tree. If at least one change was effective, the aggregate trees would differ and all changes appeared to succeed — even those that individually had no effect. Now, after the cherry-pick, each change is checked individually by comparing its path entry between the target tree and the result tree. Changes where the entry is identical (or both absent) are rejected as NoEffectiveChanges, giving the caller per-change feedback. A regression test with a two-stack fixture verifies that committing a file deletion from another stack alongside a valid modification correctly rejects the deletion while still producing a commit for the modification.
1 parent 08d2fee commit 134fd40

3 files changed

Lines changed: 100 additions & 0 deletions

File tree

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,35 @@ pub fn create_tree(
153153
}
154154
tree_with_changes = merge_result.tree.write()?.detach();
155155
}
156+
// After the cherry-pick, some changes may have had no effect on the target tree
157+
// (e.g. deleting a file that only exists in another stack's tree, not in this
158+
// branch). Check each change individually by comparing its path entry between
159+
// the target tree and the result tree — if they match, the change was a no-op.
160+
if needs_cherry_pick {
161+
let target = target_tree.attach(repo).object()?.peel_to_tree()?;
162+
let result = tree_with_changes.attach(repo).object()?.peel_to_tree()?;
163+
for possible_change in changes.iter_mut() {
164+
let spec = match possible_change {
165+
Ok(spec) => spec,
166+
Err(_) => continue,
167+
};
168+
let target_entry = target.lookup_entry(spec.path.as_bstr().split_str("/"))?;
169+
let result_entry = result.lookup_entry(spec.path.as_bstr().split_str("/"))?;
170+
if target_entry.map(|e| e.object_id()) == result_entry.map(|e| e.object_id()) {
171+
into_err_spec(possible_change, RejectionReason::NoEffectiveChanges);
172+
}
173+
}
174+
}
175+
// If every change was individually rejected, there is nothing to commit —
176+
// unless some were cherry-pick conflicts, in which case we still create a
177+
// commit so the user can see and resolve the conflicts.
178+
if changes.iter().all(|c| c.is_err())
179+
&& !changes
180+
.iter()
181+
.any(|c| matches!(c, Err((RejectionReason::CherryPickMergeConflict, _))))
182+
{
183+
break 'retry (None, None);
184+
}
156185
break 'retry (
157186
Some(tree_with_changes),
158187
Some(tree_with_changes_without_cherry_pick),
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
#!/usr/bin/env bash
2+
3+
set -eu -o pipefail
4+
5+
source "${BASH_SOURCE[0]%/*}/shared.sh"
6+
7+
### General Description
8+
9+
# A workspace commit with two stacks.
10+
# Stack A modifies `file` (prepends lines).
11+
# Stack B creates `new-file`.
12+
git init
13+
14+
seq 50 60 >file && git add . && git commit -m "base" && git tag base
15+
setup_target_to_match_main
16+
17+
git branch B
18+
19+
git checkout -b A
20+
{ seq 10; seq 50 60; } >file && git add . && git commit -m "A: 10 lines on top of file"
21+
22+
git checkout B
23+
seq 10 >new-file && git add . && git commit -m "B: new file with 10 lines"
24+
25+
create_workspace_commit_once A B

crates/but-workspace/tests/workspace/commit_engine/new_commit.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1236,6 +1236,52 @@ fn validate_no_change_on_noop() -> anyhow::Result<()> {
12361236
Ok(())
12371237
}
12381238

1239+
/// When committing a deletion of a file that only exists in another stack's tree
1240+
/// alongside a valid change, the deletion should be rejected as `NoEffectiveChanges`
1241+
/// while the valid change still produces a commit.
1242+
#[test]
1243+
fn deletion_of_file_from_other_stack_is_rejected_per_change() -> anyhow::Result<()> {
1244+
let (repo, _tmp) = writable_scenario("two-stacks-with-extra-file");
1245+
1246+
// Delete `new-file` which only exists in stack B's tree, not in A.
1247+
let new_file_path = repo.workdir_path("new-file").expect("non-bare");
1248+
std::fs::remove_file(&new_file_path)?;
1249+
1250+
// Also modify `file` which does exist in stack A's tree.
1251+
write_sequence(&repo, "file", [(1, 5), (50, 60)])?;
1252+
1253+
let parent_commit = repo.rev_parse_single("A")?;
1254+
let outcome = commit_engine::create_commit(
1255+
&repo,
1256+
Destination::NewCommit {
1257+
parent_commit_id: Some(parent_commit.into()),
1258+
message: "modify file and delete new-file on stack A".into(),
1259+
stack_segment: None,
1260+
},
1261+
vec![diff_spec(None, "file", []), diff_spec(None, "new-file", [])],
1262+
CONTEXT_LINES,
1263+
)?;
1264+
1265+
assert!(
1266+
outcome.new_commit.is_some(),
1267+
"a commit is created because the file modification is valid"
1268+
);
1269+
insta::assert_debug_snapshot!(outcome.rejected_specs, @r#"
1270+
[
1271+
(
1272+
NoEffectiveChanges,
1273+
DiffSpec {
1274+
previous_path: None,
1275+
path: "new-file",
1276+
hunk_headers: [],
1277+
},
1278+
),
1279+
]
1280+
"#);
1281+
1282+
Ok(())
1283+
}
1284+
12391285
const UI_CONTEXT_LINES: u32 = 3;
12401286

12411287
mod utils {

0 commit comments

Comments
 (0)