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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 21, 2012, 20:39 UTC
Message-ID: <20120521203941.GB28331@burratino>
In-Reply-To: <20120521145610.1911.46356.chriscool@tuxfamily.org>

Hi,

Christian Couder wrote:

> add_rev_cmdline() in revision.c is (re)allocating an array of
> struct rev_cmdline_entry. This patch releases it.

[...]

Show 11 quoted lines

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