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

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.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 10:10:34 -0700	[thread overview]
Message-ID: <xmqqmtha2eb9.fsf@gitster.g> (raw)
In-Reply-To: <patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@gmail.com> (=?utf-8?B?IsOGdmFyIEFybmZqw7Zyw7A=?= Bjarmason"'s message of "Mon, 28 Mar 2022 17:43:18 +0200")

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

> 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(
> ...
> +typedef struct list_objects_filter_options *(*opt_lof_init)(void *);

Do we want or need to expose this type to anybody outside the
list_objects_filter_options.c file?  After all, the client has to
define a function of that type like so:

> +static struct list_objects_filter_options *po_filter_revs_init(void *value)
> +{
> +	struct po_filter_data *data = value;
> +
> +	repo_init_revisions(the_repository, &data->revs, NULL);
> +	data->have_revs = 1;
> +
> +	return &data->revs.filter;
> +}

and not

	static opt_lof_init po_filter_revs_init
	{
		body of the function
	}

and the use of it, i.e. 

> @@ -3954,7 +3969,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  			      &write_bitmap_index,
>  			      N_("write a bitmap index if possible"),
>  			      WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),
> -		OPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),
> +		OPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),

casts the type away to (intptr_t) so having a handy way to spell the
type out does not contribute to type safety, either.

Other than that, looks good to me.

Thanks, will queue directly on top of Derrick's partial-bundle
clean-up series.