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

[Jonathan Nieder <jrnieder@gmail.com>]

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?

Thanks much and hope that helps,
Jonathan