WIP: Handle hunk-based operations for file additions/removals#12607
WIP: Handle hunk-based operations for file additions/removals#12607
Conversation
Allow hunk-based uncommit operations to work on files that were added or removed by treating a missing "before" entry as an empty blob and None state. This lets unified-patch computation proceed for additions, applies hunks against an empty before state, and correctly handles the case where removing all hunks from an added file should delete the file from the tree. Also preserve the correct file mode when the resulting contents match the before or after state and clean up builder upsert/remove logic accordingly.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
mtsgrd
changed the title
|
@Caleb-T-Owens here it is |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the create_tree_without_diff function to support hunk-based operations on file additions and removals. Previously, attempting hunk-based operations on added or deleted files would fail with an error. The PR allows treating missing "before" entries as empty blobs, enabling unified-patch computation and hunk application for file additions. It also correctly handles the edge case where removing all hunks from an added file should delete the file from the tree.
Changes:
- Allow hunk-based uncommit operations on file additions by treating missing before_entry as None state with empty content
- Handle file deletion when all hunks are removed from an added file
- Preserve correct file mode based on whether the resulting contents match before or after state
| id: be.id().detach(), | ||
| kind: be.mode().kind(), | ||
| }), | ||
| blob.data.clone(), |
There was a problem hiding this comment.
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.
| // 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(), | ||
| ) | ||
| } else { | ||
| (None, Vec::new()) | ||
| }; |
There was a problem hiding this comment.
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.
| // 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. |
There was a problem hiding this comment.
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.
Allow hunk-based uncommit operations to work on files that were added or
removed by treating a missing "before" entry as an empty blob and None
state. This lets unified-patch computation proceed for additions,
applies hunks against an empty before state, and correctly handles the
case where removing all hunks from an added file should delete the file
from the tree. Also preserve the correct file mode when the resulting
contents match the before or after state and clean up builder
upsert/remove logic accordingly.