tree: remove entries before upserting#14312
Conversation
When applying worktree changes, some tree entries need to be deleted, but performing those deletions in the same pass as adding new entries may cause newly added entries to be deleted. This was observed when an entry of the form "A/one" was added, and "A" subsequently deleted (thus deleting the newly added "A/one"). Fix this by making two separate passes - first deletion, then addition. In order to prevent issues like this from happening in the future, ideally gix would have an API to delete a leaf entry (returning an error if the entry to be deleted was a non-leaf - in the example above, deleting "A" when "A/one" is present would be an error), but looking at its documentation, I don't think such an API exists. I did not reproduce the comment "NOTE: See copy below!". It was added in 0aa9e0f (allow DiffSpecs with hunks even when untracked., 2025-02-26) with a paired "NOTE: See copy above!", but the latter was removed in e8edb62 (Support for hunk-selections when committing and amending, 2025-03-30), so the comment no longer seems necessary.
There was a problem hiding this comment.
Pull request overview
This PR fixes a data-loss edge case in but-core’s tree construction when applying worktree changes that include renames plus newly created paths under the rename’s old location. It does so by separating “remove old paths” from “upsert new paths”, and adds a regression test to prevent reintroduction.
Changes:
- Perform a first pass that removes
previous_pathentries before applying/upserting the rest of the changes. - Add a regression test that reproduces the “rename + new directory at old path” scenario.
- Register the new
treetest module in thebut-corecore test suite.
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 | Moves previous_path removals into a pre-pass before upserts to avoid pruning newly created subtrees. |
| crates/but-core/tests/core/tree.rs | Adds a regression test covering rename + new directory at old path. |
| crates/but-core/tests/core/main.rs | Wires the new tree test module into the test harness. |
| let work_dir = repo.workdir().expect("non-bare repo"); | ||
| for possible_change in changes.iter() { | ||
| let change_request = match possible_change { | ||
| Ok(change) => change, | ||
| Err(_) => continue, | ||
| }; | ||
| if let Some(previous_path) = change_request.previous_path.as_ref().map(|p| p.as_bstr()) { | ||
| base_tree_editor.remove(previous_path)?; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a valid suggestion, but I think that in the first place, if the destination does not exist, we should abort the whole operation anyway instead of trying to recover.
Data loss could happen anyway if the user renames A to B and adds A/one, but somehow B is missing from the filesystem - in this case, even if we protect A from being deleted, it will be still gone when the code upserts into A/one.
There was a problem hiding this comment.
To follow up on this, both Copilot and I were wrong that this part is for preventing data loss. It's actually to handle file deletions - for a deletion, the previous_path is None and the path is the file to be deleted; we know that it's a deletion because we check the filesystem and it's missing. I tried adding an assertion that previous_path is None if the change request is a deletion, and all tests pass (it's probably not worth having it in mainline so I didn't send a PR).
|
Thanks so much @jonathantanmy2!
I will see what I can do and reach out for feedback once there is something to look at. Maybe switching in the alternative, i.e. |
Motivation comes from gitbutlerapp/gitbutler#14312, where callers had to split tree-entry deletions from additions to avoid accidentally deleting a newly-added subtree after adding A/one and then deleting A. Add Editor::remove_leaf() to gix-object and expose it through gix object tree editors and cursors. The new API keeps remove() behavior unchanged, remains tolerant of absent paths, but returns an error when the target entry exists and is a tree. Co-authored-by: Sebastian Thiel <[email protected]>
Motivation comes from gitbutlerapp/gitbutler#14312, where callers had to split tree-entry deletions from additions to avoid accidentally deleting a newly-added subtree after adding A/one and then deleting A. Add Editor::remove_leaf() to gix-object and expose it through gix object tree editors and cursors. The new API keeps remove() behavior unchanged, remains tolerant of absent paths, but returns an error when the target entry exists and is a tree. Co-authored-by: Sebastian Thiel <[email protected]>
This should prevent issues as in gitbutlerapp#14312 as we expect to only ever remove leafs, and not directories. It's notable that in trees we don't expect empty directories as these are not tracked, which is something that could happen in Git indices only (sparse dirs). I am saying this because `remove_leaf` refuses to remove any directory, they are never empty.
This should prevent issues as in gitbutlerapp#14312 as we expect to only ever remove leafs, and not directories. It's notable that in trees we don't expect empty directories as these are not tracked, which is something that could happen in Git indices only (sparse dirs). I am saying this because `remove_leaf` refuses to remove any directory, they are never empty.
When applying worktree changes, some tree entries need to be deleted, but performing those deletions in the same pass as adding new entries may cause newly added entries to be deleted. This was observed when an entry of the form "A/one" was added, and "A" subsequently deleted (thus deleting the newly added "A/one"). Fix this by making two separate passes - first deletion, then addition.
In order to prevent issues like this from happening in the future, ideally gix would have an API to delete a leaf entry (returning an error if the entry to be deleted was a non-leaf - in the example above, deleting "A" when "A/one" is present would be an error), but looking at its documentation, I don't think such an API exists.
I did not reproduce the comment "NOTE: See copy below!". It was added in 0aa9e0f (allow DiffSpecs with hunks even when untracked., 2025-02-26) with a paired "NOTE: See copy above!", but the latter was removed in e8edb62 (Support for hunk-selections when committing and amending, 2025-03-30), so the comment no longer seems necessary.