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

[Junio C Hamano <gitster@pobox.com>]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> 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...

I was sifting entries in the draft "What's cooking" report to find
topics to merge to 'next'.  I read the series over and as Peff said
in his <20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net>, I
think the series overall is good.

Do you want to update the proposed log message for this one before
the entire thing is merged to 'next'?