[PATCH 0/9] fetch: smallish cleanups

[PATCH 0/9] fetch: smallish cleanups

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

Hi,

the dust around machine-parsable fetches has settled a bit, so as
promised this patch series contains a smallish set of cleanups for
git-fetch(1).

- Patch 1/9 drops the unused `DISPLAY_FORMAT_UNKNOWN` enum.

- Patch 2/9 drops a useless `NULL` check as pointed out by Peff.

- The remaining patches convert `git_fetch_config()` to not use global
  state anymore, but instead to parse all config values into the newly
  introduced `fetch_config` structure.

The patches depend on 15ba44f1b4 (Merge branch 'ps/fetch-output-format',
2023-05-15). As this is the last merge before v2.41.0-rc0 I've decided
to thus base this series on top of that tag.

Patrick

Patrick Steinhardt (9):
  fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value
  fetch: drop unneeded NULL-check for `remote_ref`
  fetch: pass through `fetch_config` directly
  fetch: use `fetch_config` to store "fetch.prune" value
  fetch: use `fetch_config` to store "fetch.pruneTags" value
  fetch: use `fetch_config` to store "fetch.showForcedUpdates" value
  fetch: use `fetch_config` to store "fetch.recurseSubmodules" value
  fetch: use `fetch_config` to store "fetch.parallel" value
  fetch: use `fetch_config` to store "submodule.fetchJobs" value

 builtin/fetch.c | 147 ++++++++++++++++++++++++++----------------------
 1 file changed, 79 insertions(+), 68 deletions(-)

-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

With 50957937f9 (fetch: introduce `display_format` enum, 2023-05-10), a
new enumeration was introduced to determine the display format that is
to be used by git-fetch(1). The `DISPLAY_FORMAT_UNKNOWN` value isn't
ever used though, and neither do we rely on the explicit `0` value for
initialization anywhere.

Remove the enum value.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 849a9be421..9147b700e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -58,7 +58,6 @@ enum {
 };
 
 enum display_format {
-	DISPLAY_FORMAT_UNKNOWN = 0,
 	DISPLAY_FORMAT_FULL,
 	DISPLAY_FORMAT_COMPACT,
 	DISPLAY_FORMAT_PORCELAIN,
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

Drop the `NULL` check for `remote_ref` in `update_local_ref()`. The
function only has a single caller, and that caller always passes in a
non-`NULL` value.

This fixes a false positive in Coverity.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9147b700e5..f268322e6f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -953,11 +953,10 @@ static int update_local_ref(struct ref *ref,
 		 * Base this on the remote's ref name, as it's
 		 * more likely to follow a standard layout.
 		 */
-		const char *name = remote_ref ? remote_ref->name : "";
-		if (starts_with(name, "refs/tags/")) {
+		if (starts_with(remote_ref->name, "refs/tags/")) {
 			msg = "storing tag";
 			what = _("[new tag]");
-		} else if (starts_with(name, "refs/heads/")) {
+		} else if (starts_with(remote_ref->name, "refs/heads/")) {
 			msg = "storing head";
 			what = _("[new branch]");
 		} else {
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/9] fetch: pass through `fetch_config` directly

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5729 bytes --]

The `fetch_config` structure currently only has a single member, which
is the `display_format`. We're about extend it to contain all parsed
config values and will thus need it available in most of the code.

Prepare for this change by passing through the `fetch_config` directly
instead of only passing its single member.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f268322e6f..401543e05d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1553,7 +1553,7 @@ static int backfill_tags(struct display_state *display_state,
 
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs,
-		    enum display_format display_format)
+		    const struct fetch_config *config)
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;
@@ -1639,7 +1639,8 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
-	display_state_init(&display_state, ref_map, transport->url, display_format);
+	display_state_init(&display_state, ref_map, transport->url,
+			   config->display_format);
 
 	if (atomic_fetch) {
 		transaction = ref_transaction_begin(&err);
@@ -1828,7 +1829,7 @@ static int add_remote_or_group(const char *name, struct string_list *list)
 }
 
 static void add_options_to_argv(struct strvec *argv,
-				enum display_format format)
+				const struct fetch_config *config)
 {
 	if (dry_run)
 		strvec_push(argv, "--dry-run");
@@ -1864,7 +1865,7 @@ static void add_options_to_argv(struct strvec *argv,
 		strvec_push(argv, "--ipv6");
 	if (!write_fetch_head)
 		strvec_push(argv, "--no-write-fetch-head");
-	if (format == DISPLAY_FORMAT_PORCELAIN)
+	if (config->display_format == DISPLAY_FORMAT_PORCELAIN)
 		strvec_pushf(argv, "--porcelain");
 }
 
@@ -1874,7 +1875,7 @@ struct parallel_fetch_state {
 	const char **argv;
 	struct string_list *remotes;
 	int next, result;
-	enum display_format format;
+	const struct fetch_config *config;
 };
 
 static int fetch_next_remote(struct child_process *cp,
@@ -1894,7 +1895,7 @@ static int fetch_next_remote(struct child_process *cp,
 	strvec_push(&cp->args, remote);
 	cp->git_cmd = 1;
 
-	if (verbosity >= 0 && state->format != DISPLAY_FORMAT_PORCELAIN)
+	if (verbosity >= 0 && state->config->display_format != DISPLAY_FORMAT_PORCELAIN)
 		printf(_("Fetching %s\n"), remote);
 
 	return 1;
@@ -1927,7 +1928,7 @@ static int fetch_finished(int result, struct strbuf *out,
 }
 
 static int fetch_multiple(struct string_list *list, int max_children,
-			  enum display_format format)
+			  const struct fetch_config *config)
 {
 	int i, result = 0;
 	struct strvec argv = STRVEC_INIT;
@@ -1945,10 +1946,10 @@ static int fetch_multiple(struct string_list *list, int max_children,
 	strvec_pushl(&argv, "-c", "fetch.bundleURI=",
 		     "fetch", "--append", "--no-auto-gc",
 		     "--no-write-commit-graph", NULL);
-	add_options_to_argv(&argv, format);
+	add_options_to_argv(&argv, config);
 
 	if (max_children != 1 && list->nr != 1) {
-		struct parallel_fetch_state state = { argv.v, list, 0, 0, format };
+		struct parallel_fetch_state state = { argv.v, list, 0, 0, config };
 		const struct run_process_parallel_opts opts = {
 			.tr2_category = "fetch",
 			.tr2_label = "parallel/fetch",
@@ -1972,7 +1973,7 @@ static int fetch_multiple(struct string_list *list, int max_children,
 
 			strvec_pushv(&cmd.args, argv.v);
 			strvec_push(&cmd.args, name);
-			if (verbosity >= 0 && format != DISPLAY_FORMAT_PORCELAIN)
+			if (verbosity >= 0 && config->display_format != DISPLAY_FORMAT_PORCELAIN)
 				printf(_("Fetching %s\n"), name);
 			cmd.git_cmd = 1;
 			if (run_command(&cmd)) {
@@ -2028,7 +2029,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 
 static int fetch_one(struct remote *remote, int argc, const char **argv,
 		     int prune_tags_ok, int use_stdin_refspecs,
-		     enum display_format display_format)
+		     const struct fetch_config *config)
 {
 	struct refspec rs = REFSPEC_INIT_FETCH;
 	int i;
@@ -2095,7 +2096,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
 	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack_atexit);
 	sigchain_push(SIGPIPE, SIG_IGN);
-	exit_code = do_fetch(gtransport, &rs, display_format);
+	exit_code = do_fetch(gtransport, &rs, config);
 	sigchain_pop(SIGPIPE);
 	refspec_clear(&rs);
 	transport_disconnect(gtransport);
@@ -2383,7 +2384,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		if (filter_options.choice || repo_has_promisor_remote(the_repository))
 			fetch_one_setup_partial(remote);
 		result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,
-				   config.display_format);
+				   &config);
 	} else {
 		int max_children = max_jobs;
 
@@ -2403,7 +2404,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			max_children = fetch_parallel_config;
 
 		/* TODO should this also die if we have a previous partial-clone? */
-		result = fetch_multiple(&list, max_children, config.display_format);
+		result = fetch_multiple(&list, max_children, &config);
 	}
 
 	/*
@@ -2424,7 +2425,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		if (max_children < 0)
 			max_children = fetch_parallel_config;
 
-		add_options_to_argv(&options, config.display_format);
+		add_options_to_argv(&options, &config);
 		result = fetch_submodules(the_repository,
 					  &options,
 					  submodule_prefix,
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/9] fetch: use `fetch_config` to store "fetch.prune" value

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

Move the parsed "fetch.prune" config value into the `fetch_config`
structure. This reduces our reliance on global variables and further
unifies the way we parse the configuration in git-fetch(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 401543e05d..488705b519 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -73,7 +73,6 @@ struct display_state {
 	int url_len, shown_url;
 };
 
-static int fetch_prune_config = -1; /* unspecified */
 static int fetch_show_forced_updates = 1;
 static uint64_t forced_updates_ms = 0;
 static int prefetch = 0;
@@ -108,6 +107,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
 
 struct fetch_config {
 	enum display_format display_format;
+	int prune;
 };
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
@@ -115,7 +115,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	struct fetch_config *fetch_config = cb;
 
 	if (!strcmp(k, "fetch.prune")) {
-		fetch_prune_config = git_config_bool(k, v);
+		fetch_config->prune = git_config_bool(k, v);
 		return 0;
 	}
 
@@ -2047,8 +2047,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
 		/* no command line request */
 		if (0 <= remote->prune)
 			prune = remote->prune;
-		else if (0 <= fetch_prune_config)
-			prune = fetch_prune_config;
+		else if (0 <= config->prune)
+			prune = config->prune;
 		else
 			prune = PRUNE_BY_DEFAULT;
 	}
@@ -2108,6 +2108,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 {
 	struct fetch_config config = {
 		.display_format = DISPLAY_FORMAT_FULL,
+		.prune = -1,
 	};
 	const char *submodule_prefix = "";
 	const char *bundle_uri;
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/9] fetch: use `fetch_config` to store "fetch.pruneTags" value

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2037 bytes --]

Move the parsed "fetch.pruneTags" config value into the `fetch_config`
structure. This reduces our reliance on global variables and further
unifies the way we parse the configuration in git-fetch(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 488705b519..94718bcb2a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -79,7 +79,6 @@ static int prefetch = 0;
 static int prune = -1; /* unspecified */
 #define PRUNE_BY_DEFAULT 0 /* do we prune by default? */
 
-static int fetch_prune_tags_config = -1; /* unspecified */
 static int prune_tags = -1; /* unspecified */
 #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */
 
@@ -108,6 +107,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
 struct fetch_config {
 	enum display_format display_format;
 	int prune;
+	int prune_tags;
 };
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
@@ -120,7 +120,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	}
 
 	if (!strcmp(k, "fetch.prunetags")) {
-		fetch_prune_tags_config = git_config_bool(k, v);
+		fetch_config->prune_tags = git_config_bool(k, v);
 		return 0;
 	}
 
@@ -2057,8 +2057,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
 		/* no command line request */
 		if (0 <= remote->prune_tags)
 			prune_tags = remote->prune_tags;
-		else if (0 <= fetch_prune_tags_config)
-			prune_tags = fetch_prune_tags_config;
+		else if (0 <= config->prune_tags)
+			prune_tags = config->prune_tags;
 		else
 			prune_tags = PRUNE_TAGS_BY_DEFAULT;
 	}
@@ -2109,6 +2109,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	struct fetch_config config = {
 		.display_format = DISPLAY_FORMAT_FULL,
 		.prune = -1,
+		.prune_tags = -1,
 	};
 	const char *submodule_prefix = "";
 	const char *bundle_uri;
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 6/9] fetch: use `fetch_config` to store "fetch.showForcedUpdates" value

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6212 bytes --]

Move the parsed "fetch.showForcedUpdaets" config value into the
`fetch_config` structure. This reduces our reliance on global variables
and further unifies the way we parse the configuration in git-fetch(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 94718bcb2a..eec04d7660 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -73,7 +73,6 @@ struct display_state {
 	int url_len, shown_url;
 };
 
-static int fetch_show_forced_updates = 1;
 static uint64_t forced_updates_ms = 0;
 static int prefetch = 0;
 static int prune = -1; /* unspecified */
@@ -108,6 +107,7 @@ struct fetch_config {
 	enum display_format display_format;
 	int prune;
 	int prune_tags;
+	int show_forced_updates;
 };
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
@@ -125,7 +125,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	}
 
 	if (!strcmp(k, "fetch.showforcedupdates")) {
-		fetch_show_forced_updates = git_config_bool(k, v);
+		fetch_config->show_forced_updates = git_config_bool(k, v);
 		return 0;
 	}
 
@@ -891,7 +891,8 @@ static int update_local_ref(struct ref *ref,
 			    struct ref_transaction *transaction,
 			    struct display_state *display_state,
 			    const struct ref *remote_ref,
-			    int summary_width)
+			    int summary_width,
+			    const struct fetch_config *config)
 {
 	struct commit *current = NULL, *updated;
 	int fast_forward = 0;
@@ -972,7 +973,7 @@ static int update_local_ref(struct ref *ref,
 		return r;
 	}
 
-	if (fetch_show_forced_updates) {
+	if (config->show_forced_updates) {
 		uint64_t t_before = getnanotime();
 		fast_forward = repo_in_merge_bases(the_repository, current,
 						   updated);
@@ -1125,7 +1126,8 @@ static int store_updated_refs(struct display_state *display_state,
 			      const char *remote_name,
 			      int connectivity_checked,
 			      struct ref_transaction *transaction, struct ref *ref_map,
-			      struct fetch_head *fetch_head)
+			      struct fetch_head *fetch_head,
+			      const struct fetch_config *config)
 {
 	int rc = 0;
 	struct strbuf note = STRBUF_INIT;
@@ -1241,7 +1243,7 @@ static int store_updated_refs(struct display_state *display_state,
 
 			if (ref) {
 				rc |= update_local_ref(ref, transaction, display_state,
-						       rm, summary_width);
+						       rm, summary_width, config);
 				free(ref);
 			} else if (write_fetch_head || dry_run) {
 				/*
@@ -1265,7 +1267,7 @@ static int store_updated_refs(struct display_state *display_state,
 		      "branches"), remote_name);
 
 	if (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {
-		if (!fetch_show_forced_updates) {
+		if (!config->show_forced_updates) {
 			warning(_(warn_show_forced_updates));
 		} else if (forced_updates_ms > FORCED_UPDATES_DELAY_WARNING_IN_MS) {
 			warning(_(warn_time_show_forced_updates),
@@ -1326,7 +1328,8 @@ static int fetch_and_consume_refs(struct display_state *display_state,
 				  struct transport *transport,
 				  struct ref_transaction *transaction,
 				  struct ref *ref_map,
-				  struct fetch_head *fetch_head)
+				  struct fetch_head *fetch_head,
+				  const struct fetch_config *config)
 {
 	int connectivity_checked = 1;
 	int ret;
@@ -1349,7 +1352,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,
 	trace2_region_enter("fetch", "consume_refs", the_repository);
 	ret = store_updated_refs(display_state, transport->remote->name,
 				 connectivity_checked, transaction, ref_map,
-				 fetch_head);
+				 fetch_head, config);
 	trace2_region_leave("fetch", "consume_refs", the_repository);
 
 out:
@@ -1520,7 +1523,8 @@ static int backfill_tags(struct display_state *display_state,
 			 struct transport *transport,
 			 struct ref_transaction *transaction,
 			 struct ref *ref_map,
-			 struct fetch_head *fetch_head)
+			 struct fetch_head *fetch_head,
+			 const struct fetch_config *config)
 {
 	int retcode, cannot_reuse;
 
@@ -1541,7 +1545,8 @@ static int backfill_tags(struct display_state *display_state,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map, fetch_head);
+	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,
+					 fetch_head, config);
 
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
@@ -1668,7 +1673,8 @@ static int do_fetch(struct transport *transport,
 			retcode = 1;
 	}
 
-	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, &fetch_head)) {
+	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
+				   &fetch_head, config)) {
 		retcode = 1;
 		goto cleanup;
 	}
@@ -1691,7 +1697,7 @@ static int do_fetch(struct transport *transport,
 			 * the transaction and don't commit anything.
 			 */
 			if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
-					  &fetch_head))
+					  &fetch_head, config))
 				retcode = 1;
 		}
 
@@ -2110,6 +2116,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		.display_format = DISPLAY_FORMAT_FULL,
 		.prune = -1,
 		.prune_tags = -1,
+		.show_forced_updates = 1,
 	};
 	const char *submodule_prefix = "";
 	const char *bundle_uri;
@@ -2207,7 +2214,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			 N_("run 'maintenance --auto' after fetching")),
 		OPT_BOOL(0, "auto-gc", &enable_auto_gc,
 			 N_("run 'maintenance --auto' after fetching")),
-		OPT_BOOL(0, "show-forced-updates", &fetch_show_forced_updates,
+		OPT_BOOL(0, "show-forced-updates", &config.show_forced_updates,
 			 N_("check for forced-updates on all updated branches")),
 		OPT_BOOL(0, "write-commit-graph", &fetch_write_commit_graph,
 			 N_("write the commit-graph after fetching")),
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 7/9] fetch: use `fetch_config` to store "fetch.recurseSubmodules" value

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5664 bytes --]

Move the parsed "fetch.recurseSubmodules" config value into the
`fetch_config` structure. This reduces our reliance on global variables
and further unifies the way we parse the configuration in git-fetch(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index eec04d7660..b40df7e7ca 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -97,7 +97,6 @@ static struct string_list deepen_not = STRING_LIST_INIT_NODUP;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *gtransport;
 static struct transport *gsecondary;
-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static struct refspec refmap = REFSPEC_INIT_FETCH;
 static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
 static struct string_list server_options = STRING_LIST_INIT_DUP;
@@ -108,6 +107,7 @@ struct fetch_config {
 	int prune;
 	int prune_tags;
 	int show_forced_updates;
+	int recurse_submodules;
 };
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
@@ -132,14 +132,14 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	if (!strcmp(k, "submodule.recurse")) {
 		int r = git_config_bool(k, v) ?
 			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
-		recurse_submodules = r;
+		fetch_config->recurse_submodules = r;
 	}
 
 	if (!strcmp(k, "submodule.fetchjobs")) {
 		submodule_fetch_jobs_config = parse_submodule_fetchjobs(k, v);
 		return 0;
 	} else if (!strcmp(k, "fetch.recursesubmodules")) {
-		recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
+		fetch_config->recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
 		return 0;
 	}
 
@@ -1210,7 +1210,7 @@ static int store_updated_refs(struct display_state *display_state,
 				ref->force = rm->peer_ref->force;
 			}
 
-			if (recurse_submodules != RECURSE_SUBMODULES_OFF &&
+			if (config->recurse_submodules != RECURSE_SUBMODULES_OFF &&
 			    (!rm->peer_ref || !oideq(&ref->old_oid, &ref->new_oid))) {
 				check_for_new_submodule_commits(&rm->old_oid);
 			}
@@ -1849,11 +1849,11 @@ static void add_options_to_argv(struct strvec *argv,
 		strvec_push(argv, "--force");
 	if (keep)
 		strvec_push(argv, "--keep");
-	if (recurse_submodules == RECURSE_SUBMODULES_ON)
+	if (config->recurse_submodules == RECURSE_SUBMODULES_ON)
 		strvec_push(argv, "--recurse-submodules");
-	else if (recurse_submodules == RECURSE_SUBMODULES_OFF)
+	else if (config->recurse_submodules == RECURSE_SUBMODULES_OFF)
 		strvec_push(argv, "--no-recurse-submodules");
-	else if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
+	else if (config->recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
 		strvec_push(argv, "--recurse-submodules=on-demand");
 	if (tags == TAGS_SET)
 		strvec_push(argv, "--tags");
@@ -2117,6 +2117,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		.prune = -1,
 		.prune_tags = -1,
 		.show_forced_updates = 1,
+		.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
 	};
 	const char *submodule_prefix = "";
 	const char *bundle_uri;
@@ -2245,7 +2246,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			     builtin_fetch_options, builtin_fetch_usage, 0);
 
 	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
-		recurse_submodules = recurse_submodules_cli;
+		config.recurse_submodules = recurse_submodules_cli;
 
 	if (negotiate_only) {
 		switch (recurse_submodules_cli) {
@@ -2256,7 +2257,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			 * submodules. Skip it by setting recurse_submodules to
 			 * RECURSE_SUBMODULES_OFF.
 			 */
-			recurse_submodules = RECURSE_SUBMODULES_OFF;
+			config.recurse_submodules = RECURSE_SUBMODULES_OFF;
 			break;
 
 		default:
@@ -2265,11 +2266,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
+	if (config.recurse_submodules != RECURSE_SUBMODULES_OFF) {
 		int *sfjc = submodule_fetch_jobs_config == -1
 			    ? &submodule_fetch_jobs_config : NULL;
-		int *rs = recurse_submodules == RECURSE_SUBMODULES_DEFAULT
-			  ? &recurse_submodules : NULL;
+		int *rs = config.recurse_submodules == RECURSE_SUBMODULES_DEFAULT
+			  ? &config.recurse_submodules : NULL;
 
 		fetch_config_from_gitmodules(sfjc, rs);
 	}
@@ -2283,7 +2284,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			 * Reference updates in submodules would be ambiguous
 			 * in porcelain mode, so we reject this combination.
 			 */
-			recurse_submodules = RECURSE_SUBMODULES_OFF;
+			config.recurse_submodules = RECURSE_SUBMODULES_OFF;
 			break;
 
 		default:
@@ -2425,7 +2426,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	 * the fetched history from each remote, so there is no need
 	 * to fetch submodules from here.
 	 */
-	if (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
+	if (!result && remote && (config.recurse_submodules != RECURSE_SUBMODULES_OFF)) {
 		struct strvec options = STRVEC_INIT;
 		int max_children = max_jobs;
 
@@ -2438,7 +2439,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		result = fetch_submodules(the_repository,
 					  &options,
 					  submodule_prefix,
-					  recurse_submodules,
+					  config.recurse_submodules,
 					  recurse_submodules_default,
 					  verbosity < 0,
 					  max_children);
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 8/9] fetch: use `fetch_config` to store "fetch.parallel" value

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]

Move the parsed "fetch.parallel" config value into the `fetch_config`
structure. This reduces our reliance on global variables and further
unifies the way we parse the configuration in git-fetch(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b40df7e7ca..29b36da18a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -87,7 +87,6 @@ static int verbosity, deepen_relative, set_upstream, refetch;
 static int progress = -1;
 static int tags = TAGS_DEFAULT, update_shallow, deepen;
 static int submodule_fetch_jobs_config = -1;
-static int fetch_parallel_config = 1;
 static int atomic_fetch;
 static enum transport_family family;
 static const char *depth;
@@ -108,6 +107,7 @@ struct fetch_config {
 	int prune_tags;
 	int show_forced_updates;
 	int recurse_submodules;
+	int parallel;
 };
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
@@ -144,11 +144,11 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	}
 
 	if (!strcmp(k, "fetch.parallel")) {
-		fetch_parallel_config = git_config_int(k, v);
-		if (fetch_parallel_config < 0)
+		fetch_config->parallel = git_config_int(k, v);
+		if (fetch_config->parallel < 0)
 			die(_("fetch.parallel cannot be negative"));
-		if (!fetch_parallel_config)
-			fetch_parallel_config = online_cpus();
+		if (!fetch_config->parallel)
+			fetch_config->parallel = online_cpus();
 		return 0;
 	}
 
@@ -2118,6 +2118,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		.prune_tags = -1,
 		.show_forced_updates = 1,
 		.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
+		.parallel = 1,
 	};
 	const char *submodule_prefix = "";
 	const char *bundle_uri;
@@ -2411,7 +2412,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			      "from one remote"));
 
 		if (max_children < 0)
-			max_children = fetch_parallel_config;
+			max_children = config.parallel;
 
 		/* TODO should this also die if we have a previous partial-clone? */
 		result = fetch_multiple(&list, max_children, &config);
@@ -2433,7 +2434,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		if (max_children < 0)
 			max_children = submodule_fetch_jobs_config;
 		if (max_children < 0)
-			max_children = fetch_parallel_config;
+			max_children = config.parallel;
 
 		add_options_to_argv(&options, &config);
 		result = fetch_submodules(the_repository,
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 9/9] fetch: use `fetch_config` to store "submodule.fetchJobs" value

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2564 bytes --]

Move the parsed "submodule.fetchJobs" config value into the
`fetch_config` structure. This reduces our reliance on global variables
and further unifies the way we parse the configuration in git-fetch(1).

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 29b36da18a..e3871048cf 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -86,7 +86,6 @@ static int write_fetch_head = 1;
 static int verbosity, deepen_relative, set_upstream, refetch;
 static int progress = -1;
 static int tags = TAGS_DEFAULT, update_shallow, deepen;
-static int submodule_fetch_jobs_config = -1;
 static int atomic_fetch;
 static enum transport_family family;
 static const char *depth;
@@ -108,6 +107,7 @@ struct fetch_config {
 	int show_forced_updates;
 	int recurse_submodules;
 	int parallel;
+	int submodule_fetch_jobs;
 };
 
 static int git_fetch_config(const char *k, const char *v, void *cb)
@@ -136,7 +136,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	}
 
 	if (!strcmp(k, "submodule.fetchjobs")) {
-		submodule_fetch_jobs_config = parse_submodule_fetchjobs(k, v);
+		fetch_config->submodule_fetch_jobs = parse_submodule_fetchjobs(k, v);
 		return 0;
 	} else if (!strcmp(k, "fetch.recursesubmodules")) {
 		fetch_config->recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
@@ -2119,6 +2119,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		.show_forced_updates = 1,
 		.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
 		.parallel = 1,
+		.submodule_fetch_jobs = -1,
 	};
 	const char *submodule_prefix = "";
 	const char *bundle_uri;
@@ -2268,8 +2269,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	}
 
 	if (config.recurse_submodules != RECURSE_SUBMODULES_OFF) {
-		int *sfjc = submodule_fetch_jobs_config == -1
-			    ? &submodule_fetch_jobs_config : NULL;
+		int *sfjc = config.submodule_fetch_jobs == -1
+			    ? &config.submodule_fetch_jobs : NULL;
 		int *rs = config.recurse_submodules == RECURSE_SUBMODULES_DEFAULT
 			  ? &config.recurse_submodules : NULL;
 
@@ -2432,7 +2433,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		int max_children = max_jobs;
 
 		if (max_children < 0)
-			max_children = submodule_fetch_jobs_config;
+			max_children = config.submodule_fetch_jobs;
 		if (max_children < 0)
 			max_children = config.parallel;
 
-- 
2.40.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value

[Jeff King]

On Wed, May 17, 2023 at 01:48:42PM +0200, Patrick Steinhardt wrote:

> With 50957937f9 (fetch: introduce `display_format` enum, 2023-05-10), a
> new enumeration was introduced to determine the display format that is
> to be used by git-fetch(1). The `DISPLAY_FORMAT_UNKNOWN` value isn't
> ever used though, and neither do we rely on the explicit `0` value for
> initialization anywhere.

To be slightly pedantic, we'd also want to make sure the we do not rely
on the zero value for reading, like:

  if (display_state->format)
     ....

But having looked over the code, we don't (it's always a switch or
equality with a known name), so this is safe to do.

Thanks for cleaning this up.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 849a9be421..9147b700e5 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -58,7 +58,6 @@ enum {
>  };
>  
>  enum display_format {
> -	DISPLAY_FORMAT_UNKNOWN = 0,
>  	DISPLAY_FORMAT_FULL,
>  	DISPLAY_FORMAT_COMPACT,
>  	DISPLAY_FORMAT_PORCELAIN,

Just for similar situations in the future, I think we could do:

  DISPLAY_FORMAT_FULL = 1,

if we were worried about keeping the zero-behavior the same for existing
callers. But given how new and how limited this code is, I feel
confident that we've checked all of the code, and what you've written
above is preferable.

-Peff

Re: [PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref`

[Jeff King]

On Wed, May 17, 2023 at 01:48:46PM +0200, Patrick Steinhardt wrote:

> Drop the `NULL` check for `remote_ref` in `update_local_ref()`. The
> function only has a single caller, and that caller always passes in a
> non-`NULL` value.
> 
> This fixes a false positive in Coverity.

Thanks for following up on this. I was thinking of just reposting it,
thinking there weren't that many other cleanups to do in fetch.c. But
you found quite a few. :)

The patch looks obviously good to me.

-Peff

Re: [PATCH 3/9] fetch: pass through `fetch_config` directly

[Jeff King]

On Wed, May 17, 2023 at 01:48:51PM +0200, Patrick Steinhardt wrote:

> The `fetch_config` structure currently only has a single member, which
> is the `display_format`. We're about extend it to contain all parsed
> config values and will thus need it available in most of the code.
> 
> Prepare for this change by passing through the `fetch_config` directly
> instead of only passing its single member.

Makes sense.

One small nit:

>  static int do_fetch(struct transport *transport,
>  		    struct refspec *rs,
> -		    enum display_format display_format)
> +		    const struct fetch_config *config)
>  {
>  	struct ref_transaction *transaction = NULL;
>  	struct ref *ref_map = NULL;
> @@ -1639,7 +1639,8 @@ static int do_fetch(struct transport *transport,
>  	if (retcode)
>  		goto cleanup;
>  
> -	display_state_init(&display_state, ref_map, transport->url, display_format);
> +	display_state_init(&display_state, ref_map, transport->url,
> +			   config->display_format);

If the point is that fetch_config may start carrying new information,
wouldn't we want to pass it as a whole down to display_state_init()? It
might eventually want to see some of that other config, too.

It's presumably academic for now, and it would not be too hard to change
later if needed, so I don't know that it's worth a re-roll. I just found
it especially funny here since the purpose of the patch is to treat the
config struct as a single unit.

-Peff

Re: [PATCH 4/9] fetch: use `fetch_config` to store "fetch.prune" value

[Jeff King]

On Wed, May 17, 2023 at 01:48:56PM +0200, Patrick Steinhardt wrote:

> Move the parsed "fetch.prune" config value into the `fetch_config`
> structure. This reduces our reliance on global variables and further
> unifies the way we parse the configuration in git-fetch(1).

Makes sense, and I like the end result. Since the other patches in this
series are all variants of this, I won't bother to respond individually
to each one (though I think you were right to split them out). They all
look good to me.

-Peff

Re: [PATCH 3/9] fetch: pass through `fetch_config` directly

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

On Thu, May 18, 2023 at 08:18:03PM -0400, Jeff King wrote:
> On Wed, May 17, 2023 at 01:48:51PM +0200, Patrick Steinhardt wrote:
> 
> > The `fetch_config` structure currently only has a single member, which
> > is the `display_format`. We're about extend it to contain all parsed
> > config values and will thus need it available in most of the code.
> > 
> > Prepare for this change by passing through the `fetch_config` directly
> > instead of only passing its single member.
> 
> Makes sense.
> 
> One small nit:
> 
> >  static int do_fetch(struct transport *transport,
> >  		    struct refspec *rs,
> > -		    enum display_format display_format)
> > +		    const struct fetch_config *config)
> >  {
> >  	struct ref_transaction *transaction = NULL;
> >  	struct ref *ref_map = NULL;
> > @@ -1639,7 +1639,8 @@ static int do_fetch(struct transport *transport,
> >  	if (retcode)
> >  		goto cleanup;
> >  
> > -	display_state_init(&display_state, ref_map, transport->url, display_format);
> > +	display_state_init(&display_state, ref_map, transport->url,
> > +			   config->display_format);
> 
> If the point is that fetch_config may start carrying new information,
> wouldn't we want to pass it as a whole down to display_state_init()? It
> might eventually want to see some of that other config, too.
> 
> It's presumably academic for now, and it would not be too hard to change
> later if needed, so I don't know that it's worth a re-roll. I just found
> it especially funny here since the purpose of the patch is to treat the
> config struct as a single unit.

Well, I decided against passing in the full configuration as it feels a
bit like a layering violation: the other code really is about the fetch
itself, while this code here is only about display logic. So passing in
the `fetch_config` felt weird to me, even more so because we continue to
only need that single value at the end of this series. I do see your
point though.

Given that none of your other comments require a reroll I'll leave this
as-is for now. Thanks for your review!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/9] fetch: pass through `fetch_config` directly

[Jeff King]

On Mon, May 22, 2023 at 10:58:51AM +0200, Patrick Steinhardt wrote:

> > If the point is that fetch_config may start carrying new information,
> > wouldn't we want to pass it as a whole down to display_state_init()? It
> > might eventually want to see some of that other config, too.
> > 
> > It's presumably academic for now, and it would not be too hard to change
> > later if needed, so I don't know that it's worth a re-roll. I just found
> > it especially funny here since the purpose of the patch is to treat the
> > config struct as a single unit.
> 
> Well, I decided against passing in the full configuration as it feels a
> bit like a layering violation: the other code really is about the fetch
> itself, while this code here is only about display logic. So passing in
> the `fetch_config` felt weird to me, even more so because we continue to
> only need that single value at the end of this series. I do see your
> point though.
> 
> Given that none of your other comments require a reroll I'll leave this
> as-is for now. Thanks for your review!

Yeah, I could see that line of thinking, as well. Leaving sounds good to
me. And I think that was my only substantive comment on the whole
series, so we can consider the whole reviewed-by: me.

-Peff