* [PATCH 0/9] fetch: smallish cleanups
@ 2023-05-17 11:48 Patrick Steinhardt
2023-05-17 11:48 ` [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value Patrick Steinhardt
` (8 more replies)
0 siblings, 9 replies; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:48 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
@ 2023-05-17 11:48 ` Patrick Steinhardt
2023-05-19 0:13 ` Jeff King
2023-05-17 11:48 ` [PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref` Patrick Steinhardt
` (7 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:48 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref`
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
2023-05-17 11:48 ` [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value Patrick Steinhardt
@ 2023-05-17 11:48 ` Patrick Steinhardt
2023-05-19 0:13 ` Jeff King
2023-05-17 11:48 ` [PATCH 3/9] fetch: pass through `fetch_config` directly Patrick Steinhardt
` (6 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:48 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/9] fetch: pass through `fetch_config` directly
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
2023-05-17 11:48 ` [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value Patrick Steinhardt
2023-05-17 11:48 ` [PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref` Patrick Steinhardt
@ 2023-05-17 11:48 ` Patrick Steinhardt
2023-05-19 0:18 ` Jeff King
2023-05-17 11:48 ` [PATCH 4/9] fetch: use `fetch_config` to store "fetch.prune" value Patrick Steinhardt
` (5 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:48 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/9] fetch: use `fetch_config` to store "fetch.prune" value
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
` (2 preceding siblings ...)
2023-05-17 11:48 ` [PATCH 3/9] fetch: pass through `fetch_config` directly Patrick Steinhardt
@ 2023-05-17 11:48 ` Patrick Steinhardt
2023-05-19 0:21 ` Jeff King
2023-05-17 11:49 ` [PATCH 5/9] fetch: use `fetch_config` to store "fetch.pruneTags" value Patrick Steinhardt
` (4 subsequent siblings)
8 siblings, 1 reply; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:48 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 5/9] fetch: use `fetch_config` to store "fetch.pruneTags" value
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
` (3 preceding siblings ...)
2023-05-17 11:48 ` [PATCH 4/9] fetch: use `fetch_config` to store "fetch.prune" value Patrick Steinhardt
@ 2023-05-17 11:49 ` Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 6/9] fetch: use `fetch_config` to store "fetch.showForcedUpdates" value Patrick Steinhardt
` (3 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:49 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 6/9] fetch: use `fetch_config` to store "fetch.showForcedUpdates" value
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
` (4 preceding siblings ...)
2023-05-17 11:49 ` [PATCH 5/9] fetch: use `fetch_config` to store "fetch.pruneTags" value Patrick Steinhardt
@ 2023-05-17 11:49 ` Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 7/9] fetch: use `fetch_config` to store "fetch.recurseSubmodules" value Patrick Steinhardt
` (2 subsequent siblings)
8 siblings, 0 replies; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:49 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 7/9] fetch: use `fetch_config` to store "fetch.recurseSubmodules" value
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
` (5 preceding siblings ...)
2023-05-17 11:49 ` [PATCH 6/9] fetch: use `fetch_config` to store "fetch.showForcedUpdates" value Patrick Steinhardt
@ 2023-05-17 11:49 ` Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 8/9] fetch: use `fetch_config` to store "fetch.parallel" value Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 9/9] fetch: use `fetch_config` to store "submodule.fetchJobs" value Patrick Steinhardt
8 siblings, 0 replies; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:49 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 8/9] fetch: use `fetch_config` to store "fetch.parallel" value
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
` (6 preceding siblings ...)
2023-05-17 11:49 ` [PATCH 7/9] fetch: use `fetch_config` to store "fetch.recurseSubmodules" value Patrick Steinhardt
@ 2023-05-17 11:49 ` Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 9/9] fetch: use `fetch_config` to store "submodule.fetchJobs" value Patrick Steinhardt
8 siblings, 0 replies; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:49 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 9/9] fetch: use `fetch_config` to store "submodule.fetchJobs" value
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
` (7 preceding siblings ...)
2023-05-17 11:49 ` [PATCH 8/9] fetch: use `fetch_config` to store "fetch.parallel" value Patrick Steinhardt
@ 2023-05-17 11:49 ` Patrick Steinhardt
8 siblings, 0 replies; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-17 11:49 UTC (permalink / raw)
To: git
[-- 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 --]
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value
2023-05-17 11:48 ` [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value Patrick Steinhardt
@ 2023-05-19 0:13 ` Jeff King
0 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2023-05-19 0:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref`
2023-05-17 11:48 ` [PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref` Patrick Steinhardt
@ 2023-05-19 0:13 ` Jeff King
0 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2023-05-19 0:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/9] fetch: pass through `fetch_config` directly
2023-05-17 11:48 ` [PATCH 3/9] fetch: pass through `fetch_config` directly Patrick Steinhardt
@ 2023-05-19 0:18 ` Jeff King
2023-05-22 8:58 ` Patrick Steinhardt
0 siblings, 1 reply; 16+ messages in thread
From: Jeff King @ 2023-05-19 0:18 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/9] fetch: use `fetch_config` to store "fetch.prune" value
2023-05-17 11:48 ` [PATCH 4/9] fetch: use `fetch_config` to store "fetch.prune" value Patrick Steinhardt
@ 2023-05-19 0:21 ` Jeff King
0 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2023-05-19 0:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/9] fetch: pass through `fetch_config` directly
2023-05-19 0:18 ` Jeff King
@ 2023-05-22 8:58 ` Patrick Steinhardt
2023-05-22 19:17 ` Jeff King
0 siblings, 1 reply; 16+ messages in thread
From: Patrick Steinhardt @ 2023-05-22 8:58 UTC (permalink / raw)
To: Jeff King; +Cc: git
[-- 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/9] fetch: pass through `fetch_config` directly
2023-05-22 8:58 ` Patrick Steinhardt
@ 2023-05-22 19:17 ` Jeff King
0 siblings, 0 replies; 16+ messages in thread
From: Jeff King @ 2023-05-22 19:17 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-05-22 19:17 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-17 11:48 [PATCH 0/9] fetch: smallish cleanups Patrick Steinhardt
2023-05-17 11:48 ` [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value Patrick Steinhardt
2023-05-19 0:13 ` Jeff King
2023-05-17 11:48 ` [PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref` Patrick Steinhardt
2023-05-19 0:13 ` Jeff King
2023-05-17 11:48 ` [PATCH 3/9] fetch: pass through `fetch_config` directly Patrick Steinhardt
2023-05-19 0:18 ` Jeff King
2023-05-22 8:58 ` Patrick Steinhardt
2023-05-22 19:17 ` Jeff King
2023-05-17 11:48 ` [PATCH 4/9] fetch: use `fetch_config` to store "fetch.prune" value Patrick Steinhardt
2023-05-19 0:21 ` Jeff King
2023-05-17 11:49 ` [PATCH 5/9] fetch: use `fetch_config` to store "fetch.pruneTags" value Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 6/9] fetch: use `fetch_config` to store "fetch.showForcedUpdates" value Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 7/9] fetch: use `fetch_config` to store "fetch.recurseSubmodules" value Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 8/9] fetch: use `fetch_config` to store "fetch.parallel" value Patrick Steinhardt
2023-05-17 11:49 ` [PATCH 9/9] fetch: use `fetch_config` to store "submodule.fetchJobs" value Patrick Steinhardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).