Re: [PATCH] remote: clear string_list after use in mv()

["René Scharfe" <l.s.r@web.de>]

Thank you for the review!

Am 02.08.2018 um 04:56 schrieb Jonathan Nieder:
> René Scharfe wrote:
> 
>> Subject: remote: clear string_list after use in mv()
> 
> This subject line doesn't fully reflect the goodness of this change.
> How about something like:
> 
> 	remote mv: avoid leaking ref names in string_list
> 
> ?

Hmm, "clearing" a string_list is how we release its memory, and since we
didn't do it before (otherwise we wouldn't need the patch) it leaked
before.  So I think the title implies plugging a leak already.

The ref names don't take up much space -- they are probably in the range
of hundreds to thousands and probably take up less than 100 bytes each.
The command exits after calling mv(), so this is a minor leak which
won't be noticed by users.

The benefit of this patch is to bring this function in line with its
siblings, which all release their string_lists when done.

>> --- a/builtin/remote.c
>> +++ b/builtin/remote.c
>> @@ -558,23 +558,23 @@ struct rename_info {
> 
> optional: Would it be worth a comment in the struct definition to say
> this string_list owns its items (or in other words that it's a _DUP
> string_list)?

Such a comment could easily get out of sync with the assignment later in
the code.  And the struct could easily be used with both types of
string_lists, even if that's not the case here, so I don't think that's
the right place.

We could add an assert(rename->remote_branches->strdup_strings) before
the string_list_append() call instead, which would document that
requirement in the right place in a way that shouldn't get out of sync
silently, but I'm not sure it's worth it here.

>>   	if (!refspec_updated)
>>   		return 0;
>>   
>>   	/*
>>   	 * First remove symrefs, then rename the rest, finally create
>>   	 * the new symrefs.
>>   	 */
>>   	for_each_ref(read_remote_branches, &rename);
> 
> As you noted, this is the first caller that writes to the string_list,
> so we don't have to worry about the 'return 0' above.  That said, I
> wonder if it might be simpler and more futureproof to use
> destructor-style cleanup handling anyway:
> 
> 	if (!refspec_updated)
> 		goto done;
>   [...]
>    done:
> 	string_list_clear(&remote_branches, 1);
> 	return 0;

There are some more early returns higher up, which would have to be
adjusted as well.  Such a restructuring would be helpful if we decide
to release the various strbufs in that function as well..

But perhaps the main problem is that the function is too long. Could
it be split up into sensible parts with a lower number of allocations
each that can be kept track of more easily?

(Sounds like a bigger bite than I can chew at the moment, though.)

>> +	string_list_clear(&remote_branches, 1);
> 
> not about this patch: I wonder if we should make the free_util
> parameter a flag word so the behavior is more obvious in the caller:
> 
> 	string_list_clear(&remote_branches, STRING_LIST_FREE_UTIL);
> 
> Or maybe even having a separate function:
> 
> 	string_list_clear_free_util(&remote_branches);

I agree that naming variants instead of using binary options is a good
idea in general, as it makes the code self-documenting.

I'd like to suggest another option: remove the second parameter of
string_list_clear() and add string_list_free_util(), which only free(3)s
->util pointers.  Users that attached heap objects to string_list items
would have to call both functions; no need to glue them together.

René