Re: [PATCH v2 4/4] builtin/stash: merge index in-core

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

"D. Ben Knoble" <ben.knoble@gmail.com> writes:

> @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
>  		    oideq(&c_tree, &info->i_tree)) {
>  			has_index = 0;
>  		} else {
> -			struct strbuf out = STRBUF_INIT;
> +			struct merge_result result = { 0 };
>  
> -			if (diff_tree_binary(&out, &info->w_commit)) {
> -				strbuf_release(&out);
> -				return error(_("could not generate diff %s^!."),
> -					     oid_to_hex(&info->w_commit));
> -			}
> +			o.branch1 = "Upstream index";
> +			o.branch2 = "Stashed index changes";
> +			o.ancestor = "Stash base";
>  
> -			ret = apply_cached(&out);
> -			strbuf_release(&out);
> -			if (ret)

So, we used to take a diff between w_commit^2^ and w_commit^2 and
then give the resulting patch to "apply --cached".  w_commit is the
working tree state, w_commit^1 is the HEAD (i.e. b_tree) when the
stash was created (i.e., "diff HEAD w_commit" is the change in the
working tree), w_commit^2 is the contents of the index
(i.e. i_tree), so we are computing a patch that represents what
"diff --cached HEAD" would have shown when we created the stash.
And the goal is to reflect this change on the current HEAD to
recreate the "staged" changes in the current index.

IOW, we want to three-way merge the change that moves you from 
b_tree to i_tree into c_tree.

> +			o.verbosity = 0;
> +
> +			head = lookup_tree(o.repo, &c_tree);
> +			merge = lookup_tree(o.repo, &info->i_tree);
> +			merge_base = lookup_tree(o.repo, &info->b_tree);
> +
> +			merge_incore_nonrecursive(&o, head, merge, merge_base,
> +						  &result);

We are using the merge machinery to perform a cherry-pick of the
changes to go from b_tree to i_tree into c_tree.  but the three
trees involved in this cherry-pick is named unnecessarily
confusingly.

merge-incore-nonrecursive() takes the common ancestor ("merge_base")
and two sides ("side1" and "side2") in this order.  It takes the
changes to go from common to side1 and computes the result of
updating the remainder (side2) with such a change (or vice versa; a
merge is symmetric).  When cherry-picking, the first tree would be
the b_tree, the second tree would be the i_tree, and the target tree
would be the c_tree.

 - b_tree serves as merge_base
 - i_tree serves as side1
 - c_tree serves as side2

Am I following what the code should be doing correctly?

I am wondering if the order of the tree trees in the
merge_incore_nonrecursive() call is correct.  Shouldn't it be

			merge_incore_nonrecursive(&o,
						  merge_base, merge, head,
						  &result);

(or merge and head swapped) if we want to update head (I would call
it side2) in such a way that the change to go from it to the
resulting tree is similar to the change between merge_base (b_tree)
and merge (i_tree)?

Ahh, or perhaps the trees are indeed given in a wrong order, but not
in a random wrong order.  merge_ort_nonrecursive(), which is *not*
the function you are using, takes head, merge, and merge_base in
this order, and that order matches what you wrote.

Perhaps the true culprit in this confusion is that the order in
which merge_ort_nonrecursive() takes its three trees (head, merge,
and common) and the order in which merge_incore_nonrecursive() takes
its trees (merge_base, side1, and side2) are different, and if we
fix them to match, it would make it easier to work with?

The new test in the attached patch will fail with this step but if
we revert the changes to builtin/stash.c in this step, it passes.

 t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh
index 3958ab3c8d..0a87e62b11 100755
--- c/t/t3903-stash.sh
+++ w/t/t3903-stash.sh
@@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' '
 	test_cmp expect actual
 '
 
+
+test_expect_success 'stash apply --index does not revert unrelated upstream index changes' '
+	test_when_finished "rm -fr playpen" &&
+	mkdir playpen &&
+	(
+		cd playpen &&
+		git init &&
+		echo "base1" >file1 &&
+		echo "base2" >file2 &&
+		git add file1 file2 &&
+		git commit -m "initial base" &&
+
+		# Make a staged change to file1 and stash it
+		echo "staged1" >file1 &&
+		git add file1 &&
+		git stash &&
+
+		# Upstream advances by modifying unrelated file2
+		echo "upstream2" >file2 &&
+		git add file2 &&
+		git commit -m "upstream change to file2" &&
+
+		# Apply the stash with --index
+		git stash apply --index &&
+
+		# Verify working tree and index state
+		test "$(git show :file1)" = "staged1" &&
+		test "$(git show :file2)" = "upstream2" &&
+		test "$(git show HEAD:file2)" = "upstream2"
+	)
+'
+
 test_expect_success 'stash apply --index leaves everything untouched on failure' '
 	git reset --hard &&
 	echo test >other-file &&