From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Fri, 25 Sep 2026 12:55:03 GMT
Subject: Re: [PATCH v2 4/4] builtin/stash: merge index in-core
Message-ID: <CALnO6CDpS9GQfONKJs=LAUvwYzYyMby+rGAUtvFQruj-ERXt-g@mail.gmail.com>
In-Reply-To: <68e83baa-6ccb-4ca8-a1df-f09d51749c67@gmail.com>

Hi Phillip,

On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Ben
>
> I've spotted a memory leak that I missed last time, apart from that this
> looks good.
>
> On 23/09/2026 13:58, D. Ben Knoble wrote:
> > @@ -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 = "Upstream index";
>
> This is the current index, calling it "upstream" is a bit confusing to
> me but that's not worth a re-roll on its own.

Will fix. The "upstream" verbiage comes from the working tree labels.

> > +                     o.branch2 = "Stashed index changes";
> > +                     o.ancestor = "Stash base";
> >
> > -                     ret = apply_cached(&out);
> > -                     strbuf_release(&out);
> > -                     if (ret)
> > +                     o.verbosity = 0;
> > +
> > +                     head = lookup_tree(o.repo, &c_tree);
> > +                     merge = lookup_tree(o.repo, &info->i_tree);
> > +                     merge_base = lookup_tree(o.repo, &info->b_tree);
> > +
> > +                     merge_incore_nonrecursive(&o, head, merge, merge_base,
> > +                                               &result);
> > +
> > +                     if (!result.clean)
> >                               return error(_("conflicts in index. "
> >                                              "Try without --index."));
>
> Sorry, I missed this last time, but we should finalize the merge before
> returning to ensure the allocations in result are freed.

Yeah, I think CI caught this:
https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31

But I'm not sure I could have understood what it was telling me
without your hint, thanks!

-- 
D. Ben Knoble