Re: [PATCH v3 1/2] stash show: teach --include-untracked and --only-untracked

[Junio C Hamano <gitster@pobox.com>]

Denton Liu <liu.denton@gmail.com> writes:

> Hi Junio,
>
> On Tue, Feb 16, 2021 at 12:22:52PM -0800, Junio C Hamano wrote:
>>  builtin/stash.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git c/builtin/stash.c w/builtin/stash.c
>> index c788a3e236..7e0204bd8a 100644
>> --- c/builtin/stash.c
>> +++ w/builtin/stash.c
>> @@ -807,10 +807,11 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op
>>  		init_tree_desc(&tree_desc[i], tree[i]->buffer, tree[i]->size);
>>  	}
>>  
>> +	/* mimic "git read-tree W U" without "-m" */
>>  	unpack_tree_opt.head_idx = -1;
>>  	unpack_tree_opt.src_index = &the_index;
>>  	unpack_tree_opt.dst_index = &the_index;
>> -	unpack_tree_opt.fn = twoway_merge;
>> +	unpack_tree_opt.fn = NULL;
>
> Perhaps it would be even more clear if we just removed this line
> entirely, otherwise it may give future readers a false impression that
> .fn is significant in any way.

Assignment of something concrete like "twoway_merge" to .fn when it
is a no-op was misleading, but between NULL or uninitialized, both
state clear that it is not used, so I am OK with either.

> Aside from that, both of your SQUASH??? commits look good to me. Thanks
> for tying up the loose ends.

We may want to write a custom unpack_trees() callback for this, and
use it here to catch "this third tree claims to be untracked, but
why does it have an entry that overlaps and/or D/F-conflicts with
the entry from the working tree side?" that you talked about in the
log message, but I think it is better to leave it for future [*], as
the feature should already be usable in its current shape.

Thanks.


[Footnote]

 * Yes, I didn't say "we can leave it", but said "it is better to
   leave it"; as long as we do not declare victory too early and end
   up shipping a half-baked unusable mess, I think it is better to
   wrap a topic early and plan to make it nicer in a future
   follow-up.

   To encourage such future refinements, however, you may add a
   NEEDSWORK comment in the code as a reminder for our future
   selves.