Skip to content

Fix restore --staged for certain vfs ops #804

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

Closed
wants to merge 2 commits into from
+40 −5
Closed

Fix restore --staged for certain vfs ops #804

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dc72f0
Fix `restore --staged` for certain vfs ops
tyrielv Sep 9, 2025
e9af392
Fix failing test t7113 due to expected change
tyrielv Oct 9, 2025
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
7 changes: 7 additions & 0 deletions builtin/checkout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,13 @@ static int checkout_paths(const struct checkout_opts *opts,
checkout_index = opts->checkout_index;

if (checkout_index) {
/* Some scenarios that checkout the index may update skipworktree bits,
* such as `restore --staged` after `cherry-pick -n` or `reset --soft`,
* so this flag should be set to ensure the correct virtual filesystem
* event is sent.
*/
the_repository->index->updated_skipworktree = 1;

if (write_locked_index(the_repository->index, &lock_file, COMMIT_LOCK))
die(_("unable to write new index file"));
} else {
Expand Down
3 changes: 2 additions & 1 deletion read-cache-ll.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,8 @@ struct index_state {
drop_cache_tree : 1,
updated_workdir : 1,
updated_skipworktree : 1,
fsmonitor_has_run_once : 1;
fsmonitor_has_run_once : 1,
clear_skip_worktree_for_added_entries : 1;
enum sparse_index_mode sparse_index;
struct hashmap name_hash;
struct hashmap dir_hash;
Expand Down
11 changes: 10 additions & 1 deletion sequencer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -787,13 +787,22 @@ static int do_recursive_merge(struct repository *r,
* to be replace with the tree the index matched before we
* started doing any picks.
*/
if (opts->no_commit && core_virtualfilesystem) {
/* When using the virtual file system, staged new files
* should clear SKIP_WORKTREE during this step to ensure the new files
* are properly added to the working tree as well as index - otherwise
* sparse-checkout functionality will prevent them from being added.
*/
o.repo->index->clear_skip_worktree_for_added_entries = 1;
}
merge_switch_to_result(&o, head_tree, &result, 1, show_output);
o.repo->index->clear_skip_worktree_for_added_entries = 0;

clean = result.clean;
if (clean < 0) {
rollback_lock_file(&index_lock);
return clean;
}

if (write_locked_index(r->index, &index_lock,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
/*
Expand Down
3 changes: 0 additions & 3 deletions t/t7113-post-index-change-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,6 @@ test_expect_success 'test status, add, commit, others trigger hook without flags
git commit -m "second" &&
test_path_is_file testsuccess && rm -f testsuccess &&
test_path_is_missing testfailure &&
git checkout -- dir1/file1.txt &&
test_path_is_file testsuccess && rm -f testsuccess &&
test_path_is_missing testfailure &&
Comment on lines -82 to -84
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to remove these lines. Would the test be failing with the changes? That would be more indicative of a bug introduced by the changes, I think; The post-index-change hook is actively used in Office, and they use Scalar instead of VFSforGit, therefore virtual filesystem-specific changes should not affect them, I'd think.

Copy link
Author

Choose a reason for hiding this comment

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

post-index-change is still called by git checkout, but due to this change it is setting one of the arguments with less discrimination, and this test case is for scenarios where neither of the arguments are set.

I will look into setting the argument more precisely so this test case will pass without change.

git update-index &&
test_path_is_missing testsuccess &&
test_path_is_missing testfailure &&
Expand Down
21 changes: 21 additions & 0 deletions unpack-trees.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1852,6 +1852,25 @@ static void mark_new_skip_worktree(struct pattern_list *pl,
enable_fscache(istate->cache_nr);
clear_ce_flags(istate, select_flag, skip_wt_flag, pl, show_progress);
disable_fscache();

/*
* 3. If clear_skip_worktree_for_added_entries is set and we are checking
* for added entries, clear skip_wt_flag from all added entries. This is
* used when running with virtualfilesystem to ensure that added entries are
* also checked out in the working tree - otherwise skip_wt_flag will
* prevent that.
*/
if ((select_flag & CE_ADDED)
&& istate->clear_skip_worktree_for_added_entries) {
for (i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce = istate->cache[i];
if ((ce->ce_flags & (CE_ADDED | skip_wt_flag))
== (CE_ADDED | skip_wt_flag)) {
ce->ce_flags &= ~skip_wt_flag;
istate->updated_skipworktree = 1;
}
}
}
Comment on lines +1855 to +1873
Copy link
Member

Choose a reason for hiding this comment

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

So this new loop runs after the previous two loops over the entire index (even clear_ce_flags() contains such a loop).

However, the virtual filesystem is already special-cased in clear_ce_flags(), via the call to `clear_ce_flags_virtualfilesystem.

Which makes me wonder whether it's that clear_ce_flags_virtualfilesystem() function that introduces that bug, and this here patch would only paper over that bug?

Copy link
Author

Choose a reason for hiding this comment

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

I'll look into that.

}

static void populate_from_existing_patterns(struct unpack_trees_options *o,
Expand Down Expand Up @@ -2004,6 +2023,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
is_sparse_index_allowed(&o->internal.result, 0))
o->internal.result.sparse_index = 1;

o->internal.result.clear_skip_worktree_for_added_entries =
o->src_index->clear_skip_worktree_for_added_entries;
/*
* Sparse checkout loop #1: set NEW_SKIP_WORKTREE on existing entries
*/
Expand Down
Loading