git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
René Scharfe <l.s.r@web.de>
Date
Nov 20, 2022, 10:13 UTC
Message-ID
<f5779e19-813c-cda9-2f84-9fe58f745e89@web.de>
In-Reply-To
<d19c6cb4-611f-afea-8a14-5e58d7509113@web.de>
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@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 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
Previous: René ScharfeNext: Junio C Hamano
Message 18 of 52 in “pack-objects: fix and simplify --filter handling”
  1. 0/3 pack-objects: fix and simplify --filter handlingRené Scharfe, Nov 12, 2022
  2. 1/3 pack-objects: fix handling of multiple --filter optionsRené Scharfe, Nov 12, 2022
  3. Ævar Arnfjörð BjarmasonNov 12, 2022
  4. René ScharfeNov 13, 2022
  5. Jeff KingNov 12, 2022
  6. Taylor BlauNov 13, 2022
  7. Jeff KingNov 13, 2022
  8. René ScharfeNov 13, 2022
  9. 2/3 pack-object: simplify --filter handlingRené Scharfe, Nov 12, 2022
  10. Ævar Arnfjörð BjarmasonNov 12, 2022
  11. Jeff KingNov 12, 2022
  12. Jeff KingNov 13, 2022
  13. René ScharfeNov 13, 2022
  14. 3/3 list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()René Scharfe, Nov 12, 2022
  15. 0/3 pack-objects: fix and simplify --filter handlingRené Scharfe, Nov 20, 2022
  16. 2/3 t5317: demonstrate failure to handle multiple --filter optionsRené Scharfe, Nov 20, 2022
  17. 1/3 t5317: stop losing return codes of git ls-filesRené Scharfe, Nov 20, 2022
  18. 3/3 Revert "pack-objects: lazily set up "struct rev_info", don't leak"René Scharfe, Nov 20, 2022
  19. Junio C HamanoNov 28, 2022
  20. René ScharfeNov 28, 2022
  21. Ævar Arnfjörð BjarmasonNov 28, 2022
  22. Ævar Arnfjörð BjarmasonNov 28, 2022
  23. René ScharfeNov 28, 2022
  24. Ævar Arnfjörð BjarmasonNov 28, 2022
  25. René ScharfeNov 28, 2022
  26. Ævar Arnfjörð BjarmasonNov 28, 2022
  27. René ScharfeNov 28, 2022
  28. René ScharfeNov 28, 2022
  29. Ævar Arnfjörð BjarmasonNov 28, 2022
  30. René ScharfeNov 28, 2022
  31. Jeff KingNov 29, 2022
  32. Junio C HamanoNov 29, 2022
  33. Ævar Arnfjörð BjarmasonNov 29, 2022
  34. Ævar Arnfjörð BjarmasonNov 29, 2022
  35. René ScharfeNov 29, 2022
  36. Ævar Arnfjörð BjarmasonNov 28, 2022
  37. Ævar Arnfjörð BjarmasonNov 28, 2022
  38. t5314: check exit code of "rev-parse"Ævar Arnfjörð Bjarmason, Nov 28, 2022
  39. René ScharfeNov 28, 2022
  40. t5314: check exit code of "git"Ævar Arnfjörð Bjarmason, Nov 28, 2022
  41. Jeff KingNov 22, 2022
  42. 0/5 pack-objects: fix and simplify --filter handlingRené Scharfe, Nov 29, 2022
  43. 1/5 t5317: stop losing return codes of git ls-filesRené Scharfe, Nov 29, 2022
  44. 2/5 t5317: demonstrate failure to handle multiple --filter optionsRené Scharfe, Nov 29, 2022
  45. 3/5 pack-objects: fix handling of multiple --filter optionsRené Scharfe, Nov 29, 2022
  46. Junio C HamanoNov 30, 2022
  47. René ScharfeNov 30, 2022
  48. 4/5 pack-objects: simplify --filter handlingRené Scharfe, Nov 29, 2022
  49. Ævar Arnfjörð BjarmasonNov 29, 2022
  50. René ScharfeNov 30, 2022
  51. 5/5 list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()René Scharfe, Nov 29, 2022
  52. Junio C HamanoNov 30, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.