Skip to contentSkip to conversation

fetch: smallish cleanups

PATCH In review Patrick Steinhardt 2 participants 16 messages May 17, 2023 – May 22, 2023

lore
Share & tools
lore
author
#1 lore raw

PATCH 1/9 fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value

In reply to Patrick Steinhardt

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 <[email protected]>
---
 builtin/fetch.c | 1 -
 1 file changed, 1 deletion(-)
builtin/fetch.c +0 -1
index 849a9be421..9147b700e5 100644
@@ -58,7 +58,6 @@ enum {
 };
 
 enum display_format {
-	DISPLAY_FORMAT_UNKNOWN = 0,
 	DISPLAY_FORMAT_FULL,
 	DISPLAY_FORMAT_COMPACT,
 	DISPLAY_FORMAT_PORCELAIN,
Signature · 1 line
2.40.1

Patrick Steinhardt

author
#0 lore raw

cover fetch: smallish cleanups

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(-)
Signature · 1 line
2.40.1
author
#3 lore raw

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

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 <[email protected]>
---
 builtin/fetch.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
builtin/fetch.c +2 -3
index 9147b700e5..f268322e6f 100644
@@ -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 {
Signature · 1 line
2.40.1
author
#5 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>
---
 builtin/fetch.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)
builtin/fetch.c +16 -15
index f268322e6f..401543e05d 100644
@@ -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,
Signature · 1 line
2.40.1
author
#9 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>
---
 builtin/fetch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
builtin/fetch.c +5 -4
index 401543e05d..488705b519 100644
@@ -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;
Signature · 1 line
2.40.1
author
#11 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>
---
 builtin/fetch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
builtin/fetch.c +5 -4
index 488705b519..94718bcb2a 100644
@@ -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;
Signature · 1 line
2.40.1
author
#12 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>
---
 builtin/fetch.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)
builtin/fetch.c +21 -14
index 94718bcb2a..eec04d7660 100644
@@ -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")),
Signature · 1 line
2.40.1
author
#13 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>
---
 builtin/fetch.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)
builtin/fetch.c +16 -15
index eec04d7660..b40df7e7ca 100644
@@ -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);
Signature · 1 line
2.40.1
author
#14 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>
---
 builtin/fetch.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
builtin/fetch.c +8 -7
index b40df7e7ca..29b36da18a 100644
@@ -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,
Signature · 1 line
2.40.1
author
#15 lore raw

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

In reply to Patrick Steinhardt

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 <[email protected]>
---
 builtin/fetch.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
builtin/fetch.c +6 -5
index 29b36da18a..e3871048cf 100644
@@ -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;
Signature · 1 line
2.40.1
#2 lore raw

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

In reply to Patrick Steinhardt
Quoted 5 lines · Patrick Steinhardt

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.

builtin/fetch.c enum { · line 63

Quoted 12 lines
builtin/fetch.c +0 -1
index 849a9be421..9147b700e5 100644
@@ -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

#4 lore raw

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

In reply to Patrick Steinhardt
Quoted 5 lines · Patrick Steinhardt

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

#6 lore raw

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

In reply to Patrick Steinhardt
Quoted 6 lines · Patrick Steinhardt

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:

Quoted 8 lines

static int do_fetch(struct transport *transport,

builtin/fetch.c +1 -1
 		    struct refspec *rs,
-		    enum display_format display_format)
+		    const struct fetch_config *config)
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;

builtin/fetch.c static int do_fetch(struct transport *transport, · line 1643

Quoted 6 lines
builtin/fetch.c +2 -1
@@ -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

#10 lore raw

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

In reply to Patrick Steinhardt
Quoted 3 lines · Patrick Steinhardt

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

author
#7 lore raw

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

In reply to Jeff King
Quoted 32 lines · Jeff King
Quoted 6 lines · Patrick Steinhardt

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:

Quoted 8 lines

static int do_fetch(struct transport *transport,

builtin/fetch.c +1 -1
 		    struct refspec *rs,
-		    enum display_format display_format)
+		    const struct fetch_config *config)
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;

builtin/fetch.c static int do_fetch(struct transport *transport, · line 1643

Quoted 6 lines
builtin/fetch.c +2 -1
@@ -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.

Quoted 4 lines

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

#8 lore raw

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

Quoted 16 lines · Patrick Steinhardt
Quoted 8 lines

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.

Quoted 2 lines

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