From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Tue, 22 Sep 2026 20:34:07 GMT
Subject: Re: [PATCH 2/2] builtin/stash: merge index in-core
Message-ID: <CALnO6CDxew2b0X+HMiT0Vai_hj+MaueV9Ht2BOB5zrsZ27QUwg@mail.gmail.com>
In-Reply-To: <41d28f9d-b86a-4d65-9a85-656ea9d216e9@gmail.com>

Thanks again, Philip :)

On Tue, Sep 22, 2026 at 9:57 AM Phillip Wood <phillip.wood123@gmail.com> 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.

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

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

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

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

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