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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2026, 16:24 UTC
Message-ID: <xmqqpky1wb76.fsf@gitster.g>
In-Reply-To: <CALnO6CBhoBcVjLXidvii+o_Ump_k9disW177LeSS0118t3oGKg@mail.gmail.com>

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

Show 22 quoted lines

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

Thanks.