Hi,
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
?
> Switch to the _DUP variant of string_list for remote_branches to allow
> string_list_clear() to release the allocated memory at the end, and
> actually call that function. Free the util pointer as well; it is
> allocated in read_remote_branches().
>
> NB: This string_list is empty until read_remote_branches() is called
> via for_each_ref(), so there is no need to clean it up when returning
> before that point.
>
> Signed-off-by: Rene Scharfe <l.s.r@xxxxxx>
> ---
> Patch generated with ---function-context for easier review -- that
> makes it look much bigger than it actually is, though.
Thanks, that indeed helps.
[...]
> --- 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)?
I think we're fine without, since it's local to the file, but may make
sense to do if rerolling.
[...]
> @@ -607,133 +607,134 @@ static int migrate_file(struct remote *remote)
> static int mv(int argc, const char **argv)
> {
> struct option options[] = {
> OPT_END()
> };
> struct remote *oldremote, *newremote;
> struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,
> old_remote_context = STRBUF_INIT;
> - struct string_list remote_branches = STRING_LIST_INIT_NODUP;
> + struct string_list remote_branches = STRING_LIST_INIT_DUP;
Verified that these are the only two functions that touch the
remote_branches field. This patch didn't miss any callers.
[...]
> 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;
[...]
> + 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);
That's a topic for another day, though.
With whatever subset of the changes suggested above makes sense,
Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
Thanks for a pleasant read.
