Merge branch 'ds/partial-bundle-more' into ab/plug-leak-in-revisions

gitster · gitster · commit 1e2574e5854e · 2022-04-03T15:03:05.000-07:00
* ds/partial-bundle-more:
  pack-objects: lazily set up "struct rev_info", don't leak
  bundle: output hash information in 'verify'
  bundle: move capabilities to end of 'verify'
  pack-objects: parse --filter directly into revs.filter
  pack-objects: move revs out of get_object_list()
  list-objects-filter: remove CL_ARG__FILTER
diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
@@ -75,11 +75,11 @@ verify <file>::
 	cleanly to the current repository.  This includes checks on the
 	bundle format itself as well as checking that the prerequisite
 	commits exist and are fully linked in the current repository.
-	Information about additional capabilities, such as "object filter",
-	is printed. See "Capabilities" in link:technical/bundle-format.html
-	for more information. Finally, 'git bundle' prints a list of
-	missing commits, if any. The exit code is zero for success, but
-	will be nonzero if the bundle file is invalid.
+	Then, 'git bundle' prints a list of missing commits, if any.
+	Finally, information about additional capabilities, such as "object
+	filter", is printed. See "Capabilities" in link:technical/bundle-format.html
+	for more information. The exit code is zero for success, but will
+	be nonzero if the bundle file is invalid.
 
 list-heads <file>::
 	Lists the references defined in the bundle.  If followed by a
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
@@ -153,11 +153,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 			args.from_promisor = 1;
 			continue;
 		}
-		if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {
+		if (skip_prefix(arg, ("--filter="), &arg)) {
 			parse_list_objects_filter(&args.filter_options, arg);
 			continue;
 		}
-		if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {
+		if (!strcmp(arg, ("--no-filter"))) {
 			list_objects_filter_set_no_filter(&args.filter_options);
 			continue;
 		}
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
@@ -237,8 +237,6 @@ static unsigned long cache_max_small_delta_size = 1000;
 
 static unsigned long window_memory_limit = 0;
 
-static struct list_objects_filter_options filter_options;
-
 static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;
 
 enum missing_action {
@@ -3714,20 +3712,17 @@ static void mark_bitmap_preferred_tips(void)
 	}
 }
 
-static void get_object_list(int ac, const char **av)
+static void get_object_list(struct rev_info *revs, int ac, const char **av)
 {
-	struct rev_info revs;
 	struct setup_revision_opt s_r_opt = {
 		.allow_exclude_promisor_objects = 1,
 	};
 	char line[1000];
 	int flags = 0;
 	int save_warning;
 
-	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;
-	setup_revisions(ac, av, &revs, &s_r_opt);
-	list_objects_filter_copy(&revs.filter, &filter_options);
+	setup_revisions(ac, av, revs, &s_r_opt);
 
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
@@ -3757,13 +3752,13 @@ static void get_object_list(int ac, const char **av)
 			}
 			die(_("not a rev '%s'"), line);
 		}
-		if (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))
+		if (handle_revision_arg(line, revs, flags, REVARG_CANNOT_BE_FILENAME))
 			die(_("bad revision '%s'"), line);
 	}
 
 	warn_on_object_refname_ambiguity = save_warning;
 
-	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
+	if (use_bitmap_index && !get_object_list_from_bitmap(revs))
 		return;
 
 	if (use_delta_islands)
@@ -3772,24 +3767,24 @@ static void get_object_list(int ac, const char **av)
 	if (write_bitmap_index)
 		mark_bitmap_preferred_tips();
 
-	if (prepare_revision_walk(&revs))
+	if (prepare_revision_walk(revs))
 		die(_("revision walk setup failed"));
-	mark_edges_uninteresting(&revs, show_edge, sparse);
+	mark_edges_uninteresting(revs, show_edge, sparse);
 
 	if (!fn_show_object)
 		fn_show_object = show_object;
-	traverse_commit_list(&revs,
+	traverse_commit_list(revs,
 			     show_commit, fn_show_object,
 			     NULL);
 
 	if (unpack_unreachable_expiration) {
-		revs.ignore_missing_links = 1;
-		if (add_unseen_recent_objects_to_traversal(&revs,
+		revs->ignore_missing_links = 1;
+		if (add_unseen_recent_objects_to_traversal(revs,
 				unpack_unreachable_expiration))
 			die(_("unable to add recent objects"));
-		if (prepare_revision_walk(&revs))
+		if (prepare_revision_walk(revs))
 			die(_("revision walk setup failed"));
-		traverse_commit_list(&revs, record_recent_commit,
+		traverse_commit_list(revs, record_recent_commit,
 				     record_recent_object, NULL);
 	}
 
@@ -3862,6 +3857,21 @@ static int option_parse_unpack_unreachable(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;
@@ -3872,6 +3882,8 @@ 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 option pack_objects_options[] = {
 		OPT_SET_INT('q', "quiet", &progress,
 			    N_("do not show progress meter"), 0),
@@ -3957,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(&filter_options),
+		OPT_PARSE_LIST_OBJECTS_FILTER_INIT(&pfd, po_filter_revs_init),
 		OPT_CALLBACK_F(0, "missing", NULL, N_("action"),
 		  N_("handling for missing objects"), PARSE_OPT_NONEG,
 		  option_parse_missing_action),
@@ -4077,7 +4089,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 (filter_options.choice) {
+	if (pfd.have_revs && pfd.revs.filter.choice) {
 		if (!pack_to_stdout)
 			die(_("cannot use --filter without --stdout"));
 		if (stdin_packs)
@@ -4153,8 +4165,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			add_unreachable_loose_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);
 	} else {
-		get_object_list(rp.nr, rp.v);
+		struct rev_info revs;
+
+		repo_init_revisions(the_repository, &revs, NULL);
+		get_object_list(&revs, rp.nr, rp.v);
 	}
 	cleanup_preferred_base();
 	if (include_tag && nr_result)
diff --git a/bundle.c b/bundle.c
@@ -267,11 +267,6 @@ int verify_bundle(struct repository *r,
 			  (uintmax_t)r->nr);
 		list_refs(r, 0, NULL);
 
-		if (header->filter.choice) {
-			printf_ln("The bundle uses this filter: %s",
-				  list_objects_filter_spec(&header->filter));
-		}
-
 		r = &header->prerequisites;
 		if (!r->nr) {
 			printf_ln(_("The bundle records a complete history."));
@@ -282,6 +277,12 @@ int verify_bundle(struct repository *r,
 				  (uintmax_t)r->nr);
 			list_refs(r, 0, NULL);
 		}
+
+		printf_ln("The bundle uses this hash algorithm: %s",
+			  header->hash_algo->name);
+		if (header->filter.choice)
+			printf_ln("The bundle uses this filter: %s",
+				  list_objects_filter_spec(&header->filter));
 	}
 	return ret;
 }
diff --git 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
@@ -69,9 +69,6 @@ struct list_objects_filter_options {
 	 */
 };
 
-/* Normalized command line arguments */
-#define CL_ARG__FILTER "filter"
-
 /*
  * Parse value of the argument to the "filter" keyword.
  * On the command line this looks like:
@@ -107,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, CL_ARG__FILTER, fo, N_("args"), \
-	  N_("object filtering"), \
-	  opt_parse_list_objects_filter)
+	OPT_PARSE_LIST_OBJECTS_FILTER_INIT((fo), NULL)
 
 /*
  * Translates abbreviated numbers in the filter's filter_spec into their
diff --git a/revision.c b/revision.c
@@ -2691,9 +2691,9 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 		revs->no_walk = 0;
 	} else if (!strcmp(arg, "--single-worktree")) {
 		revs->single_worktree = 1;
-	} else if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {
+	} else if (skip_prefix(arg, ("--filter="), &arg)) {
 		parse_list_objects_filter(&revs->filter, arg);
-	} else if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {
+	} else if (!strcmp(arg, ("--no-filter"))) {
 		list_objects_filter_set_no_filter(&revs->filter);
 	} else {
 		return 0;
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
@@ -122,6 +122,8 @@ format_and_save_expect () {
 	sed -e 's/Z$//' >expect
 }
 
+HASH_MESSAGE="The bundle uses this hash algorithm: $GIT_DEFAULT_HASH"
+
 #            (C)   (D, pull/1/head, topic/1)
 #             o --- o
 #            /       \                              (L)
@@ -194,11 +196,12 @@ test_expect_success 'create bundle from special rev: main^!' '
 
 	git bundle verify special-rev.bdl |
 		make_user_friendly_and_stable_output >actual &&
-	format_and_save_expect <<-\EOF &&
+	format_and_save_expect <<-EOF &&
 	The bundle contains this ref:
 	<COMMIT-P> refs/heads/main
 	The bundle requires this ref:
 	<COMMIT-O> Z
+	$HASH_MESSAGE
 	EOF
 	test_cmp expect actual &&
 
@@ -215,12 +218,13 @@ test_expect_success 'create bundle with --max-count option' '
 
 	git bundle verify max-count.bdl |
 		make_user_friendly_and_stable_output >actual &&
-	format_and_save_expect <<-\EOF &&
+	format_and_save_expect <<-EOF &&
 	The bundle contains these 2 refs:
 	<COMMIT-P> refs/heads/main
 	<TAG-1> refs/tags/v1
 	The bundle requires this ref:
 	<COMMIT-O> Z
+	$HASH_MESSAGE
 	EOF
 	test_cmp expect actual &&
 
@@ -240,7 +244,7 @@ test_expect_success 'create bundle with --since option' '
 
 	git bundle verify since.bdl |
 		make_user_friendly_and_stable_output >actual &&
-	format_and_save_expect <<-\EOF &&
+	format_and_save_expect <<-EOF &&
 	The bundle contains these 5 refs:
 	<COMMIT-P> refs/heads/main
 	<COMMIT-N> refs/heads/release
@@ -250,6 +254,7 @@ test_expect_success 'create bundle with --since option' '
 	The bundle requires these 2 refs:
 	<COMMIT-M> Z
 	<COMMIT-K> Z
+	$HASH_MESSAGE
 	EOF
 	test_cmp expect actual &&
 
@@ -267,11 +272,12 @@ test_expect_success 'create bundle 1 - no prerequisites' '
 	EOF
 	git bundle create stdin-1.bdl --stdin <input &&
 
-	cat >expect <<-\EOF &&
+	format_and_save_expect <<-EOF &&
 	The bundle contains these 2 refs:
 	<COMMIT-D> refs/heads/topic/1
 	<COMMIT-H> refs/heads/topic/2
 	The bundle records a complete history.
+	$HASH_MESSAGE
 	EOF
 
 	# verify bundle, which has no prerequisites
@@ -308,13 +314,14 @@ test_expect_success 'create bundle 2 - has prerequisites' '
 		--stdin \
 		release <input &&
 
-	format_and_save_expect <<-\EOF &&
+	format_and_save_expect <<-EOF &&
 	The bundle contains this ref:
 	<COMMIT-N> refs/heads/release
 	The bundle requires these 3 refs:
 	<COMMIT-D> Z
 	<COMMIT-E> Z
 	<COMMIT-G> Z
+	$HASH_MESSAGE
 	EOF
 
 	git bundle verify 2.bdl |
@@ -367,13 +374,14 @@ test_expect_success 'create bundle 3 - two refs, same object' '
 		--stdin \
 		main HEAD <input &&
 
-	format_and_save_expect <<-\EOF &&
+	format_and_save_expect <<-EOF &&
 	The bundle contains these 2 refs:
 	<COMMIT-P> refs/heads/main
 	<COMMIT-P> HEAD
 	The bundle requires these 2 refs:
 	<COMMIT-M> Z
 	<COMMIT-K> Z
+	$HASH_MESSAGE
 	EOF
 
 	git bundle verify 3.bdl |
@@ -409,12 +417,13 @@ test_expect_success 'create bundle 4 - with tags' '
 		--stdin \
 		--all <input &&
 
-	cat >expect <<-\EOF &&
+	cat >expect <<-EOF &&
 	The bundle contains these 3 refs:
 	<TAG-1> refs/tags/v1
 	<TAG-2> refs/tags/v2
 	<TAG-3> refs/tags/v3
 	The bundle records a complete history.
+	$HASH_MESSAGE
 	EOF
 
 	git bundle verify 4.bdl |
@@ -510,8 +519,9 @@ do
 		<TAG-2> refs/tags/v2
 		<TAG-3> refs/tags/v3
 		<COMMIT-P> HEAD
-		The bundle uses this filter: $filter
 		The bundle records a complete history.
+		$HASH_MESSAGE
+		The bundle uses this filter: $filter
 		EOF
 		test_cmp expect actual &&
 

Original file line number	Diff line number	Diff line change
`@@ -153,11 +153,11 @@ int cmd_fetch_pack(int argc, const char *argv, const char prefix)`
`153`	`153`	`args.from_promisor = 1;`
`154`	`154`	`continue;`
`155`	`155`	`}`
`156`		`- if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) {`
	`156`	`+ if (skip_prefix(arg, ("--filter="), &arg)) {`
`157`	`157`	`parse_list_objects_filter(&args.filter_options, arg);`
`158`	`158`	`continue;`
`159`	`159`	`}`
`160`		`- if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) {`
	`160`	`+ if (!strcmp(arg, ("--no-filter"))) {`
`161`	`161`	`list_objects_filter_set_no_filter(&args.filter_options);`
`162`	`162`	`continue;`
`163`	`163`	`}`