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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 25, 2026, 16:04 UTC
Message-ID: <6e6420e8-3cbd-4975-a781-645e1ffbc1d2@gmail.com>
In-Reply-To: <xmqqse2yz4y4.fsf@gitster.g>

Hi Junio

On 24/09/2026 22:59, Junio C Hamano wrote:

Show 6 quoted lines

> "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.

Show 5 quoted lines

> 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

Show 49 quoted lines

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