Hi,
From: Jonathan Nieder <jrnieder@xxxxxxxxx>
> Hi,
>
> Christian Couder wrote:
>
>> add_rev_cmdline() in revision.c is (re)allocating an array of
>> struct rev_cmdline_entry. This patch releases it.
> [...]
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -217,6 +217,8 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>> git_config(git_default_config, NULL);
>> parse_args(argc, argv, &opts);
>> res = sequencer_pick_revisions(&opts);
>> + if (opts.revs)
>> + free(opts.revs->cmdline.rev);
>> free(opts.revs);
>> if (res < 0)
>> die(_("revert failed"));
>
> Quick thoughts:
>
> This feels like a layering violation. Avoidable? Maybe revision.c
> could gain a helper to allow this to be written like so:
>
> free_rev_info(opts.revs);
> free(opts.revs);
>
> Since this is a one-time allocation it is probably worth mentioning in
> the log message that this is a futureproofing/valgrind-cleanliness
> measure and is not actually fixing a leak.
Ok, I will create the free_rev_info() helper and improve the commit
message.
> Micronit: it would feel slightly more comfortable if the free() were
> after the die(), even though the die() should probably be changed to
> exit(). That way someone wanting to add code after the die() that
> continues to assume opts.revs is valid would be able to.
>
> Of course I'd imagine the largest leak in cherry-pick is the
> deliberately constantly growing object hash table. It would be very
> interesting to fix that --- do you know how libgit2 handles it?
About the constantly growing hash table, perhaps it should be taken
care of in a try_to_free_routine() used by xmalloc and other such
functions. And no I don't know much about libgit2.
> Thanks much and hope that helps,
> Jonathan
Thanks for your kind review,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html