Re: [PATCH v2] pack-objects: lazily set up "struct rev_info", don't leak

From: Derrick Stolee <derrickstolee@github.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Bagas Sanjaya <bagasdotme@gmail.com>
Subject: Re: [PATCH v2] pack-objects: lazily set up "struct rev_info", don't leak
Date: Mon, 28 Mar 2022 11:58:16 -0400	[thread overview]
Message-ID: <4781bae6-4ac6-1217-b018-aef286cdca7c@github.com> (raw)
In-Reply-To: <patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@gmail.com>

On 3/28/2022 11:43 AM, Ævar Arnfjörð Bjarmason wrote:
> An earlier version of this commit[3] went behind
> opt_parse_list_objects_filter()'s back by faking up a "struct option"
> before calling it. Let's avoid that and instead create a blessed API
> for this pattern.

...

> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index f02d8df1422..4b25287886d 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -285,6 +285,10 @@ int opt_parse_list_objects_filter(const struct option *opt,
>  				  const char *arg, int unset)
>  {
>  	struct list_objects_filter_options *filter_options = opt->value;
> +	opt_lof_init init = (opt_lof_init)opt->defval;
> +
> +	if (init)
> +		filter_options = init(opt->value);
>  
>  	if (unset || !arg)
>  		list_objects_filter_set_no_filter(filter_options);
> diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
> index 90e4bc96252..ffc02d77e76 100644
> --- a/list-objects-filter-options.h
> +++ b/list-objects-filter-options.h
> @@ -104,13 +104,31 @@ void parse_list_objects_filter(
>  	struct list_objects_filter_options *filter_options,
>  	const char *arg);
>  
> +/**
> + * The opt->value to opt_parse_list_objects_filter() is either a
> + * "struct list_objects_filter_option *" when using
> + * OPT_PARSE_LIST_OBJECTS_FILTER().
> + *
> + * Or, if using no "struct option" field is used by the callback,
> + * except the "defval" which is expected to be an "opt_lof_init"
> + * function, which is called with the "opt->value" and must return a
> + * pointer to the ""struct list_objects_filter_option *" to be used.
> + *
> + * The OPT_PARSE_LIST_OBJECTS_FILTER_INIT() can be used e.g. the
> + * "struct list_objects_filter_option" is embedded in a "struct
> + * rev_info", which the "defval" could be tasked with lazily
> + * initializing. See cmd_pack_objects() for an example.
> + */
>  int opt_parse_list_objects_filter(const struct option *opt,
>  				  const char *arg, int unset);
> +typedef struct list_objects_filter_options *(*opt_lof_init)(void *);
> +#define OPT_PARSE_LIST_OBJECTS_FILTER_INIT(fo, init) \
> +	{ OPTION_CALLBACK, 0, "filter", (fo), N_("args"), \
> +	  N_("object filtering"), 0, opt_parse_list_objects_filter, \
> +	  (intptr_t)(init) }
>  
>  #define OPT_PARSE_LIST_OBJECTS_FILTER(fo) \
> -	OPT_CALLBACK(0, "filter", fo, N_("args"), \
> -	  N_("object filtering"), \
> -	  opt_parse_list_objects_filter)
> +	OPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)

I like this way of combining the macros into one implementation
(guarded with a NULL check).

This version looks good to me.

Thanks,
-Stolee