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

[Victoria Dye <vdye@github.com>]

Elijah Newren wrote:
>> -               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.
> 
>>                 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.
> 
>> +                */
>> +               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/

>> +               }
>> +
> 
> [I did some slight editing to the diff to make the next two parts
> appear next to each other]
> 
>> -               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.