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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2022, 17:10 UTC
Message-ID: <xmqqmtha2eb9.fsf@gitster.g>
In-Reply-To: <patch-v2-1.1-9951d92176e-20220328T154049Z-avarab@gmail.com>

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

Show 7 quoted lines

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

Show 9 quoted lines

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

Show 6 quoted lines

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