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

On Sat, Sep 26, 2026 at 5:51 AM Phillip Wood [off-list ref] wrote:

On 25/09/2026 17:24, Junio C Hamano wrote:

quoted

"D. Ben Knoble" [off-list ref] writes:

quoted

On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano [off-list ref] wrote:

quoted

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.

quoted

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

s/text/test ;)

test_write_lines committed B unstaged >expect &&
test_cmp expect file

This does fail on the original code (head, base, merge_base) because
the index (git show :file) has "A B staged" lines instead of
"committed B staged" lines.

This test does pass on the new code, but needs some
arrangement/cleanup for the later "stash -k" test to succeed, so I'll
include that in the next round as well.

-- 
D. Ben Knoble