Re: [PATCH 5/7] revert: free revs->cmdline.rev

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

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