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

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

> >>> +                     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!

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

> >>> +                     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)

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

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

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