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

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

On 25/09/2026 17:24, Junio C Hamano wrote:
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
> 
>> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> 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?
>>
>> Indeed, the confusion is that simple ;) Shamefully, we don't have
>> enough test coverage to catch that regression, so I'm very glad indeed
>> you spotted it.
>>
>>> 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.
>>
>> Any objection to me adding this test as a preparatory patch? There's
>> no sign-off, so I don't want to mess up the DCO here.
> 
> It was written merely as an illustration and is not something I am
> proud of.  For example, creating a totally new playpen repository
> only for a single piece of test and remove the entire thing when the
> single test piece is done was done only to make sure the existing
> test that come later can never be affected.  Also the test only uses
> the most trivial case (a file is added in the stashed change, nobody
> else involved in the stash application has touched the file so there
> is nothing to "merge" in the file).  It was enough to demonstrate
> that the order of arguments given to the function was wrong, but
> we wouldn't catch problems in content-level merge with such a test.
> 
> So, I wouldn't mind if you reused that as one in a series of tests,
> but I'd prefer to see those who are move invested in the topic to
> come up with a bit more realistic scenario.

Maybe something like the test below (which I admit I haven't actually 
tested). That checks we merge the file contents and puts the changes in 
the file close enough together so that the old code would fail and has 
different contents for the three merged blobs.

test_write_lines A B C >file &&
git commit -m xxx file &&
test_write_lines A B staged >file &&
git add file &&
test_write_lines A B unstaged >file &&
git stash &&
test_write_lines committed B C >file &&
git commit -m yyy file &&
git stash pop --index &&
git show :file >actual &&
test_write_lines committed B staged >expect &&
text_cmp expect actual &&
test_write_lines committed B unstaged >expect &&
test_cmp expect file

Thanks

Phillip