Re: [PATCH v3 6/6] revisions API: don't leak memory on argv elements that need free()-ing

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 3, 2022, 18:30 UTC
Message-ID: <xmqq8ro59o3d.fsf@gitster.g>
In-Reply-To: <patch-v3-6.6-5e637f55ff1-20220802T152925Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 6 quoted lines

> @@ -2784,6 +2784,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  			const char *arg = argv[i];
>  			if (strcmp(arg, "--"))
>  				continue;
> +			if (opt && opt->free_removed_argv_elements)
> +				free((char *)argv[i]);

We have "arg", let's free that instead.

>  			argv[i] = NULL;
>  			argc = i;

>  	unsigned int	assume_dashdash:1,
> -			allow_exclude_promisor_objects:1;
> +			allow_exclude_promisor_objects:1,
> +			free_removed_argv_elements:1;

It would be nicer to pick a name that explains why we want to "free removed argv[] elements" than "if you remove argv[] elements, then please free them", because the explanation on the reason why we request the API to free would help future developers to decide if they need to free in some situations where they do not "remove" but do something else in their updates to the revisions API.

If we cannot come up with a better name, at least we should add a comment that says that the caller owns the **argv strings, and it asks the API to free those if reference to them are lost in any way.

Thanks.  Overall the series was a pleasant read.