Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list

[Jeff King <peff@peff.net>]

On Mon, May 15, 2017 at 01:29:07PM +0200, SZEDER Gábor wrote:

> On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > From: Jeff King <peff@peff.net>
> >
> > Using free() on a refspec was always leaky, as its string
> > fields also need freed. But it became more so when ad00f128d
> > (clone: respect configured fetch respecs during initial
> > fetch, 2016-03-30) taught clone to create a list of
> > refspecs, each of which need to be freed.
> >
> > [sg: adjusted the function parameters.]
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  builtin/clone.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 4144190da..4bf28d7f5 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >         strbuf_release(&value);
> >         junk_mode = JUNK_LEAVE_ALL;
> >
> > -       free(refspec);
> > +       free_refspec(remote->fetch_refspec_nr + 1, remote->fetch);
> >         return err;
> >  }
> 
> Erm...  I should have given a bit more thought to this last patch,
> shouldn't I?
> 
> First, the unchanged commit message is now (i.e. by using the parsed
> refspecs returned by remote_get()) completely outdated.
> Second, while it properly frees those refspecs, i.e. the array and all
> its string fields, it will now leak the memory pointed by the
> 'refspec' variable.  However, why free just that one field of the
> 'struct *remote'?  Alas, we don't seem to have a free_remote()
> function...

Right, the "struct remote" fields are meant to last the program
lifetime, and just go away when the program does. So they are never
freed. We could just lump this in with them, and remove the free()
entirely.

It's a little funny with your patch as-is, because "struct remote"
doesn't actually know about the refspec we stuck on the end. But since
it doesn't free its refspecs anyway, it's not any more leaky than all
the other entries.

-Peff