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

Hi Ben

On 22/09/2026 13:43, D. Ben Knoble wrote:

On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood [off-list ref] wrote:

quoted

On 19/09/2026 22:26, D. Ben Knoble wrote:

quoted

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!

quoted

quoted

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

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.

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!

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.

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)

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

quoted

quoted

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.

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.

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

That's great

Thanks

Phillip

Thanks for the review.