This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518.
5cb28270a1 (pack-objects: lazily set up "struct rev_info", don't leak,
2022-03-28) avoided leaking rev_info allocations in many cases by
calling repo_init_revisions() only when the .filter member was actually
needed, but then still leaking it. That was fixed later by 2108fe4a19
(revisions API users: add straightforward release_revisions(),
2022-04-13), making the reverted commit unnecessary.
5cb28270a1 broke support for multiple --filter options by calling
repo_init_revisions() for each one, clobbering earlier state and
leaking rev_info allocations. Reverting it fixes that regression.
Luckily it only affects users that run pack-objects directly. Internal
callers in bundle.c and upload-pack.c combine multiple filters into a
single option using "+".
5cb28270a1 introduced OPT_PARSE_LIST_OBJECTS_FILTER_INIT. It relies on
being able to faithfully convert function pointers to object pointers
and back, which is undefined in C99. The standard mentions this ability
as a common extension in its annex J (J.5.7 Function pointer casts), but
avoiding that dependency is more portable. The macro hasn't been used
since, OPT_PARSE_LIST_OBJECTS_FILTER is enough so far.
So revert 5cb28270a1 fully to fix support for multiple --filter options
and avoid reliance on undefined behavior. Its intent is better served
by the release_revisions() call added by 2108fe4a19 alone -- we just
need to do it unconditionally, mirroring the unconditional call of
repo_init_revisions().
Helped-by: Jeff King <peff@xxxxxxxx>
Signed-off-by: René Scharfe <l.s.r@xxxxxx>
---
builtin/pack-objects.c | 31 +++++---------------------
list-objects-filter-options.c | 4 ----
list-objects-filter-options.h | 24 +++-----------------
t/t5317-pack-objects-filter-objects.sh | 2 +-
4 files changed, 10 insertions(+), 51 deletions(-)
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 573d0b20b7..3e74fbb0cd 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4149,21 +4149,6 @@ static int option_parse_cruft_expiration(const struct option *opt,
return 0;
}
-struct po_filter_data {
- unsigned have_revs:1;
- struct rev_info revs;
-};
-
-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;
-}
-
int cmd_pack_objects(int argc, const char **argv, const char *prefix)
{
int use_internal_rev_list = 0;
@@ -4174,7 +4159,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
int rev_list_index = 0;
int stdin_packs = 0;
struct string_list keep_pack_list = STRING_LIST_INIT_NODUP;
- struct po_filter_data pfd = { .have_revs = 0 };
+ struct rev_info revs;
struct option pack_objects_options[] = {
OPT_SET_INT('q', "quiet", &progress,
@@ -4265,7 +4250,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_INIT(&pfd, po_filter_revs_init),
+ OPT_PARSE_LIST_OBJECTS_FILTER(&revs.filter),
OPT_CALLBACK_F(0, "missing", NULL, N_("action"),
N_("handling for missing objects"), PARSE_OPT_NONEG,
option_parse_missing_action),
@@ -4284,6 +4269,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
read_replace_refs = 0;
+ repo_init_revisions(the_repository, &revs, NULL);
+
sparse = git_env_bool("GIT_TEST_PACK_SPARSE", -1);
if (the_repository->gitdir) {
prepare_repo_settings(the_repository);
@@ -4385,7 +4372,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
if (!rev_list_all || !rev_list_reflog || !rev_list_index)
unpack_unreachable_expiration = 0;
- if (pfd.have_revs && pfd.revs.filter.choice) {
+ if (revs.filter.choice) {
if (!pack_to_stdout)
die(_("cannot use --filter without --stdout"));
if (stdin_packs)
@@ -4472,16 +4459,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
read_cruft_objects();
} else if (!use_internal_rev_list) {
read_object_list_from_stdin();
- } else if (pfd.have_revs) {
- get_object_list(&pfd.revs, rp.nr, rp.v);
- release_revisions(&pfd.revs);
} else {
- struct rev_info revs;
-
- repo_init_revisions(the_repository, &revs, NULL);
get_object_list(&revs, rp.nr, rp.v);
- release_revisions(&revs);
}
+ release_revisions(&revs);
cleanup_preferred_base();
if (include_tag && nr_result)
for_each_tag_ref(add_ref_tag, NULL);
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 5339660238..ee01bcd2cc 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -290,10 +290,6 @@ 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 7eeadab2dd..64af2e29e2 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -107,31 +107,13 @@ 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_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)
+ OPT_CALLBACK(0, "filter", fo, N_("args"), \
+ N_("object filtering"), \
+ opt_parse_list_objects_filter)
/*
* Translates abbreviated numbers in the filter's filter_spec into their
diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
index 25faebaada..5b707d911b 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -265,7 +265,7 @@ test_expect_success 'verify normal and blob:limit packfiles have same commits/tr
test_cmp expected observed
'
-test_expect_failure 'verify small limit and big limit results in small limit' '
+test_expect_success 'verify small limit and big limit results in small limit' '
git -C r2 ls-files -s large.1000 >ls_files_result &&
test_parse_ls_files_stage_oids <ls_files_result |
sort >expected &&
--
2.38.1
