Re: [PATCH v2] pack-objects: lazily set up "struct rev_info", don't leak
From: Derrick Stolee <hidden>
Date: 2022-03-28 15:58:25
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.
...
quoted hunk ↗ jump to hunk
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