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

Thanks again, Philip :)

On Tue, Sep 22, 2026 at 9:57 AM Phillip Wood [off-list ref] wrote:

Hi Ben

On 22/09/2026 13:43, D. Ben Knoble wrote:
I think there are wierd cases where one diff algorithm results in
conflicts and another doesn't because they generate different (but
equally valid) diffs so allowing the user to tweak the algorithm we use
via init_ui_merge_options() is probably a good idea.

Gotcha; I've already queued this locally.

quoted

quoted

quoted

+                     o.verbosity = 0;

Looking at the code in merge-ort.c it appears the verbosity option was
used by the recursive strategy but isn't used anymore so I think we
could drop this.

Intriguing. (Assuming the default "2") There's a "< 5" check in
path_msg() that wouldn't be affected by dropping this, and a "> 2"
check in checkout() that… also wouldn't be affected?

The former is not affected because we're cherry-picking so never have an
inner merge from merging multiple merge bases. The latter is not
affected because we don't checkout the result!

That's very helpful; I find it challenging right now to navigate the
various call-graphs here :)

quoted

But it might matter if something is setting the verbosity elsewhere
(config, GIT_MERGE_VERBOSITY), and I think we really want this merge
to be quiet? I seem to remember reading commits in this area quieting
"git reset" and so on to keep the noise down.

So I'm inclined to leave it for now, especially in case it later does get used.

merge ort does not print anything - it just adds messages to an strmap
in struct merge_result() which we ignore here. I guess setting it to
zero might avoid a little work generating the messages.

Possibly! I still think it signals our intent to be quiet better this way, too.

quoted

quoted

quoted

+                     oidcpy(&index_tree, &result.tree->object.oid);
+                     clear_merge_options(&o);

Looking at replay.c:replay_revisions() I think this should be

merge_finalize(&opts, &result);

Hm, possibly. It does look like that does more with the "result,"
which is probably needed.

Oh, we definitely want to free the strmap in the merge result.

 > But it doesn't actually clear the merge options.

Isn't that because there are no allocations in that struct? (obuf is
unused - it looks like we could clean up the struct by removing the
members that were used by merge-recursive but are ignored by merge-ort)

Maybe---I was more worried about un-reusable state, but it's true that
the clear function is a no-op right now, heh. So it was a bit of "in
case one day this is mandatory," perhaps.

quoted

On one hand, I thought it could be important not to reuse that struct
between merges. But if we do use the "ui" init, it might be ok?
replay_revisions() does use the same struct between calls to
merge_incore_nonrecursive().

Oh, but one other thing: we unconditionally reinit the merge options
later on in do_apply_stash(). We could conditionally initialize there
("if (has_index)"), I suppose?

I'd just move the call to init_ui_merge_options() above "if (index)". As
far as I know it should be fine to reuse it - any state is stored in the
result

Yeah, that's smarter. Locally I got tripped by the case where we said
--index but skip some work; but it should be fine to unconditionally
initialize those options earlier.

quoted

Funny, I was getting aborts before removing the asserts because I
hadn't set the labels, aha. Looks like we've come back around to
keeping the labels.

Sorry for that detour

No worries.

quoted

I'll probably keep a similar structure as the
working tree merge uses, I think.

I'd use fixed names and not bother with all the conditionals around the
label text to keep it simple.

That's what I ended up with locally, yeah. I finally decided it was
too complicated to do anything else for labels that would be really
hard to find.

I'll get v2 out in the morning, probably.

-- 
D. Ben Knoble