Web lists-archives.com

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




On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor <szeder.dev@xxxxxxxxx> wrote:
> From: Jeff King <peff@xxxxxxxx>
>
> 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@xxxxxxxx>
> Signed-off-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
> ---
>  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...