From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Mon, 28 Sep 2026 12:02:25 GMT
Subject: Re: [PATCH v3 5/5] builtin/stash: merge index in-core
Message-ID: <CALnO6CBTsfMsPrkSMHj6bRMqHc3vEfNEJnh6vz=2+_qCf_26Sg@mail.gmail.com>
In-Reply-To: <xmqqo6dir04i.fsf@gitster.g>

On Sun, Sep 27, 2026 at 2:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
>
> > @@ -671,29 +627,27 @@ 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));
> > -                     }
> > +                     o.branch1 = "Current index";
> > +                     o.branch2 = "Stashed index changes";
> > +                     o.ancestor = "Stash base";
> >
> > -                     ret = apply_cached(&out);
> > -                     strbuf_release(&out);
> > -                     if (ret)
> > +                     o.verbosity = 0;
>
> We realize that 'o' is a struct merge_options defined on the stack
> for this function, initialized with init_ui_merge_options() fairly
> early on.  It would have initialized '.verbosity' to the default
> verbosity, the merge.verbosity configuration variable, or the
> GIT_MERGE_VERBOSITY environment variable.
>
> You drop the verbosity here, presumably because you want to match
> the previous implementation 'diff-tree | apply --cached' (which I
> guess was fairly quiet, but I do not use 'stash pop --index'
> myself).

Yes, the original piped "apply --cached" output to a strbuf and discarded it.

> > +
> > +                     if (!result.clean)
> >                               return error(_("conflicts in index. "
> >                                              "Try without --index."));
> > -
> > -                     discard_index(the_repository->index);
> > -                     repo_read_index(the_repository);
> > -                     if (write_index_as_tree(&index_tree, the_repository->index,
> > -                                             repo_get_index_file(the_repository), 0, NULL))
> > -                             return error(_("could not save index tree"));
> > -
> > -                     reset_head();
> > -                     discard_index(the_repository->index);
> > -                     repo_read_index(the_repository);
> >               }
> >       }
>
>
> But the thing is, this is not the end of the function, or the last
> call to the merge machinery using 'o'.  We then use the same 'o' to
> drive another three-way merge.  Yet nobody restores '.verbosity'
> that was unconditionally turned off above for that second merge.

But you're right, we should restore the verbosity (which is not what
the sketch patch does exactly). Will fix.