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

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