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

SZEDER Gábor [off-list ref] writes:

On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor [off-list ref] wrote:

quoted

From: Jeff King <redacted>

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 <redacted>
Signed-off-by: SZEDER Gábor <redacted>
---
 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 [off-list ref], 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'?

On Tue, May 23, 2017 at 04:38:07PM +0900, Junio C Hamano wrote:

quoted

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 [off-list ref], 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'?

I didn't quite say that it was good. I think the overall direction is
good, but I had some comments on patch 1. Maybe we want to accept the
ugliness there, but I had some suggestions that might make it less bad.

-Peff

On Tue, May 23, 2017 at 9:38 AM, Junio C Hamano [off-list ref] wrote:

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 [off-list ref], 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'?

I followed Peff's advice on adding the default fetch refspec to this
temporary configuration environment (what's the proper term for
that?), which resulted in the simplest and cleanest code yet, and, as
a by product made this last patch moot, so I will drop it in the next
version.  The code is ready, but the change requires updates to the
first patch's commit message, maybe I can get around to it in the
evening.