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