Skip to content

WIP: Handle hunk-based operations for file additions/removals #12607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
mtsgrd wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

WIP: Handle hunk-based operations for file additions/removals #12607

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 37 additions & 21 deletions crates/but-workspace/src/tree_manipulation/create_tree_without_diff.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,21 @@ pub fn create_tree_without_diff(
if change.hunk_headers.is_empty() {
revert_file_to_before_state(&before_entry, &mut builder, &change)?;
} else {
let Some(before_entry) = before_entry else {
anyhow::bail!(
"Deletions or additions aren't well-defined for hunk-based operations - use the whole-file mode instead"
);
};
// For file additions (no before_entry), use None/empty as
// the "before" state so the hunk subtraction still works.
let (before_state, before_data): (Option<ChangeState>, Vec<u8>) =
if let Some(ref be) = before_entry {
let blob = be.object()?.into_blob();
(
Some(ChangeState {
id: be.id().detach(),
kind: be.mode().kind(),
}),
blob.data.clone(),
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The call to blob.data.clone() at line 144 could be avoided by using blob.data directly or by moving the blob instead of cloning. Since blob is consumed right after this, cloning adds unnecessary memory allocation. Consider restructuring to avoid the clone.

Copilot uses AI. Check for mistakes.
)
} else {
(None, Vec::new())
};
Comment on lines +134 to +148
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The new feature allowing hunk-based operations on file additions lacks test coverage. While whole-file uncommit operations have tests in tests/workspace/commit/uncommit_changes.rs, there are no tests that verify hunk-based uncommit on added files, or the specific case where all hunks are removed from an added file (which should delete the file). Consider adding tests for these scenarios.

Copilot uses AI. Check for mistakes.

let diff = but_core::UnifiedPatch::compute(
repository,
Expand All @@ -145,10 +155,7 @@ pub fn create_tree_without_diff(
id: after_entry.id().detach(),
kind: after_entry.mode().kind(),
},
ChangeState {
id: before_entry.id().detach(),
kind: before_entry.mode().kind(),
},
before_state,
context_lines,
)?
.context(
Expand Down Expand Up @@ -185,28 +192,37 @@ pub fn create_tree_without_diff(
}

// TODO: Validate that the hunks correspond with actual changes?
let before_blob = before_entry.object()?.into_blob();

let new_hunks = new_hunks_after_removals(
diff_hunks.into_iter().map(Into::into).collect(),
good_hunk_headers,
)?;
let new_after_contents = but_core::apply_hunks(
before_blob.data.as_bstr(),
before_data.as_bstr(),
after_blob.data.as_bstr(),
&new_hunks,
)?;
let mode = if new_after_contents == before_blob.data {
before_entry.mode().kind()

if before_entry.is_none() && new_after_contents == before_data {
// All changes removed from a file addition - remove
// the file from the tree entirely.
builder.remove(change.path.as_bstr())?;
} else {
after_entry.mode().kind()
};
let new_after_contents = repository.write_blob(&new_after_contents)?;
let mode = if new_after_contents == before_data {
before_entry
.as_ref()
.expect("before_entry.is_none() case handled above")
.mode()
.kind()
} else {
after_entry.mode().kind()
};
let new_after_contents = repository.write_blob(&new_after_contents)?;

// Keep the mode of the after state. We _should_ at some
// point introduce the mode specifically as part of the
// DiscardSpec, but for now, we can just use the after state.
builder.upsert(change.path.as_bstr(), mode, new_after_contents)?;
// Keep the mode of the after state. We _should_ at some
// point introduce the mode specifically as part of the
// DiscardSpec, but for now, we can just use the after state.
Comment on lines +221 to +223
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The comment "Keep the mode of the after state" is now misleading. The code actually uses the before state's mode when the contents match the before state (line 210-215), and only uses the after state's mode when the contents differ. The comment should be updated to reflect this conditional logic.

Copilot uses AI. Check for mistakes.
builder.upsert(change.path.as_bstr(), mode, new_after_contents)?;
}
}
}
_ => {
Expand Down
Loading