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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Junio

On 24/09/2026 22:59, Junio C Hamano wrote:
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
> 
> 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.

Ouch that's nasty. Well spotted, I missed it when I read the code 
(because the arguments were in the same order as the call to 
merge_ort_nonrecursive()) and the tests we have use the same version of 
the file for "base" and "stage2" so do not notice if they'd been 
transposed. It is rather confusing that two functions that are so 
closely related take their arguments in a different order.

> 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?

I think it is definitely worth fixing them to take the trees in the same 
order. My preference would be "base", "stage1", "stage2" but so long as 
they match each other I dont object to "stage1", "stage2", "base".

Thanks

Phillip

> 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 &&