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

Denton Liu [off-list ref] writes:

+static void diff_include_untracked(const struct stash_info *info, struct diff_options *diff_opt)
+{
+	const struct object_id *oid[] = { &info->w_commit, &info->u_tree };
+	struct tree *tree[ARRAY_SIZE(oid)];
+	struct tree_desc tree_desc[ARRAY_SIZE(oid)];
+	struct unpack_trees_options unpack_tree_opt = { 0 };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(oid); i++) {
+		tree[i] = parse_tree_indirect(oid[i]);
+		if (parse_tree(tree[i]) < 0)
+			die(_("failed to parse tree"));
+		init_tree_desc(&tree_desc[i], tree[i]->buffer, tree[i]->size);
+	}
+
+	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;

OK, it looks like this was borrowed from read_tree implementation
for reading two trees into the index, sort-of, but is a bit funny.

The setting of .fn to twoway_merge is misleading.  The .fn callback
is to be used when .merge is set (otherwise nothing should call it
inside unpack-trees.c), but nobody seems to set opt.merge to true.

+	if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
+		die(_("failed to unpack trees"));
+
+	do_diff_cache(&info->b_commit, diff_opt);
+}

Nice to see that it was just a simple matter of programming ;-)



 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;
 
 	if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
 		die(_("failed to unpack trees"));

Hi Junio,

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

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.

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

-Denton

 	if (unpack_trees(ARRAY_SIZE(tree_desc), tree_desc, &unpack_tree_opt))
 		die(_("failed to unpack trees"));