Re: [PATCH 04/11] branch: fix a leak in dwim_and_setup_tracking
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Jun 16, 2023, 22:45 UTC
- Message-ID
- <e75092a4-2629-85a9-1bd2-3193d6719a65@gmail.com>
- In-Reply-To
- <20230612032106.GD306352@coredump.intra.peff.net>
On 11-jun-2023 23:21:06, Jeff King wrote:
Show 30 quoted lines
> On Sun, Jun 11, 2023 at 08:49:43PM +0200, Rubén Justo wrote:
>
> > In e89f151db1 (branch: move --set-upstream-to behavior to
> > dwim_and_setup_tracking(), 2022-01-28) the string returned by
> > dwim_branch_start() was not freed, resulting in a memory leak.
>
> Yep, looks good. One small comment:
>
> > diff --git a/branch.c b/branch.c
> > index ba3914adf5..a7333a4c32 100644
> > --- a/branch.c
> > +++ b/branch.c
> > @@ -638,9 +638,10 @@ void dwim_and_setup_tracking(struct repository *r, const char *new_ref,
> > const char *orig_ref, enum branch_track track,
> > int quiet)
> > {
> > - char *real_orig_ref;
> > + char *real_orig_ref = NULL;
> > dwim_branch_start(r, orig_ref, track, &real_orig_ref, NULL);
> > setup_tracking(new_ref, real_orig_ref, track, quiet);
> > + free(real_orig_ref);
> > }
>
> Adding the NULL-initialization signals that we expect that real_orig_ref
> might sometimes _not_ be filled in by dwim_branch_start(). But I think
> it always will be; and if it weren't, that would make passing it to
> setup_tracking() rather suspicious.
>
> So I don't think the NULL initialization is wrong, and certainly it is
> more defensive. But I find it a little misleading.I don't have any objection to this. But I've seen Junio has already merged this to 'next'. I'm not going to re-roll this commit. Sorry.
Thank you for your review.