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

Denton Liu [off-list ref] writes:

Hi Junio,

On Tue, Feb 16, 2021 at 12:22:52PM -0800, Junio C Hamano wrote:

quoted

 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.