{"thread":{"id":"59757","subject":"[PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value","startedAt":"2023-05-17T11:48:49Z","lastAt":"2023-05-22T19:17:41Z","messageCount":16,"participants":["Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"477436","messageId":"9417a62e9bbd30d9350b65e5396a31f9cb7c60b6.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:48:42Z","receivedAt":"2023-05-17T11:48:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With 50957937f9 (fetch: introduce `display_format` enum, 2023-05-10), a\nnew enumeration was introduced to determine the display format that is\nto be used by git-fetch(1). The `DISPLAY_FORMAT_UNKNOWN` value isn't\never used though, and neither do we rely on the explicit `0` value for\ninitialization anywhere.\n\nRemove the enum value.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 849a9be421..9147b700e5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -58,7 +58,6 @@ enum {\n };\n \n enum display_format {\n-\tDISPLAY_FORMAT_UNKNOWN = 0,\n \tDISPLAY_FORMAT_FULL,\n \tDISPLAY_FORMAT_COMPACT,\n \tDISPLAY_FORMAT_PORCELAIN,\n-- \n2.40.1\n\n"},{"id":"477437","messageId":"cover.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":null,"subject":"[PATCH 0/9] fetch: smallish cleanups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:48:38Z","receivedAt":"2023-05-17T11:48:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthe dust around machine-parsable fetches has settled a bit, so as\npromised this patch series contains a smallish set of cleanups for\ngit-fetch(1).\n\n- Patch 1/9 drops the unused `DISPLAY_FORMAT_UNKNOWN` enum.\n\n- Patch 2/9 drops a useless `NULL` check as pointed out by Peff.\n\n- The remaining patches convert `git_fetch_config()` to not use global\n  state anymore, but instead to parse all config values into the newly\n  introduced `fetch_config` structure.\n\nThe patches depend on 15ba44f1b4 (Merge branch 'ps/fetch-output-format',\n2023-05-15). As this is the last merge before v2.41.0-rc0 I've decided\nto thus base this series on top of that tag.\n\nPatrick\n\nPatrick Steinhardt (9):\n  fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value\n  fetch: drop unneeded NULL-check for `remote_ref`\n  fetch: pass through `fetch_config` directly\n  fetch: use `fetch_config` to store \"fetch.prune\" value\n  fetch: use `fetch_config` to store \"fetch.pruneTags\" value\n  fetch: use `fetch_config` to store \"fetch.showForcedUpdates\" value\n  fetch: use `fetch_config` to store \"fetch.recurseSubmodules\" value\n  fetch: use `fetch_config` to store \"fetch.parallel\" value\n  fetch: use `fetch_config` to store \"submodule.fetchJobs\" value\n\n builtin/fetch.c | 147 ++++++++++++++++++++++++++----------------------\n 1 file changed, 79 insertions(+), 68 deletions(-)\n\n-- \n2.40.1\n\n"},{"id":"477438","messageId":"7197540df60d34e8f727e29d300dd0c96e31d310.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:48:46Z","receivedAt":"2023-05-17T11:49:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Drop the `NULL` check for `remote_ref` in `update_local_ref()`. The\nfunction only has a single caller, and that caller always passes in a\nnon-`NULL` value.\n\nThis fixes a false positive in Coverity.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9147b700e5..f268322e6f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -953,11 +953,10 @@ static int update_local_ref(struct ref *ref,\n \t\t * Base this on the remote's ref name, as it's\n \t\t * more likely to follow a standard layout.\n \t\t */\n-\t\tconst char *name = remote_ref ? remote_ref->name : \"\";\n-\t\tif (starts_with(name, \"refs/tags/\")) {\n+\t\tif (starts_with(remote_ref->name, \"refs/tags/\")) {\n \t\t\tmsg = \"storing tag\";\n \t\t\twhat = _(\"[new tag]\");\n-\t\t} else if (starts_with(name, \"refs/heads/\")) {\n+\t\t} else if (starts_with(remote_ref->name, \"refs/heads/\")) {\n \t\t\tmsg = \"storing head\";\n \t\t\twhat = _(\"[new branch]\");\n \t\t} else {\n-- \n2.40.1\n\n"},{"id":"477439","messageId":"b09f21dff786780dfb0ae71efd9a0cabe31c1e0b.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 3/9] fetch: pass through `fetch_config` directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:48:51Z","receivedAt":"2023-05-17T11:49:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `fetch_config` structure currently only has a single member, which\nis the `display_format`. We're about extend it to contain all parsed\nconfig values and will thus need it available in most of the code.\n\nPrepare for this change by passing through the `fetch_config` directly\ninstead of only passing its single member.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 31 ++++++++++++++++---------------\n 1 file changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f268322e6f..401543e05d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1553,7 +1553,7 @@ static int backfill_tags(struct display_state *display_state,\n \n static int do_fetch(struct transport *transport,\n \t\t    struct refspec *rs,\n-\t\t    enum display_format display_format)\n+\t\t    const struct fetch_config *config)\n {\n \tstruct ref_transaction *transaction = NULL;\n \tstruct ref *ref_map = NULL;\n@@ -1639,7 +1639,8 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tdisplay_state_init(&display_state, ref_map, transport->url, display_format);\n+\tdisplay_state_init(&display_state, ref_map, transport->url,\n+\t\t\t   config->display_format);\n \n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n@@ -1828,7 +1829,7 @@ static int add_remote_or_group(const char *name, struct string_list *list)\n }\n \n static void add_options_to_argv(struct strvec *argv,\n-\t\t\t\tenum display_format format)\n+\t\t\t\tconst struct fetch_config *config)\n {\n \tif (dry_run)\n \t\tstrvec_push(argv, \"--dry-run\");\n@@ -1864,7 +1865,7 @@ static void add_options_to_argv(struct strvec *argv,\n \t\tstrvec_push(argv, \"--ipv6\");\n \tif (!write_fetch_head)\n \t\tstrvec_push(argv, \"--no-write-fetch-head\");\n-\tif (format == DISPLAY_FORMAT_PORCELAIN)\n+\tif (config->display_format == DISPLAY_FORMAT_PORCELAIN)\n \t\tstrvec_pushf(argv, \"--porcelain\");\n }\n \n@@ -1874,7 +1875,7 @@ struct parallel_fetch_state {\n \tconst char **argv;\n \tstruct string_list *remotes;\n \tint next, result;\n-\tenum display_format format;\n+\tconst struct fetch_config *config;\n };\n \n static int fetch_next_remote(struct child_process *cp,\n@@ -1894,7 +1895,7 @@ static int fetch_next_remote(struct child_process *cp,\n \tstrvec_push(&cp->args, remote);\n \tcp->git_cmd = 1;\n \n-\tif (verbosity >= 0 && state->format != DISPLAY_FORMAT_PORCELAIN)\n+\tif (verbosity >= 0 && state->config->display_format != DISPLAY_FORMAT_PORCELAIN)\n \t\tprintf(_(\"Fetching %s\\n\"), remote);\n \n \treturn 1;\n@@ -1927,7 +1928,7 @@ static int fetch_finished(int result, struct strbuf *out,\n }\n \n static int fetch_multiple(struct string_list *list, int max_children,\n-\t\t\t  enum display_format format)\n+\t\t\t  const struct fetch_config *config)\n {\n \tint i, result = 0;\n \tstruct strvec argv = STRVEC_INIT;\n@@ -1945,10 +1946,10 @@ static int fetch_multiple(struct string_list *list, int max_children,\n \tstrvec_pushl(&argv, \"-c\", \"fetch.bundleURI=\",\n \t\t     \"fetch\", \"--append\", \"--no-auto-gc\",\n \t\t     \"--no-write-commit-graph\", NULL);\n-\tadd_options_to_argv(&argv, format);\n+\tadd_options_to_argv(&argv, config);\n \n \tif (max_children != 1 && list->nr != 1) {\n-\t\tstruct parallel_fetch_state state = { argv.v, list, 0, 0, format };\n+\t\tstruct parallel_fetch_state state = { argv.v, list, 0, 0, config };\n \t\tconst struct run_process_parallel_opts opts = {\n \t\t\t.tr2_category = \"fetch\",\n \t\t\t.tr2_label = \"parallel/fetch\",\n@@ -1972,7 +1973,7 @@ static int fetch_multiple(struct string_list *list, int max_children,\n \n \t\t\tstrvec_pushv(&cmd.args, argv.v);\n \t\t\tstrvec_push(&cmd.args, name);\n-\t\t\tif (verbosity >= 0 && format != DISPLAY_FORMAT_PORCELAIN)\n+\t\t\tif (verbosity >= 0 && config->display_format != DISPLAY_FORMAT_PORCELAIN)\n \t\t\t\tprintf(_(\"Fetching %s\\n\"), name);\n \t\t\tcmd.git_cmd = 1;\n \t\t\tif (run_command(&cmd)) {\n@@ -2028,7 +2029,7 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \n static int fetch_one(struct remote *remote, int argc, const char **argv,\n \t\t     int prune_tags_ok, int use_stdin_refspecs,\n-\t\t     enum display_format display_format)\n+\t\t     const struct fetch_config *config)\n {\n \tstruct refspec rs = REFSPEC_INIT_FETCH;\n \tint i;\n@@ -2095,7 +2096,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n \tsigchain_push_common(unlock_pack_on_signal);\n \tatexit(unlock_pack_atexit);\n \tsigchain_push(SIGPIPE, SIG_IGN);\n-\texit_code = do_fetch(gtransport, &rs, display_format);\n+\texit_code = do_fetch(gtransport, &rs, config);\n \tsigchain_pop(SIGPIPE);\n \trefspec_clear(&rs);\n \ttransport_disconnect(gtransport);\n@@ -2383,7 +2384,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tif (filter_options.choice || repo_has_promisor_remote(the_repository))\n \t\t\tfetch_one_setup_partial(remote);\n \t\tresult = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,\n-\t\t\t\t   config.display_format);\n+\t\t\t\t   &config);\n \t} else {\n \t\tint max_children = max_jobs;\n \n@@ -2403,7 +2404,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tmax_children = fetch_parallel_config;\n \n \t\t/* TODO should this also die if we have a previous partial-clone? */\n-\t\tresult = fetch_multiple(&list, max_children, config.display_format);\n+\t\tresult = fetch_multiple(&list, max_children, &config);\n \t}\n \n \t/*\n@@ -2424,7 +2425,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tif (max_children < 0)\n \t\t\tmax_children = fetch_parallel_config;\n \n-\t\tadd_options_to_argv(&options, config.display_format);\n+\t\tadd_options_to_argv(&options, &config);\n \t\tresult = fetch_submodules(the_repository,\n \t\t\t\t\t  &options,\n \t\t\t\t\t  submodule_prefix,\n-- \n2.40.1\n\n"},{"id":"477440","messageId":"1fa4922bea12ef0363e2638bbb5c70218c6ec357.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 4/9] fetch: use `fetch_config` to store \"fetch.prune\" value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:48:56Z","receivedAt":"2023-05-17T11:49:14Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move the parsed \"fetch.prune\" config value into the `fetch_config`\nstructure. This reduces our reliance on global variables and further\nunifies the way we parse the configuration in git-fetch(1).\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 401543e05d..488705b519 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -73,7 +73,6 @@ struct display_state {\n \tint url_len, shown_url;\n };\n \n-static int fetch_prune_config = -1; /* unspecified */\n static int fetch_show_forced_updates = 1;\n static uint64_t forced_updates_ms = 0;\n static int prefetch = 0;\n@@ -108,6 +107,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n \n struct fetch_config {\n \tenum display_format display_format;\n+\tint prune;\n };\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n@@ -115,7 +115,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \tstruct fetch_config *fetch_config = cb;\n \n \tif (!strcmp(k, \"fetch.prune\")) {\n-\t\tfetch_prune_config = git_config_bool(k, v);\n+\t\tfetch_config->prune = git_config_bool(k, v);\n \t\treturn 0;\n \t}\n \n@@ -2047,8 +2047,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n \t\t/* no command line request */\n \t\tif (0 <= remote->prune)\n \t\t\tprune = remote->prune;\n-\t\telse if (0 <= fetch_prune_config)\n-\t\t\tprune = fetch_prune_config;\n+\t\telse if (0 <= config->prune)\n+\t\t\tprune = config->prune;\n \t\telse\n \t\t\tprune = PRUNE_BY_DEFAULT;\n \t}\n@@ -2108,6 +2108,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n {\n \tstruct fetch_config config = {\n \t\t.display_format = DISPLAY_FORMAT_FULL,\n+\t\t.prune = -1,\n \t};\n \tconst char *submodule_prefix = \"\";\n \tconst char *bundle_uri;\n-- \n2.40.1\n\n"},{"id":"477441","messageId":"3aa64ca1941cb8204448a48e1d44b9563c002d19.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 5/9] fetch: use `fetch_config` to store \"fetch.pruneTags\" value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:49:00Z","receivedAt":"2023-05-17T11:49:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move the parsed \"fetch.pruneTags\" config value into the `fetch_config`\nstructure. This reduces our reliance on global variables and further\nunifies the way we parse the configuration in git-fetch(1).\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 488705b519..94718bcb2a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -79,7 +79,6 @@ static int prefetch = 0;\n static int prune = -1; /* unspecified */\n #define PRUNE_BY_DEFAULT 0 /* do we prune by default? */\n \n-static int fetch_prune_tags_config = -1; /* unspecified */\n static int prune_tags = -1; /* unspecified */\n #define PRUNE_TAGS_BY_DEFAULT 0 /* do we prune tags by default? */\n \n@@ -108,6 +107,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n struct fetch_config {\n \tenum display_format display_format;\n \tint prune;\n+\tint prune_tags;\n };\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n@@ -120,7 +120,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t}\n \n \tif (!strcmp(k, \"fetch.prunetags\")) {\n-\t\tfetch_prune_tags_config = git_config_bool(k, v);\n+\t\tfetch_config->prune_tags = git_config_bool(k, v);\n \t\treturn 0;\n \t}\n \n@@ -2057,8 +2057,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,\n \t\t/* no command line request */\n \t\tif (0 <= remote->prune_tags)\n \t\t\tprune_tags = remote->prune_tags;\n-\t\telse if (0 <= fetch_prune_tags_config)\n-\t\t\tprune_tags = fetch_prune_tags_config;\n+\t\telse if (0 <= config->prune_tags)\n+\t\t\tprune_tags = config->prune_tags;\n \t\telse\n \t\t\tprune_tags = PRUNE_TAGS_BY_DEFAULT;\n \t}\n@@ -2109,6 +2109,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tstruct fetch_config config = {\n \t\t.display_format = DISPLAY_FORMAT_FULL,\n \t\t.prune = -1,\n+\t\t.prune_tags = -1,\n \t};\n \tconst char *submodule_prefix = \"\";\n \tconst char *bundle_uri;\n-- \n2.40.1\n\n"},{"id":"477442","messageId":"5e842c1327efa73cac4f51160d70dd16ba09bfb3.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 6/9] fetch: use `fetch_config` to store \"fetch.showForcedUpdates\" value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:49:04Z","receivedAt":"2023-05-17T11:49:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move the parsed \"fetch.showForcedUpdaets\" config value into the\n`fetch_config` structure. This reduces our reliance on global variables\nand further unifies the way we parse the configuration in git-fetch(1).\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 35 +++++++++++++++++++++--------------\n 1 file changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 94718bcb2a..eec04d7660 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -73,7 +73,6 @@ struct display_state {\n \tint url_len, shown_url;\n };\n \n-static int fetch_show_forced_updates = 1;\n static uint64_t forced_updates_ms = 0;\n static int prefetch = 0;\n static int prune = -1; /* unspecified */\n@@ -108,6 +107,7 @@ struct fetch_config {\n \tenum display_format display_format;\n \tint prune;\n \tint prune_tags;\n+\tint show_forced_updates;\n };\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n@@ -125,7 +125,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t}\n \n \tif (!strcmp(k, \"fetch.showforcedupdates\")) {\n-\t\tfetch_show_forced_updates = git_config_bool(k, v);\n+\t\tfetch_config->show_forced_updates = git_config_bool(k, v);\n \t\treturn 0;\n \t}\n \n@@ -891,7 +891,8 @@ static int update_local_ref(struct ref *ref,\n \t\t\t    struct ref_transaction *transaction,\n \t\t\t    struct display_state *display_state,\n \t\t\t    const struct ref *remote_ref,\n-\t\t\t    int summary_width)\n+\t\t\t    int summary_width,\n+\t\t\t    const struct fetch_config *config)\n {\n \tstruct commit *current = NULL, *updated;\n \tint fast_forward = 0;\n@@ -972,7 +973,7 @@ static int update_local_ref(struct ref *ref,\n \t\treturn r;\n \t}\n \n-\tif (fetch_show_forced_updates) {\n+\tif (config->show_forced_updates) {\n \t\tuint64_t t_before = getnanotime();\n \t\tfast_forward = repo_in_merge_bases(the_repository, current,\n \t\t\t\t\t\t   updated);\n@@ -1125,7 +1126,8 @@ static int store_updated_refs(struct display_state *display_state,\n \t\t\t      const char *remote_name,\n \t\t\t      int connectivity_checked,\n \t\t\t      struct ref_transaction *transaction, struct ref *ref_map,\n-\t\t\t      struct fetch_head *fetch_head)\n+\t\t\t      struct fetch_head *fetch_head,\n+\t\t\t      const struct fetch_config *config)\n {\n \tint rc = 0;\n \tstruct strbuf note = STRBUF_INIT;\n@@ -1241,7 +1243,7 @@ static int store_updated_refs(struct display_state *display_state,\n \n \t\t\tif (ref) {\n \t\t\t\trc |= update_local_ref(ref, transaction, display_state,\n-\t\t\t\t\t\t       rm, summary_width);\n+\t\t\t\t\t\t       rm, summary_width, config);\n \t\t\t\tfree(ref);\n \t\t\t} else if (write_fetch_head || dry_run) {\n \t\t\t\t/*\n@@ -1265,7 +1267,7 @@ static int store_updated_refs(struct display_state *display_state,\n \t\t      \"branches\"), remote_name);\n \n \tif (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {\n-\t\tif (!fetch_show_forced_updates) {\n+\t\tif (!config->show_forced_updates) {\n \t\t\twarning(_(warn_show_forced_updates));\n \t\t} else if (forced_updates_ms > FORCED_UPDATES_DELAY_WARNING_IN_MS) {\n \t\t\twarning(_(warn_time_show_forced_updates),\n@@ -1326,7 +1328,8 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n \t\t\t\t  struct transport *transport,\n \t\t\t\t  struct ref_transaction *transaction,\n \t\t\t\t  struct ref *ref_map,\n-\t\t\t\t  struct fetch_head *fetch_head)\n+\t\t\t\t  struct fetch_head *fetch_head,\n+\t\t\t\t  const struct fetch_config *config)\n {\n \tint connectivity_checked = 1;\n \tint ret;\n@@ -1349,7 +1352,7 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(display_state, transport->remote->name,\n \t\t\t\t connectivity_checked, transaction, ref_map,\n-\t\t\t\t fetch_head);\n+\t\t\t\t fetch_head, config);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n \n out:\n@@ -1520,7 +1523,8 @@ static int backfill_tags(struct display_state *display_state,\n \t\t\t struct transport *transport,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t struct ref *ref_map,\n-\t\t\t struct fetch_head *fetch_head)\n+\t\t\t struct fetch_head *fetch_head,\n+\t\t\t const struct fetch_config *config)\n {\n \tint retcode, cannot_reuse;\n \n@@ -1541,7 +1545,8 @@ static int backfill_tags(struct display_state *display_state,\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tretcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map, fetch_head);\n+\tretcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,\n+\t\t\t\t\t fetch_head, config);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n@@ -1668,7 +1673,8 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\n \t}\n \n-\tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, &fetch_head)) {\n+\tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n+\t\t\t\t   &fetch_head, config)) {\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n@@ -1691,7 +1697,7 @@ static int do_fetch(struct transport *transport,\n \t\t\t * the transaction and don't commit anything.\n \t\t\t */\n \t\t\tif (backfill_tags(&display_state, transport, transaction, tags_ref_map,\n-\t\t\t\t\t  &fetch_head))\n+\t\t\t\t\t  &fetch_head, config))\n \t\t\t\tretcode = 1;\n \t\t}\n \n@@ -2110,6 +2116,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t.display_format = DISPLAY_FORMAT_FULL,\n \t\t.prune = -1,\n \t\t.prune_tags = -1,\n+\t\t.show_forced_updates = 1,\n \t};\n \tconst char *submodule_prefix = \"\";\n \tconst char *bundle_uri;\n@@ -2207,7 +2214,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"run 'maintenance --auto' after fetching\")),\n \t\tOPT_BOOL(0, \"auto-gc\", &enable_auto_gc,\n \t\t\t N_(\"run 'maintenance --auto' after fetching\")),\n-\t\tOPT_BOOL(0, \"show-forced-updates\", &fetch_show_forced_updates,\n+\t\tOPT_BOOL(0, \"show-forced-updates\", &config.show_forced_updates,\n \t\t\t N_(\"check for forced-updates on all updated branches\")),\n \t\tOPT_BOOL(0, \"write-commit-graph\", &fetch_write_commit_graph,\n \t\t\t N_(\"write the commit-graph after fetching\")),\n-- \n2.40.1\n\n"},{"id":"477443","messageId":"74eee5858d1511e35a0039ef8bd65585676b3f72.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 7/9] fetch: use `fetch_config` to store \"fetch.recurseSubmodules\" value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:49:08Z","receivedAt":"2023-05-17T11:49:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move the parsed \"fetch.recurseSubmodules\" config value into the\n`fetch_config` structure. This reduces our reliance on global variables\nand further unifies the way we parse the configuration in git-fetch(1).\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 31 ++++++++++++++++---------------\n 1 file changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex eec04d7660..b40df7e7ca 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -97,7 +97,6 @@ static struct string_list deepen_not = STRING_LIST_INIT_NODUP;\n static struct strbuf default_rla = STRBUF_INIT;\n static struct transport *gtransport;\n static struct transport *gsecondary;\n-static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static struct refspec refmap = REFSPEC_INIT_FETCH;\n static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n@@ -108,6 +107,7 @@ struct fetch_config {\n \tint prune;\n \tint prune_tags;\n \tint show_forced_updates;\n+\tint recurse_submodules;\n };\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n@@ -132,14 +132,14 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \tif (!strcmp(k, \"submodule.recurse\")) {\n \t\tint r = git_config_bool(k, v) ?\n \t\t\tRECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n-\t\trecurse_submodules = r;\n+\t\tfetch_config->recurse_submodules = r;\n \t}\n \n \tif (!strcmp(k, \"submodule.fetchjobs\")) {\n \t\tsubmodule_fetch_jobs_config = parse_submodule_fetchjobs(k, v);\n \t\treturn 0;\n \t} else if (!strcmp(k, \"fetch.recursesubmodules\")) {\n-\t\trecurse_submodules = parse_fetch_recurse_submodules_arg(k, v);\n+\t\tfetch_config->recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);\n \t\treturn 0;\n \t}\n \n@@ -1210,7 +1210,7 @@ static int store_updated_refs(struct display_state *display_state,\n \t\t\t\tref->force = rm->peer_ref->force;\n \t\t\t}\n \n-\t\t\tif (recurse_submodules != RECURSE_SUBMODULES_OFF &&\n+\t\t\tif (config->recurse_submodules != RECURSE_SUBMODULES_OFF &&\n \t\t\t    (!rm->peer_ref || !oideq(&ref->old_oid, &ref->new_oid))) {\n \t\t\t\tcheck_for_new_submodule_commits(&rm->old_oid);\n \t\t\t}\n@@ -1849,11 +1849,11 @@ static void add_options_to_argv(struct strvec *argv,\n \t\tstrvec_push(argv, \"--force\");\n \tif (keep)\n \t\tstrvec_push(argv, \"--keep\");\n-\tif (recurse_submodules == RECURSE_SUBMODULES_ON)\n+\tif (config->recurse_submodules == RECURSE_SUBMODULES_ON)\n \t\tstrvec_push(argv, \"--recurse-submodules\");\n-\telse if (recurse_submodules == RECURSE_SUBMODULES_OFF)\n+\telse if (config->recurse_submodules == RECURSE_SUBMODULES_OFF)\n \t\tstrvec_push(argv, \"--no-recurse-submodules\");\n-\telse if (recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)\n+\telse if (config->recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)\n \t\tstrvec_push(argv, \"--recurse-submodules=on-demand\");\n \tif (tags == TAGS_SET)\n \t\tstrvec_push(argv, \"--tags\");\n@@ -2117,6 +2117,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t.prune = -1,\n \t\t.prune_tags = -1,\n \t\t.show_forced_updates = 1,\n+\t\t.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,\n \t};\n \tconst char *submodule_prefix = \"\";\n \tconst char *bundle_uri;\n@@ -2245,7 +2246,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t     builtin_fetch_options, builtin_fetch_usage, 0);\n \n \tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n-\t\trecurse_submodules = recurse_submodules_cli;\n+\t\tconfig.recurse_submodules = recurse_submodules_cli;\n \n \tif (negotiate_only) {\n \t\tswitch (recurse_submodules_cli) {\n@@ -2256,7 +2257,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t * submodules. Skip it by setting recurse_submodules to\n \t\t\t * RECURSE_SUBMODULES_OFF.\n \t\t\t */\n-\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\t\tconfig.recurse_submodules = RECURSE_SUBMODULES_OFF;\n \t\t\tbreak;\n \n \t\tdefault:\n@@ -2265,11 +2266,11 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n+\tif (config.recurse_submodules != RECURSE_SUBMODULES_OFF) {\n \t\tint *sfjc = submodule_fetch_jobs_config == -1\n \t\t\t    ? &submodule_fetch_jobs_config : NULL;\n-\t\tint *rs = recurse_submodules == RECURSE_SUBMODULES_DEFAULT\n-\t\t\t  ? &recurse_submodules : NULL;\n+\t\tint *rs = config.recurse_submodules == RECURSE_SUBMODULES_DEFAULT\n+\t\t\t  ? &config.recurse_submodules : NULL;\n \n \t\tfetch_config_from_gitmodules(sfjc, rs);\n \t}\n@@ -2283,7 +2284,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t * Reference updates in submodules would be ambiguous\n \t\t\t * in porcelain mode, so we reject this combination.\n \t\t\t */\n-\t\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\t\tconfig.recurse_submodules = RECURSE_SUBMODULES_OFF;\n \t\t\tbreak;\n \n \t\tdefault:\n@@ -2425,7 +2426,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t * the fetched history from each remote, so there is no need\n \t * to fetch submodules from here.\n \t */\n-\tif (!result && remote && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n+\tif (!result && remote && (config.recurse_submodules != RECURSE_SUBMODULES_OFF)) {\n \t\tstruct strvec options = STRVEC_INIT;\n \t\tint max_children = max_jobs;\n \n@@ -2438,7 +2439,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tresult = fetch_submodules(the_repository,\n \t\t\t\t\t  &options,\n \t\t\t\t\t  submodule_prefix,\n-\t\t\t\t\t  recurse_submodules,\n+\t\t\t\t\t  config.recurse_submodules,\n \t\t\t\t\t  recurse_submodules_default,\n \t\t\t\t\t  verbosity < 0,\n \t\t\t\t\t  max_children);\n-- \n2.40.1\n\n"},{"id":"477444","messageId":"8e729d7035a5dc2e1bc1272522eeba465051ee37.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 8/9] fetch: use `fetch_config` to store \"fetch.parallel\" value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:49:13Z","receivedAt":"2023-05-17T11:49:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move the parsed \"fetch.parallel\" config value into the `fetch_config`\nstructure. This reduces our reliance on global variables and further\nunifies the way we parse the configuration in git-fetch(1).\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 15 ++++++++-------\n 1 file changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b40df7e7ca..29b36da18a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -87,7 +87,6 @@ static int verbosity, deepen_relative, set_upstream, refetch;\n static int progress = -1;\n static int tags = TAGS_DEFAULT, update_shallow, deepen;\n static int submodule_fetch_jobs_config = -1;\n-static int fetch_parallel_config = 1;\n static int atomic_fetch;\n static enum transport_family family;\n static const char *depth;\n@@ -108,6 +107,7 @@ struct fetch_config {\n \tint prune_tags;\n \tint show_forced_updates;\n \tint recurse_submodules;\n+\tint parallel;\n };\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n@@ -144,11 +144,11 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t}\n \n \tif (!strcmp(k, \"fetch.parallel\")) {\n-\t\tfetch_parallel_config = git_config_int(k, v);\n-\t\tif (fetch_parallel_config < 0)\n+\t\tfetch_config->parallel = git_config_int(k, v);\n+\t\tif (fetch_config->parallel < 0)\n \t\t\tdie(_(\"fetch.parallel cannot be negative\"));\n-\t\tif (!fetch_parallel_config)\n-\t\t\tfetch_parallel_config = online_cpus();\n+\t\tif (!fetch_config->parallel)\n+\t\t\tfetch_config->parallel = online_cpus();\n \t\treturn 0;\n \t}\n \n@@ -2118,6 +2118,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t.prune_tags = -1,\n \t\t.show_forced_updates = 1,\n \t\t.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,\n+\t\t.parallel = 1,\n \t};\n \tconst char *submodule_prefix = \"\";\n \tconst char *bundle_uri;\n@@ -2411,7 +2412,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\t      \"from one remote\"));\n \n \t\tif (max_children < 0)\n-\t\t\tmax_children = fetch_parallel_config;\n+\t\t\tmax_children = config.parallel;\n \n \t\t/* TODO should this also die if we have a previous partial-clone? */\n \t\tresult = fetch_multiple(&list, max_children, &config);\n@@ -2433,7 +2434,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tif (max_children < 0)\n \t\t\tmax_children = submodule_fetch_jobs_config;\n \t\tif (max_children < 0)\n-\t\t\tmax_children = fetch_parallel_config;\n+\t\t\tmax_children = config.parallel;\n \n \t\tadd_options_to_argv(&options, &config);\n \t\tresult = fetch_submodules(the_repository,\n-- \n2.40.1\n\n"},{"id":"477445","messageId":"0eb84712a07243957d8037d7f1b3ad01260abe2f.1684324059.git.ps@pks.im","threadId":"59757","inReplyTo":"cover.1684324059.git.ps@pks.im","subject":"[PATCH 9/9] fetch: use `fetch_config` to store \"submodule.fetchJobs\" value","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-17T11:49:17Z","receivedAt":"2023-05-17T11:49:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Move the parsed \"submodule.fetchJobs\" config value into the\n`fetch_config` structure. This reduces our reliance on global variables\nand further unifies the way we parse the configuration in git-fetch(1).\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 29b36da18a..e3871048cf 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -86,7 +86,6 @@ static int write_fetch_head = 1;\n static int verbosity, deepen_relative, set_upstream, refetch;\n static int progress = -1;\n static int tags = TAGS_DEFAULT, update_shallow, deepen;\n-static int submodule_fetch_jobs_config = -1;\n static int atomic_fetch;\n static enum transport_family family;\n static const char *depth;\n@@ -108,6 +107,7 @@ struct fetch_config {\n \tint show_forced_updates;\n \tint recurse_submodules;\n \tint parallel;\n+\tint submodule_fetch_jobs;\n };\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n@@ -136,7 +136,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t}\n \n \tif (!strcmp(k, \"submodule.fetchjobs\")) {\n-\t\tsubmodule_fetch_jobs_config = parse_submodule_fetchjobs(k, v);\n+\t\tfetch_config->submodule_fetch_jobs = parse_submodule_fetchjobs(k, v);\n \t\treturn 0;\n \t} else if (!strcmp(k, \"fetch.recursesubmodules\")) {\n \t\tfetch_config->recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);\n@@ -2119,6 +2119,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t.show_forced_updates = 1,\n \t\t.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,\n \t\t.parallel = 1,\n+\t\t.submodule_fetch_jobs = -1,\n \t};\n \tconst char *submodule_prefix = \"\";\n \tconst char *bundle_uri;\n@@ -2268,8 +2269,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (config.recurse_submodules != RECURSE_SUBMODULES_OFF) {\n-\t\tint *sfjc = submodule_fetch_jobs_config == -1\n-\t\t\t    ? &submodule_fetch_jobs_config : NULL;\n+\t\tint *sfjc = config.submodule_fetch_jobs == -1\n+\t\t\t    ? &config.submodule_fetch_jobs : NULL;\n \t\tint *rs = config.recurse_submodules == RECURSE_SUBMODULES_DEFAULT\n \t\t\t  ? &config.recurse_submodules : NULL;\n \n@@ -2432,7 +2433,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tint max_children = max_jobs;\n \n \t\tif (max_children < 0)\n-\t\t\tmax_children = submodule_fetch_jobs_config;\n+\t\t\tmax_children = config.submodule_fetch_jobs;\n \t\tif (max_children < 0)\n \t\t\tmax_children = config.parallel;\n \n-- \n2.40.1\n\n"},{"id":"477575","messageId":"20230519001309.GA2442034@coredump.intra.peff.net","threadId":"59757","inReplyTo":"9417a62e9bbd30d9350b65e5396a31f9cb7c60b6.1684324059.git.ps@pks.im","subject":"Re: [PATCH 1/9] fetch: drop unused DISPLAY_FORMAT_UNKNOWN enum value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-19T00:13:09Z","receivedAt":"2023-05-19T00:13:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2023 at 01:48:42PM +0200, Patrick Steinhardt wrote:\n\n> With 50957937f9 (fetch: introduce `display_format` enum, 2023-05-10), a\n> new enumeration was introduced to determine the display format that is\n> to be used by git-fetch(1). The `DISPLAY_FORMAT_UNKNOWN` value isn't\n> ever used though, and neither do we rely on the explicit `0` value for\n> initialization anywhere.\n\nTo be slightly pedantic, we'd also want to make sure the we do not rely\non the zero value for reading, like:\n\n  if (display_state->format)\n     ....\n\nBut having looked over the code, we don't (it's always a switch or\nequality with a known name), so this is safe to do.\n\nThanks for cleaning this up.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 849a9be421..9147b700e5 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -58,7 +58,6 @@ enum {\n>  };\n>  \n>  enum display_format {\n> -\tDISPLAY_FORMAT_UNKNOWN = 0,\n>  \tDISPLAY_FORMAT_FULL,\n>  \tDISPLAY_FORMAT_COMPACT,\n>  \tDISPLAY_FORMAT_PORCELAIN,\n\nJust for similar situations in the future, I think we could do:\n\n  DISPLAY_FORMAT_FULL = 1,\n\nif we were worried about keeping the zero-behavior the same for existing\ncallers. But given how new and how limited this code is, I feel\nconfident that we've checked all of the code, and what you've written\nabove is preferable.\n\n-Peff\n"},{"id":"477576","messageId":"20230519001355.GB2442034@coredump.intra.peff.net","threadId":"59757","inReplyTo":"7197540df60d34e8f727e29d300dd0c96e31d310.1684324059.git.ps@pks.im","subject":"Re: [PATCH 2/9] fetch: drop unneeded NULL-check for `remote_ref`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-19T00:13:55Z","receivedAt":"2023-05-19T00:13:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2023 at 01:48:46PM +0200, Patrick Steinhardt wrote:\n\n> Drop the `NULL` check for `remote_ref` in `update_local_ref()`. The\n> function only has a single caller, and that caller always passes in a\n> non-`NULL` value.\n> \n> This fixes a false positive in Coverity.\n\nThanks for following up on this. I was thinking of just reposting it,\nthinking there weren't that many other cleanups to do in fetch.c. But\nyou found quite a few. :)\n\nThe patch looks obviously good to me.\n\n-Peff\n"},{"id":"477577","messageId":"20230519001803.GC2442034@coredump.intra.peff.net","threadId":"59757","inReplyTo":"b09f21dff786780dfb0ae71efd9a0cabe31c1e0b.1684324059.git.ps@pks.im","subject":"Re: [PATCH 3/9] fetch: pass through `fetch_config` directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-19T00:18:03Z","receivedAt":"2023-05-19T00:19:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2023 at 01:48:51PM +0200, Patrick Steinhardt wrote:\n\n> The `fetch_config` structure currently only has a single member, which\n> is the `display_format`. We're about extend it to contain all parsed\n> config values and will thus need it available in most of the code.\n> \n> Prepare for this change by passing through the `fetch_config` directly\n> instead of only passing its single member.\n\nMakes sense.\n\nOne small nit:\n\n>  static int do_fetch(struct transport *transport,\n>  \t\t    struct refspec *rs,\n> -\t\t    enum display_format display_format)\n> +\t\t    const struct fetch_config *config)\n>  {\n>  \tstruct ref_transaction *transaction = NULL;\n>  \tstruct ref *ref_map = NULL;\n> @@ -1639,7 +1639,8 @@ static int do_fetch(struct transport *transport,\n>  \tif (retcode)\n>  \t\tgoto cleanup;\n>  \n> -\tdisplay_state_init(&display_state, ref_map, transport->url, display_format);\n> +\tdisplay_state_init(&display_state, ref_map, transport->url,\n> +\t\t\t   config->display_format);\n\nIf the point is that fetch_config may start carrying new information,\nwouldn't we want to pass it as a whole down to display_state_init()? It\nmight eventually want to see some of that other config, too.\n\nIt's presumably academic for now, and it would not be too hard to change\nlater if needed, so I don't know that it's worth a re-roll. I just found\nit especially funny here since the purpose of the patch is to treat the\nconfig struct as a single unit.\n\n-Peff\n"},{"id":"477578","messageId":"20230519002128.GD2442034@coredump.intra.peff.net","threadId":"59757","inReplyTo":"1fa4922bea12ef0363e2638bbb5c70218c6ec357.1684324059.git.ps@pks.im","subject":"Re: [PATCH 4/9] fetch: use `fetch_config` to store \"fetch.prune\" value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-19T00:21:28Z","receivedAt":"2023-05-19T00:23:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 17, 2023 at 01:48:56PM +0200, Patrick Steinhardt wrote:\n\n> Move the parsed \"fetch.prune\" config value into the `fetch_config`\n> structure. This reduces our reliance on global variables and further\n> unifies the way we parse the configuration in git-fetch(1).\n\nMakes sense, and I like the end result. Since the other patches in this\nseries are all variants of this, I won't bother to respond individually\nto each one (though I think you were right to split them out). They all\nlook good to me.\n\n-Peff\n"},{"id":"477639","messageId":"ZGsuy6T_yiK4qxxJ@ncase","threadId":"59757","inReplyTo":"20230519001803.GC2442034@coredump.intra.peff.net","subject":"Re: [PATCH 3/9] fetch: pass through `fetch_config` directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-05-22T08:58:51Z","receivedAt":"2023-05-22T09:00:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 18, 2023 at 08:18:03PM -0400, Jeff King wrote:\n> On Wed, May 17, 2023 at 01:48:51PM +0200, Patrick Steinhardt wrote:\n> \n> > The `fetch_config` structure currently only has a single member, which\n> > is the `display_format`. We're about extend it to contain all parsed\n> > config values and will thus need it available in most of the code.\n> > \n> > Prepare for this change by passing through the `fetch_config` directly\n> > instead of only passing its single member.\n> \n> Makes sense.\n> \n> One small nit:\n> \n> >  static int do_fetch(struct transport *transport,\n> >  \t\t    struct refspec *rs,\n> > -\t\t    enum display_format display_format)\n> > +\t\t    const struct fetch_config *config)\n> >  {\n> >  \tstruct ref_transaction *transaction = NULL;\n> >  \tstruct ref *ref_map = NULL;\n> > @@ -1639,7 +1639,8 @@ static int do_fetch(struct transport *transport,\n> >  \tif (retcode)\n> >  \t\tgoto cleanup;\n> >  \n> > -\tdisplay_state_init(&display_state, ref_map, transport->url, display_format);\n> > +\tdisplay_state_init(&display_state, ref_map, transport->url,\n> > +\t\t\t   config->display_format);\n> \n> If the point is that fetch_config may start carrying new information,\n> wouldn't we want to pass it as a whole down to display_state_init()? It\n> might eventually want to see some of that other config, too.\n> \n> It's presumably academic for now, and it would not be too hard to change\n> later if needed, so I don't know that it's worth a re-roll. I just found\n> it especially funny here since the purpose of the patch is to treat the\n> config struct as a single unit.\n\nWell, I decided against passing in the full configuration as it feels a\nbit like a layering violation: the other code really is about the fetch\nitself, while this code here is only about display logic. So passing in\nthe `fetch_config` felt weird to me, even more so because we continue to\nonly need that single value at the end of this series. I do see your\npoint though.\n\nGiven that none of your other comments require a reroll I'll leave this\nas-is for now. Thanks for your review!\n\nPatrick\n"},{"id":"477643","messageId":"20230522191730.GA365027@coredump.intra.peff.net","threadId":"59757","inReplyTo":"ZGsuy6T_yiK4qxxJ@ncase","subject":"Re: [PATCH 3/9] fetch: pass through `fetch_config` directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-05-22T19:17:30Z","receivedAt":"2023-05-22T19:17:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 22, 2023 at 10:58:51AM +0200, Patrick Steinhardt wrote:\n\n> > If the point is that fetch_config may start carrying new information,\n> > wouldn't we want to pass it as a whole down to display_state_init()? It\n> > might eventually want to see some of that other config, too.\n> > \n> > It's presumably academic for now, and it would not be too hard to change\n> > later if needed, so I don't know that it's worth a re-roll. I just found\n> > it especially funny here since the purpose of the patch is to treat the\n> > config struct as a single unit.\n> \n> Well, I decided against passing in the full configuration as it feels a\n> bit like a layering violation: the other code really is about the fetch\n> itself, while this code here is only about display logic. So passing in\n> the `fetch_config` felt weird to me, even more so because we continue to\n> only need that single value at the end of this series. I do see your\n> point though.\n> \n> Given that none of your other comments require a reroll I'll leave this\n> as-is for now. Thanks for your review!\n\nYeah, I could see that line of thinking, as well. Leaving sounds good to\nme. And I think that was my only substantive comment on the whole\nseries, so we can consider the whole reviewed-by: me.\n\n-Peff\n"}]}