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

> On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> 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!
> 
>>> @@ -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?

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

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

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

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