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

>> -               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?
> 

> 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?
> 

>> +               }
>> +
> 
> [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.
>