Skip to content

Detect and reject individual no-effect changes after cherry-pick#13665

Open
mtsgrd wants to merge 1 commit intomasterfrom
surface-no-effective-changes
Open

Detect and reject individual no-effect changes after cherry-pick#13665
mtsgrd wants to merge 1 commit intomasterfrom
surface-no-effective-changes

Conversation

@mtsgrd
Copy link
Copy Markdown
Contributor

@mtsgrd mtsgrd commented May 6, 2026

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

@github-actions github-actions Bot added the rust Pull requests that update Rust code label May 6, 2026
@mtsgrd mtsgrd force-pushed the surface-no-effective-changes branch from 134fd40 to 6231351 Compare May 6, 2026 13:06
@mtsgrd mtsgrd marked this pull request as ready for review May 6, 2026 13:08
Copilot AI review requested due to automatic review settings May 6, 2026 13:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 as NoEffectiveChanges when 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 thread crates/but-core/src/tree/mod.rs Outdated
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
@mtsgrd mtsgrd force-pushed the surface-no-effective-changes branch from 6231351 to 10de214 Compare May 6, 2026 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants