Re: [PATCH v2 1/7] reset: behave correctly with sparse-checkout

Elijah Newren wrote:

quoted

-               int is_missing = !(one->mode && !is_null_oid(&one->oid));
+               struct diff_filespec *two = q->queue[i]->two;
+               int is_in_reset_tree = one->mode && !is_null_oid(&one->oid);

Isn't !is_null_oid(&one->oid) redundant to checking one->mode?  When
does the diff machinery ever give you a non-zero mode with a null oid?

It looks like this originally only checked the mode, and the extra OID check
was introduced in ff00b682f2 (reset [<commit>] paths...: do not mishandle
unmerged paths, 2011-07-13). I was able to remove `!is_null_oid(&one->oid)`
from the condition and run the `t71*` tests without any failures, but I'm
hesitant to remove it on the off chance that this handles a case I'm not
thinking of.

Also, is_in_reset_tree == !is_missing; I'll note that below.

quoted

                struct cache_entry *ce;

+               /*
+                * If the file being reset has `skip-worktree` enabled, we need
+                * to check it out to prevent the file from being hard reset.

I don't understand this comment.  If the file wasn't originally in the
index (is_missing), and is being added to it, and is correctly marked
as skip_worktree, and the file isn't in the working tree, then it
sounds like everything is already in a good state.  Files outside the
sparse checkout are meant to have the skip_worktree bit set and be
missing from the working tree.

Also, I don't know what you mean by 'hard reset' here.

quoted

+                */
+               pos = cache_name_pos(two->path, strlen(two->path));
+               if (pos >= 0 && ce_skip_worktree(active_cache[pos])) {
+                       struct checkout state = CHECKOUT_INIT;
+                       state.force = 1;
+                       state.refresh_cache = 1;
+                       state.istate = &the_index;
+
+                       checkout_entry(active_cache[pos], &state, NULL, NULL);

Does this introduce an error in the opposite direction from the one
stated in the commit message?  Namely we have two things that should
be in sync: the skip_worktree flag stating whether the file should be
present in the working directory (skip_worktree), and the question of
whether the file is actually in the working directory.  In the commit
message, you pointed out a case where the y were out of sync one way:
the skip_worktree flag was not set but the file was missing.  Here you
say the skip_worktree flag is set, but you add it to the working tree
anyway.

Or am I misunderstanding the code?

Most of this is addressed in [1], and you're right that what's in this 
patch isn't the right fix for the problem. This patch tried to solve the
issue of "skip-worktree is being ignored and reset files are showing up 
deleted" by continuing to ignore `skip-worktree`, but now checking out the
`skip-worktree` files based on their pre-reset state in the index (unless
they, for some reason, were already present in the worktree). However, that
completely disregards the reasoning for having `skip-worktree` in the first
place (the user wants the file *ignored* in the worktree) and violates the
premise of `git reset --mixed` not modifying the worktree, so the better
solution is to set `skip-worktree` in the resulting index entry and not
check out anything.

[1] https://lore.kernel.org/git/9b99e856-24cc-03fd-7871-de92dc6e39b6@github.com/ (local)

quoted

+               }
+

[I did some slight editing to the diff to make the next two parts
appear next to each other]

quoted

-               if (is_missing && !intent_to_add) {
+               if (!is_in_reset_tree && !intent_to_add) {

I thought this was some subtle bugfix or something, and spent a while
trying to figure it out, before realizing that is_in_reset_tree was
simply defined as !is_missing (for some reason I was assuming it was
dealing with two->mode while is_missing was looking at one->mode).  So
this is a simple variable renaming, which I think is probably good,
but I'd prefer if this was separated into a different patch to make it
easier to review.

Good call, I'll include this in V3.