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

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Sep 22, 2026, 12:43 UTC
Message-ID: <CALnO6CDG4Emny7xESxN8GObaXb_P9gPHBZ857hrAvDjiMSqsKQ@mail.gmail.com>
In-Reply-To: <2551b801-4cb3-4880-ac01-7d14a188ddd4@gmail.com>

On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 13 quoted lines

>
> Hi Ben
>
> On 19/09/2026 22:26, D. Ben Knoble wrote:
> > Fortunately, we can achieve 2 goals at once: avoid round-tripping to the
> > file-system (and invoking expensive subprocesses) by performing the
> > merge in-core. Since the results are never seen, we don't need to set
> > the usual branch and ancestor labels.
>
> When the merge succeeds without conflicts we use the result so it is
> seen. It would be clearer to say that "If there are conflicts we discard
> the result so ...". The rest of the commit message explains the problem
> nicely.

Indeed. This is what I get for (unusually) dashing off the commit message up against the clock. Thanks!

Show 17 quoted lines

> > @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
> >                   oideq(&c_tree, &info->i_tree)) {
> >                       has_index = 0;
> >               } else {
> > -                     struct strbuf out = STRBUF_INIT;
> > +                     struct merge_result result = { 0 };
> >
> > -                     if (diff_tree_binary(&out, &info->w_commit)) {
> > -                             strbuf_release(&out);
> > -                             return error(_("could not generate diff %s^!."),
> > -                                          oid_to_hex(&info->w_commit));
> > -                     }
> > +                     init_basic_merge_options(&o, the_repository);
>
> This means we potentially use different diff algorithms when merging the
> index and when merging the work tree, let's use the _ui variant here
> instead.

Yep, you know I'd spotted that and wasn't expecting it to make a meaningful difference. It's an easy swap, but I thought that (like above, since we don't show the conflict results) the diff algorithm wouldn't matter too much.

Maybe it affects the actual merge-ability, though, in which case I agree using the same is important?

Show 5 quoted lines

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

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.

Show 6 quoted lines

> > +                     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. But it doesn't actually clear the merge options.

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?

Show 21 quoted lines

> > diff --git a/merge-ort.c b/merge-ort.c
> > index c410a5d353..f69a49d48a 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -5035,8 +5035,6 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> >       trace2_region_enter("merge", "sanity checks", opt->repo);
> >       assert(opt->repo);
> >
> > -     assert(opt->branch1 && opt->branch2);
>
> This, and the hunk below, make me nervous. Normally assertions like this
> exist because the pointers are unconditionally dereferenced later on.
> Looking at merge_3way() it asserts opt->ancestor is non-NULL and
> dereferences all three labels. t3903 does not appear to have test
> coverage for the index merge failing (if it did I think we'd see a
> SIGSEV), we should probably add a test that checks the command fails
> leaving the index and work tree untouched, and verifies the message on
> stderr.
>
> Lets set some simple, fixed, ancestor and branch names in
> do_apply_stash() above.

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. I'll probably keep a similar structure as the working tree merge uses, I think.

A fail-to-merge test also seems like a good idea. Let me mull on that.

Thanks for the review.

-- 
D. Ben Knoble