{"thread":{"id":"59546","subject":"[PATCH 0/2] Add fetch.updateHead option","startedAt":"2023-04-05T01:27:51Z","lastAt":"2023-04-07T02:41:30Z","messageCount":9,"participants":["Felipe Contreras","Ævar Arnfjörð Bjarmason","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"474816","messageId":"20230405012742.2452208-1-felipe.contreras@gmail.com","threadId":"59546","inReplyTo":null,"subject":"[PATCH 0/2] Add fetch.updateHead option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-05T01:27:40Z","receivedAt":"2023-04-05T01:27:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"It's surprising that `git clone` and `git init && git remote add -f` don't\ncreate the same remote state.\n\nFix this by introducing a new configuration: `fetch.updateHead` which updates\nthe remote `HEAD` when it's not present with \"missing\", or always with\n\"always\".\n\nBy default it's \"never\", which retains the current behavior.\n\nThis has already been discussed before [1].\n\n[1] https://lore.kernel.org/git/20201118091219.3341585-1-felipe.contreras@gmail.com/\n\nFelipe Contreras (2):\n Add fetch.updateHead option\n fetch: add support for HEAD update on mirrors\n\n Documentation/config/fetch.txt | 4 ++\n Documentation/config/remote.txt | 3 ++\n builtin/fetch.c | 69 ++++++++++++++++++++++++++++++++-\n remote.c | 21 ++++++++++\n remote.h | 11 ++++++\n t/t5510-fetch.sh | 49 +++++++++++++++++++++++\n 6 files changed, 156 insertions(+), 1 deletion(-)\n\n-- \n2.40.0+fc1\n\n"},{"id":"474817","messageId":"20230405012742.2452208-2-felipe.contreras@gmail.com","threadId":"59546","inReplyTo":"20230405012742.2452208-1-felipe.contreras@gmail.com","subject":"[PATCH 1/2] Add fetch.updateHead option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-05T01:27:41Z","receivedAt":"2023-04-05T01:27:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Users might change the behavior when running \"git fetch\" so that the\nremote's HEAD symbolic ref is updated at certain point.\n\nFor example after running \"git remote add\" the remote HEAD is not\nset like it is with \"git clone\".\n\nSetting \"fetch.updatehead = missing\" would probably be a sensible\ndefault that everyone would want, but for now the default behavior is to\nnever update HEAD, so there shouldn't be any functional changes.\n\nFor the next major version of Git, we might want to change this default.\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/config/fetch.txt | 4 +++\n Documentation/config/remote.txt | 3 ++\n builtin/fetch.c | 64 ++++++++++++++++++++++++++++++++-\n remote.c | 21 +++++++++++\n remote.h | 11 ++++++\n t/t5510-fetch.sh | 31 ++++++++++++++++\n 6 files changed, 133 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex 568f0f75b3..dc147ffb35 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -120,3 +120,7 @@ fetch.bundleCreationToken::\n The creation token values are chosen by the provider serving the specific\n bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to\n remove the value for the `fetch.bundleCreationToken` value before fetching.\n+\n+fetch.updateHead::\n+\tDefines when to update the remote HEAD symbolic ref. Values are 'never',\n+\t'missing' (update only when HEAD is missing), and 'always'.\ndiff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\nindex 0678b4bcfe..9d739d2ed4 100644\n--- a/Documentation/config/remote.txt\n+++ b/Documentation/config/remote.txt\n@@ -86,3 +86,6 @@ remote..partialclonefilter::\n \tChanging or clearing this value will only affect fetches for new commits.\n \tTo fetch associated objects for commits already present in the local object\n \tdatabase, use the `--refetch` option of linkgit:git-fetch[1].\n+\n+remote..updateHead::\n+\tDefines when to update the remote HEAD symbolic ref. See `fetch.updateHead`.\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7221e57f35..7e93a1aa46 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -59,6 +59,8 @@ 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+static int fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;\n+\n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n static int write_fetch_head = 1;\n static int verbosity, deepen_relative, set_upstream, refetch;\n@@ -129,6 +131,9 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(k, \"fetch.updatehead\"))\n+\t\treturn parse_update_head(&fetch_update_head, k, v);\n+\n \treturn git_default_config(k, v, cb);\n }\n \n@@ -1579,6 +1584,47 @@ static int backfill_tags(struct transport *transport,\n \treturn retcode;\n }\n \n+static void update_head(int config, const struct ref *head, const struct remote *remote)\n+{\n+\tchar *ref, *target;\n+\tconst char *r;\n+\tint flags;\n+\n+\tif (!head || !head->symref || !remote)\n+\t\treturn;\n+\n+\tref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n+\ttarget = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n+\n+\tif (!ref || !target) {\n+\t\twarning(_(\"could not update remote head\"));\n+\t\treturn;\n+\t}\n+\n+\tr = resolve_ref_unsafe(ref, 0, NULL, &flags);\n+\n+\tif (r) {\n+\t\tif (config == FETCH_UPDATE_HEAD_MISSING) {\n+\t\t\tif (flags & REF_ISSYMREF)\n+\t\t\t\t/* already present */\n+\t\t\t\treturn;\n+\t\t} else if (config == FETCH_UPDATE_HEAD_ALWAYS) {\n+\t\t\tif (!strcmp(r, target))\n+\t\t\t\t/* already up-to-date */\n+\t\t\t\treturn;\n+\t\t} else\n+\t\t\t/* should never happen */\n+\t\t\treturn;\n+\t}\n+\n+\tif (!create_symref(ref, target, \"remote update head\")) {\n+\t\tif (verbosity >= 0)\n+\t\t\tprintf(_(\"Updated remote '%s' HEAD\\n\"), remote->name);\n+\t} else {\n+\t\twarning(_(\"could not update remote head\"));\n+\t}\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs)\n {\n@@ -1592,6 +1638,7 @@ static int do_fetch(struct transport *transport,\n \tint must_list_refs = 1;\n \tstruct fetch_head fetch_head = { 0 };\n \tstruct strbuf err = STRBUF_INIT;\n+\tint need_update_head = 0, update_head_config = 0;\n \n \tif (tags == TAGS_DEFAULT) {\n \t\tif (transport->remote->fetch_tags == 2)\n@@ -1626,9 +1673,21 @@ static int do_fetch(struct transport *transport,\n \t} else {\n \t\tstruct branch *branch = branch_get(NULL);\n \n-\t\tif (transport->remote->fetch.nr)\n+\t\tif (transport->remote->fetch.nr) {\n+\n+\t\t\tif (transport->remote->update_head)\n+\t\t\t\tupdate_head_config = transport->remote->update_head;\n+\t\t\telse\n+\t\t\t\tupdate_head_config = fetch_update_head;\n+\n+\t\t\tneed_update_head = update_head_config && update_head_config != FETCH_UPDATE_HEAD_NEVER;\n+\n+\t\t\tif (need_update_head)\n+\t\t\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n \t\t\trefspec_ref_prefixes(&transport->remote->fetch,\n \t\t\t\t\t &transport_ls_refs_options.ref_prefixes);\n+\t\t}\n+\n \t\tif (branch_has_merge_config(branch) &&\n \t\t !strcmp(branch->remote_name, transport->remote->name)) {\n \t\t\tint i;\n@@ -1737,6 +1796,9 @@ static int do_fetch(struct transport *transport,\n \n \tcommit_fetch_head(&fetch_head);\n \n+\tif (need_update_head)\n+\t\tupdate_head(update_head_config, find_ref_by_name(remote_refs, \"HEAD\"), transport->remote);\n+\n \tif (set_upstream) {\n \t\tstruct branch *branch = branch_get(\"HEAD\");\n \t\tstruct ref *rm;\ndiff --git a/remote.c b/remote.c\nindex 641b083d90..5f3a9aa53e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -344,6 +344,25 @@ static void read_branches_file(struct remote_state *remote_state,\n \tremote->fetch_tags = 1; /* always auto-follow */\n }\n \n+int parse_update_head(int *r, const char *var, const char *value)\n+{\n+\tif (!r)\n+\t\treturn -1;\n+\telse if (!value)\n+\t\treturn config_error_nonbool(var);\n+\telse if (!strcmp(value, \"never\"))\n+\t\t*r = FETCH_UPDATE_HEAD_NEVER;\n+\telse if (!strcmp(value, \"missing\"))\n+\t\t*r = FETCH_UPDATE_HEAD_MISSING;\n+\telse if (!strcmp(value, \"always\"))\n+\t\t*r = FETCH_UPDATE_HEAD_ALWAYS;\n+\telse {\n+\t\terror(_(\"malformed value for %s: %s\"), var, value);\n+\t\treturn error(_(\"must be one of never, missing, or always\"));\n+\t}\n+\treturn 0;\n+}\n+\n static int handle_config(const char *key, const char *value, void *cb)\n {\n \tconst char *name;\n@@ -473,6 +492,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\t\t\t key, value);\n \t} else if (!strcmp(subkey, \"vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n+\t} else if (!strcmp(subkey, \"updatehead\")) {\n+\t\treturn parse_update_head(&remote->update_head, key, value);\n \t}\n \treturn 0;\n }\ndiff --git a/remote.h b/remote.h\nindex 73638cefeb..9dce42d65d 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -22,6 +22,13 @@ enum {\n \tREMOTE_BRANCHES\n };\n \n+enum {\n+\tFETCH_UPDATE_HEAD_DEFAULT = 0,\n+\tFETCH_UPDATE_HEAD_NEVER,\n+\tFETCH_UPDATE_HEAD_MISSING,\n+\tFETCH_UPDATE_HEAD_ALWAYS,\n+};\n+\n struct rewrite {\n \tconst char *base;\n \tsize_t baselen;\n@@ -97,6 +104,8 @@ struct remote {\n \tint prune;\n \tint prune_tags;\n \n+\tint update_head;\n+\n \t/**\n \t * The configured helper programs to run on the remote side, for\n \t * Git-native protocols.\n@@ -449,4 +458,6 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);\n char *relative_url(const char *remote_url, const char *url,\n \t\t const char *up_path);\n \n+int parse_update_head(int *r, const char *var, const char *value);\n+\n #endif\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex dc44da9c79..dbeb2928ae 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -814,6 +814,37 @@ test_expect_success 'fetch from multiple configured URLs in single remote' '\n \tgit fetch multipleurls\n '\n \n+test_cmp_symbolic_ref () {\n+\tgit symbolic-ref \"$1\" >actual &&\n+\techo \"$2\" >expected &&\n+\ttest_cmp expected actual\n+}\n+\n+test_expect_success 'updatehead' '\n+\ttest_when_finished \"rm -rf updatehead\" &&\n+\n+\tgit init updatehead &&\n+\t(\n+\t\tcd updatehead &&\n+\n+\t\tgit config fetch.updateHead never &&\n+\t\tgit remote add origin .. &&\n+\t\tgit fetch &&\n+\t\ttest_must_fail git rev-parse --verify refs/remotes/origin/HEAD &&\n+\n+\t\tgit config fetch.updateHead missing &&\n+\t\tgit fetch &&\n+\t\ttest_cmp_symbolic_ref refs/remotes/origin/HEAD refs/remotes/origin/main &&\n+\t\tgit symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/side &&\n+\t\tgit fetch &&\n+\t\ttest_cmp_symbolic_ref refs/remotes/origin/HEAD refs/remotes/origin/side &&\n+\n+\t\tgit config fetch.updateHead always &&\n+\t\tgit fetch &&\n+\t\ttest_cmp_symbolic_ref refs/remotes/origin/HEAD refs/remotes/origin/main\n+\t)\n+'\n+\n # configured prune tests\n \n set_config_tristate () {\n-- \n2.40.0+fc1\n\n"},{"id":"474818","messageId":"20230405012742.2452208-3-felipe.contreras@gmail.com","threadId":"59546","inReplyTo":"20230405012742.2452208-1-felipe.contreras@gmail.com","subject":"[PATCH 2/2] fetch: add support for HEAD update on mirrors","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-05T01:27:42Z","receivedAt":"2023-04-05T01:28:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Signed-off-by: Felipe Contreras \n---\n builtin/fetch.c | 15 ++++++++++-----\n t/t5510-fetch.sh | 18 ++++++++++++++++++\n 2 files changed, 28 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 7e93a1aa46..6bf147b012 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1593,12 +1593,17 @@ static void update_head(int config, const struct ref *head, const struct remote\n \tif (!head || !head->symref || !remote)\n \t\treturn;\n \n-\tref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n-\ttarget = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n+\tif (!remote->mirror) {\n+\t\tref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n+\t\ttarget = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n \n-\tif (!ref || !target) {\n-\t\twarning(_(\"could not update remote head\"));\n-\t\treturn;\n+\t\tif (!ref || !target) {\n+\t\t\twarning(_(\"could not update remote head\"));\n+\t\t\treturn;\n+\t\t}\n+\t} else {\n+\t\tref = \"HEAD\";\n+\t\ttarget = head->symref;\n \t}\n \n \tr = resolve_ref_unsafe(ref, 0, NULL, &flags);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex dbeb2928ae..d3f3b24378 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -845,6 +845,24 @@ test_expect_success 'updatehead' '\n \t)\n '\n \n+test_expect_success 'updatehead mirror' '\n+\ttest_when_finished \"rm -rf updatehead\" &&\n+\n+\tgit clone --mirror . updatehead &&\n+\t(\n+\t\tcd updatehead &&\n+\n+\t\tgit config fetch.updateHead missing &&\n+\t\tgit symbolic-ref HEAD refs/heads/side &&\n+\t\tgit fetch &&\n+\t\ttest_cmp_symbolic_ref HEAD refs/heads/side &&\n+\n+\t\tgit config fetch.updateHead always &&\n+\t\tgit fetch &&\n+\t\ttest_cmp_symbolic_ref HEAD refs/heads/main\n+\t)\n+'\n+\n # configured prune tests\n \n set_config_tristate () {\n-- \n2.40.0+fc1\n\n"},{"id":"474832","messageId":"230405.86fs9evfte.gmgdl@evledraar.gmail.com","threadId":"59546","inReplyTo":"20230405012742.2452208-2-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/2] Add fetch.updateHead option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-05T09:16:12Z","receivedAt":"2023-04-05T09:29:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 04 2023, Felipe Contreras wrote:\n\n> Users might change the behavior when running \"git fetch\" so that the\n> remote's HEAD symbolic ref is updated at certain point.\n>\n> For example after running \"git remote add\" the remote HEAD is not\n> set like it is with \"git clone\".\n>\n> Setting \"fetch.updatehead = missing\" would probably be a sensible\n> default that everyone would want, but for now the default behavior is to\n> never update HEAD, so there shouldn't be any functional changes.\n>\n> For the next major version of Git, we might want to change this default.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> Documentation/config/fetch.txt | 4 +++\n> Documentation/config/remote.txt | 3 ++\n> builtin/fetch.c | 64 ++++++++++++++++++++++++++++++++-\n> remote.c | 21 +++++++++++\n> remote.h | 11 ++++++\n> t/t5510-fetch.sh | 31 ++++++++++++++++\n> 6 files changed, 133 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\n> index 568f0f75b3..dc147ffb35 100644\n> --- a/Documentation/config/fetch.txt\n> +++ b/Documentation/config/fetch.txt\n> @@ -120,3 +120,7 @@ fetch.bundleCreationToken::\n> The creation token values are chosen by the provider serving the specific\n> bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to\n> remove the value for the `fetch.bundleCreationToken` value before fetching.\n> +\n> +fetch.updateHead::\n> +\tDefines when to update the remote HEAD symbolic ref. Values are 'never',\n> +\t'missing' (update only when HEAD is missing), and 'always'.\n> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> index 0678b4bcfe..9d739d2ed4 100644\n> --- a/Documentation/config/remote.txt\n> +++ b/Documentation/config/remote.txt\n> @@ -86,3 +86,6 @@ remote..partialclonefilter::\n> \tChanging or clearing this value will only affect fetches for new commits.\n> \tTo fetch associated objects for commits already present in the local object\n> \tdatabase, use the `--refetch` option of linkgit:git-fetch[1].\n> +\n> +remote..updateHead::\n> +\tDefines when to update the remote HEAD symbolic ref. See `fetch.updateHead`.\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 7221e57f35..7e93a1aa46 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -59,6 +59,8 @@ 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> +static int fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;\n> +\n> static int all, append, dry_run, force, keep, multiple, update_head_ok;\n> static int write_fetch_head = 1;\n> static int verbosity, deepen_relative, set_upstream, refetch;\n> @@ -129,6 +131,9 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (!strcmp(k, \"fetch.updatehead\"))\n> +\t\treturn parse_update_head(&fetch_update_head, k, v);\n> +\n> \treturn git_default_config(k, v, cb);\n> }\n> \n> @@ -1579,6 +1584,47 @@ static int backfill_tags(struct transport *transport,\n> \treturn retcode;\n> }\n> \n> +static void update_head(int config, const struct ref *head, const struct remote *remote)\n\nHere you pass a \"const struct remote\".\n\n> +{\n> +\tchar *ref, *target;\n> +\tconst char *r;\n> +\tint flags;\n> +\n> +\tif (!head || !head->symref || !remote)\n> +\t\treturn;\n> +\n> +\tref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n> +\ttarget = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n\nBut here we end up with this cast, as it's not const after all, we're\nmodifying it.\n\nI think this sort of thing makes the code harder to read & reason about,\nand adds cast verbosity.\n\nIf you want to clearly communicate that the \"remote->name\" and\n\"remote->mirror\" you're using are \"const\" I think a better way to do\nthis is to pass those as explicit parameters to this new static helper\nfunction, and then just pass a \"struct refspec *fetch_rs\" directly.\n\n> +\n> +\tif (!ref || !target) {\n> +\t\twarning(_(\"could not update remote head\"));\n> +\t\treturn;\n> +\t}\n> +\n> +\tr = resolve_ref_unsafe(ref, 0, NULL, &flags);\n> +\n> +\tif (r) {\n> +\t\tif (config == FETCH_UPDATE_HEAD_MISSING) {\n> +\t\t\tif (flags & REF_ISSYMREF)\n> +\t\t\t\t/* already present */\n> +\t\t\t\treturn;\n> +\t\t} else if (config == FETCH_UPDATE_HEAD_ALWAYS) {\n> +\t\t\tif (!strcmp(r, target))\n> +\t\t\t\t/* already up-to-date */\n> +\t\t\t\treturn;\n\nI think you should name the \"enum\" you're adding below, the one that\ncontains the new \"FETCH_UPDATE_HEAD_DEFAULT\".\n\nThen this could be a \"switch\", and the compiler could check the\narguments, i.e. you could pass an enum type instead of an \"int\".\n\n> +\t\t} else\n\n{} missing, if you keep this, but...\n\n> +\t\t\t/* should never happen */\n> +\t\t\treturn;\n\n...so, here we're not checking some enum values, but presumably other\nthings check this, I haven't checked.\n\n\nBut for a \"should never happen\", should we make this a \"BUG()\", or is it\nuser-controlled?\n\n\n\n> +\t}\n> +\n> +\tif (!create_symref(ref, target, \"remote update head\")) {\n> +\t\tif (verbosity >= 0)\n> +\t\t\tprintf(_(\"Updated remote '%s' HEAD\\n\"), remote->name);\n> +\t} else {\n> +\t\twarning(_(\"could not update remote head\"));\n> +\t}\n> +}\n> +\n> static int do_fetch(struct transport *transport,\n> \t\t struct refspec *rs)\n> {\n> @@ -1592,6 +1638,7 @@ static int do_fetch(struct transport *transport,\n> \tint must_list_refs = 1;\n> \tstruct fetch_head fetch_head = { 0 };\n> \tstruct strbuf err = STRBUF_INIT;\n> +\tint need_update_head = 0, update_head_config = 0;\n> \n> \tif (tags == TAGS_DEFAULT) {\n> \t\tif (transport->remote->fetch_tags == 2)\n> @@ -1626,9 +1673,21 @@ static int do_fetch(struct transport *transport,\n> \t} else {\n> \t\tstruct branch *branch = branch_get(NULL);\n> \n> -\t\tif (transport->remote->fetch.nr)\n> +\t\tif (transport->remote->fetch.nr) {\n> +\n> +\t\t\tif (transport->remote->update_head)\n> +\t\t\t\tupdate_head_config = transport->remote->update_head;\n> +\t\t\telse\n> +\t\t\t\tupdate_head_config = fetch_update_head;\n> +\n> +\t\t\tneed_update_head = update_head_config && update_head_config != FETCH_UPDATE_HEAD_NEVER;\n> +\n> +\t\t\tif (need_update_head)\n> +\t\t\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n> \t\t\trefspec_ref_prefixes(&transport->remote->fetch,\n> \t\t\t\t\t &transport_ls_refs_options.ref_prefixes);\n> +\t\t}\n> +\n> \t\tif (branch_has_merge_config(branch) &&\n> \t\t !strcmp(branch->remote_name, transport->remote->name)) {\n> \t\t\tint i;\n> @@ -1737,6 +1796,9 @@ static int do_fetch(struct transport *transport,\n> \n> \tcommit_fetch_head(&fetch_head);\n> \n> +\tif (need_update_head)\n> +\t\tupdate_head(update_head_config, find_ref_by_name(remote_refs, \"HEAD\"), transport->remote);\n\nSome overly long lines here...\n\n> +\n> \tif (set_upstream) {\n> \t\tstruct branch *branch = branch_get(\"HEAD\");\n> \t\tstruct ref *rm;\n> diff --git a/remote.c b/remote.c\n> index 641b083d90..5f3a9aa53e 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -344,6 +344,25 @@ static void read_branches_file(struct remote_state *remote_state,\n> \tremote->fetch_tags = 1; /* always auto-follow */\n> }\n> \n> +int parse_update_head(int *r, const char *var, const char *value)\n> +{\n> +\tif (!r)\n> +\t\treturn -1;\n> +\telse if (!value)\n> +\t\treturn config_error_nonbool(var);\n> +\telse if (!strcmp(value, \"never\"))\n> +\t\t*r = FETCH_UPDATE_HEAD_NEVER;\n> +\telse if (!strcmp(value, \"missing\"))\n> +\t\t*r = FETCH_UPDATE_HEAD_MISSING;\n> +\telse if (!strcmp(value, \"always\"))\n> +\t\t*r = FETCH_UPDATE_HEAD_ALWAYS;\n\nDitto, this could really benefit from an enum type, instead of the bare\n\"int\".\n\n> +\telse {\n> +\t\terror(_(\"malformed value for %s: %s\"), var, value);\n> +\t\treturn error(_(\"must be one of never, missing, or always\"));\n\nShouldn't we use git_die_config() or similar here, to get the line\nnumber etc., or do we get that somehow (I can't recall).\n\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> static int handle_config(const char *key, const char *value, void *cb)\n> {\n> \tconst char *name;\n> @@ -473,6 +492,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n> \t\t\t\t\t key, value);\n> \t} else if (!strcmp(subkey, \"vcs\")) {\n> \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n> +\t} else if (!strcmp(subkey, \"updatehead\")) {\n> +\t\treturn parse_update_head(&remote->update_head, key, value);\n> \t}\n> \treturn 0;\n> }\n> diff --git a/remote.h b/remote.h\n> index 73638cefeb..9dce42d65d 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -22,6 +22,13 @@ enum {\n> \tREMOTE_BRANCHES\n> };\n> \n> +enum {\n> +\tFETCH_UPDATE_HEAD_DEFAULT = 0,\n\nWe tend to only init these to 0 when the default being 0 matters,\ni.e. we use it as a boolean, but is that the case here?\n\n> +\tFETCH_UPDATE_HEAD_NEVER,\n> +\tFETCH_UPDATE_HEAD_MISSING,\n> +\tFETCH_UPDATE_HEAD_ALWAYS,\n> +};\n\nI.e. let's name this.\n\n\n> +\n> struct rewrite {\n> \tconst char *base;\n> \tsize_t baselen;\n> @@ -97,6 +104,8 @@ struct remote {\n> \tint prune;\n> \tint prune_tags;\n> \n> +\tint update_head;\n> +\n> \t/**\n> \t * The configured helper programs to run on the remote side, for\n> \t * Git-native protocols.\n> @@ -449,4 +458,6 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);\n> char *relative_url(const char *remote_url, const char *url,\n> \t\t const char *up_path);\n> \n> +int parse_update_head(int *r, const char *var, const char *value);\n\nFor new functions and/or enums, having some brief API docs (using the\n\"/** ... */\" syntax) would be better.\n\n\n> +\n> #endif\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index dc44da9c79..dbeb2928ae 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -814,6 +814,37 @@ test_expect_success 'fetch from multiple configured URLs in single remote' '\n> \tgit fetch multipleurls\n> '\n> \n> +test_cmp_symbolic_ref () {\n> +\tgit symbolic-ref \"$1\" >actual &&\n> +\techo \"$2\" >expected &&\n> +\ttest_cmp expected actual\n> +}\n\nSort of an aside, but this seems to be the Nth use of this pattern in\nthe test suite, e.g. t1401-symbolic-ref.sh repeatedly hardcodes the\nsame.\n\nI wonder if a prep commit to stick this in test-lib-functions.sh would\nbe in order, or maybe a \"--symbolic\" argument to \"test_cmp_rev\"?\n"},{"id":"474833","messageId":"230405.86bkk2vfpo.gmgdl@evledraar.gmail.com","threadId":"59546","inReplyTo":"20230405012742.2452208-2-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/2] Add fetch.updateHead option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-05T09:28:56Z","receivedAt":"2023-04-05T09:31:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 04 2023, Felipe Contreras wrote:\n\n> Users might change the behavior when running \"git fetch\" so that the\n> remote's HEAD symbolic ref is updated at certain point.\n>\n> For example after running \"git remote add\" the remote HEAD is not\n> set like it is with \"git clone\".\n>\n> Setting \"fetch.updatehead = missing\" would probably be a sensible\n> default that everyone would want, but for now the default behavior is to\n> never update HEAD, so there shouldn't be any functional changes.\n>\n> For the next major version of Git, we might want to change this default.\n>\n> Signed-off-by: Felipe Contreras \n> ---\n> Documentation/config/fetch.txt | 4 +++\n> Documentation/config/remote.txt | 3 ++\n> builtin/fetch.c | 64 ++++++++++++++++++++++++++++++++-\n> remote.c | 21 +++++++++++\n> remote.h | 11 ++++++\n> t/t5510-fetch.sh | 31 ++++++++++++++++\n> 6 files changed, 133 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\n> index 568f0f75b3..dc147ffb35 100644\n> --- a/Documentation/config/fetch.txt\n> +++ b/Documentation/config/fetch.txt\n> @@ -120,3 +120,7 @@ fetch.bundleCreationToken::\n> The creation token values are chosen by the provider serving the specific\n> bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to\n> remove the value for the `fetch.bundleCreationToken` value before fetching.\n> +\n> +fetch.updateHead::\n> +\tDefines when to update the remote HEAD symbolic ref. Values are 'never',\n> +\t'missing' (update only when HEAD is missing), and 'always'.\n\nMissed the first time around, I think it would be useful to explain the\nhistorical behavior heher, and why it's been in place.\n\nI.e. that we use this during the initial fetch/clone to find the \"HEAD\",\nfor discovering the default branch, but then proceed to not care about\nit after that.\n\nWe should also link and cross-link to the other recent-ish config\noptions (whose name I'm blanking on), which implement the \"take the\nremote's suggestion of a default branch name\" here.\n\n"},{"id":"474835","messageId":"ZC1KW3oN1JgrvTfn@ncase","threadId":"59546","inReplyTo":"230405.86fs9evfte.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] Add fetch.updateHead option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-04-05T10:15:55Z","receivedAt":"2023-04-05T10:16:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 05, 2023 at 11:16:12AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> On Tue, Apr 04 2023, Felipe Contreras wrote:\n[snip]\n> > @@ -1579,6 +1584,47 @@ static int backfill_tags(struct transport *transport,\n> > \treturn retcode;\n> > }\n> > \n> > +static void update_head(int config, const struct ref *head, const struct remote *remote)\n> \n> Here you pass a \"const struct remote\".\n> \n> > +{\n> > +\tchar *ref, *target;\n> > +\tconst char *r;\n> > +\tint flags;\n> > +\n> > +\tif (!head || !head->symref || !remote)\n> > +\t\treturn;\n> > +\n> > +\tref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n> > +\ttarget = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n> \n> But here we end up with this cast, as it's not const after all, we're\n> modifying it.\n> \n> I think this sort of thing makes the code harder to read & reason about,\n> and adds cast verbosity.\n> \n> If you want to clearly communicate that the \"remote->name\" and\n> \"remote->mirror\" you're using are \"const\" I think a better way to do\n> this is to pass those as explicit parameters to this new static helper\n> function, and then just pass a \"struct refspec *fetch_rs\" directly.\n\nI think the underlying problem is that `apply_refspecs()` and\ntransitively called functions expect the argument to be non-const even\nthough they never modify it.\n\nSo maybe the proper way to handle this would be to add a preparatory\npatch that constifies the parameter. Something like what I've attached\nto the end of this mail.\n\nPatrick\n\n-- >8 --\n\ndiff --git a/remote.c b/remote.c\nindex b04e5da338..1752c391c3 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -851,7 +851,7 @@ static int refspec_match(const struct refspec_item *refspec,\n \treturn !strcmp(refspec->src, name);\n }\n \n-int omit_name_by_refspec(const char *name, struct refspec *rs)\n+int omit_name_by_refspec(const char *name, const struct refspec *rs)\n {\n \tint i;\n \n@@ -880,7 +880,7 @@ struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs)\n \treturn ref_map;\n }\n \n-static int query_matches_negative_refspec(struct refspec *rs, struct refspec_item *query)\n+static int query_matches_negative_refspec(const struct refspec *rs, struct refspec_item *query)\n {\n \tint i, matched_negative = 0;\n \tint find_src = !query->src;\n@@ -968,7 +968,7 @@ static void query_refspecs_multiple(struct refspec *rs,\n \t}\n }\n \n-int query_refspecs(struct refspec *rs, struct refspec_item *query)\n+int query_refspecs(const struct refspec *rs, struct refspec_item *query)\n {\n \tint i;\n \tint find_src = !query->src;\n@@ -1002,7 +1002,7 @@ int query_refspecs(struct refspec *rs, struct refspec_item *query)\n \treturn -1;\n }\n \n-char *apply_refspecs(struct refspec *rs, const char *name)\n+char *apply_refspecs(const struct refspec *rs, const char *name)\n {\n \tstruct refspec_item query;\n \ndiff --git a/remote.h b/remote.h\nindex 5b38ee20b8..cd3c1439ab 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -253,7 +253,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);\n * Check whether a name matches any negative refspec in rs. Returns 1 if the\n * name matches at least one negative refspec, and 0 otherwise.\n */\n-int omit_name_by_refspec(const char *name, struct refspec *rs);\n+int omit_name_by_refspec(const char *name, const struct refspec *rs);\n \n /*\n * Remove all entries in the input list which match any negative refspec in\n@@ -261,8 +261,8 @@ int omit_name_by_refspec(const char *name, struct refspec *rs);\n */\n struct ref *apply_negative_refspecs(struct ref *ref_map, struct refspec *rs);\n \n-int query_refspecs(struct refspec *rs, struct refspec_item *query);\n-char *apply_refspecs(struct refspec *rs, const char *name);\n+int query_refspecs(const struct refspec *rs, struct refspec_item *query);\n+char *apply_refspecs(const struct refspec *rs, const char *name);\n \n int check_push_refs(struct ref *src, struct refspec *rs);\n int match_push_refs(struct ref *src, struct ref **dst,\n\n"},{"id":"474849","messageId":"CAMP44s0oBKfp6bXbg_+vp4CuRj_nh8uDBTCeT65z7UCUzj4K0Q@mail.gmail.com","threadId":"59546","inReplyTo":"230405.86fs9evfte.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] Add fetch.updateHead option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-05T14:55:17Z","receivedAt":"2023-04-05T14:55:39Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Apr 5, 2023 at 4:28 AM Ævar Arnfjörð Bjarmason wrote:\n>\n>\n> On Tue, Apr 04 2023, Felipe Contreras wrote:\n>\n> > Users might change the behavior when running \"git fetch\" so that the\n> > remote's HEAD symbolic ref is updated at certain point.\n> >\n> > For example after running \"git remote add\" the remote HEAD is not\n> > set like it is with \"git clone\".\n> >\n> > Setting \"fetch.updatehead = missing\" would probably be a sensible\n> > default that everyone would want, but for now the default behavior is to\n> > never update HEAD, so there shouldn't be any functional changes.\n> >\n> > For the next major version of Git, we might want to change this default.\n> >\n> > Signed-off-by: Felipe Contreras \n> > ---\n> > Documentation/config/fetch.txt | 4 +++\n> > Documentation/config/remote.txt | 3 ++\n> > builtin/fetch.c | 64 ++++++++++++++++++++++++++++++++-\n> > remote.c | 21 +++++++++++\n> > remote.h | 11 ++++++\n> > t/t5510-fetch.sh | 31 ++++++++++++++++\n> > 6 files changed, 133 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\n> > index 568f0f75b3..dc147ffb35 100644\n> > --- a/Documentation/config/fetch.txt\n> > +++ b/Documentation/config/fetch.txt\n> > @@ -120,3 +120,7 @@ fetch.bundleCreationToken::\n> > The creation token values are chosen by the provider serving the specific\n> > bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to\n> > remove the value for the `fetch.bundleCreationToken` value before fetching.\n> > +\n> > +fetch.updateHead::\n> > + Defines when to update the remote HEAD symbolic ref. Values are 'never',\n> > + 'missing' (update only when HEAD is missing), and 'always'.\n> > diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n> > index 0678b4bcfe..9d739d2ed4 100644\n> > --- a/Documentation/config/remote.txt\n> > +++ b/Documentation/config/remote.txt\n> > @@ -86,3 +86,6 @@ remote..partialclonefilter::\n> > Changing or clearing this value will only affect fetches for new commits.\n> > To fetch associated objects for commits already present in the local object\n> > database, use the `--refetch` option of linkgit:git-fetch[1].\n> > +\n> > +remote..updateHead::\n> > + Defines when to update the remote HEAD symbolic ref. See `fetch.updateHead`.\n> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n> > index 7221e57f35..7e93a1aa46 100644\n> > --- a/builtin/fetch.c\n> > +++ b/builtin/fetch.c\n> > @@ -59,6 +59,8 @@ 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> > +static int fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;\n> > +\n> > static int all, append, dry_run, force, keep, multiple, update_head_ok;\n> > static int write_fetch_head = 1;\n> > static int verbosity, deepen_relative, set_upstream, refetch;\n> > @@ -129,6 +131,9 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n> > return 0;\n> > }\n> >\n> > + if (!strcmp(k, \"fetch.updatehead\"))\n> > + return parse_update_head(&fetch_update_head, k, v);\n> > +\n> > return git_default_config(k, v, cb);\n> > }\n> >\n> > @@ -1579,6 +1584,47 @@ static int backfill_tags(struct transport *transport,\n> > return retcode;\n> > }\n> >\n> > +static void update_head(int config, const struct ref *head, const struct remote *remote)\n>\n> Here you pass a \"const struct remote\".\n>\n> > +{\n> > + char *ref, *target;\n> > + const char *r;\n> > + int flags;\n> > +\n> > + if (!head || !head->symref || !remote)\n> > + return;\n> > +\n> > + ref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n> > + target = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n>\n> But here we end up with this cast, as it's not const after all, we're\n> modifying it.\n\nIt is a const, and we are not modifying it. `apply_refspecs()` is not\nsaying what it should say: the refspec remains constant.\n\nAs Patrick explained: `apply_refspecs()` should probably be fixed.\n\n> > +\n> > + if (!ref || !target) {\n> > + warning(_(\"could not update remote head\"));\n> > + return;\n> > + }\n> > +\n> > + r = resolve_ref_unsafe(ref, 0, NULL, &flags);\n> > +\n> > + if (r) {\n> > + if (config == FETCH_UPDATE_HEAD_MISSING) {\n> > + if (flags & REF_ISSYMREF)\n> > + /* already present */\n> > + return;\n> > + } else if (config == FETCH_UPDATE_HEAD_ALWAYS) {\n> > + if (!strcmp(r, target))\n> > + /* already up-to-date */\n> > + return;\n>\n> I think you should name the \"enum\" you're adding below, the one that\n> contains the new \"FETCH_UPDATE_HEAD_DEFAULT\".\n>\n> Then this could be a \"switch\", and the compiler could check the\n> arguments, i.e. you could pass an enum type instead of an \"int\".\n\nSure, it can be an `enum fetch_update_mode` instead of `int`, but I\ndon't see what value it provides, other than more verbosity. The enum\nright above is also unnamed, and 'remote->origin' is an int. And it's\nnot the only enum of that kind in the source code.\n\nUsing a switch is better, but that doesn't require an enum type. The\nmultiple ifs are just a remnant of a previous version of the code.\n\n> > + } else\n>\n> {} missing, if you keep this, but...\n>\n> > + /* should never happen */\n> > + return;\n>\n> ...so, here we're not checking some enum values, but presumably other\n> things check this, I haven't checked.\n\nYes, the function cannot be called otherwise.\n\n> But for a \"should never happen\", should we make this a \"BUG()\", or is it\n> user-controlled?\n\nSure, it can be a `BUG()`. It truly should not happen.\n\n> > + }\n> > +\n> > + if (!create_symref(ref, target, \"remote update head\")) {\n> > + if (verbosity >= 0)\n> > + printf(_(\"Updated remote '%s' HEAD\\n\"), remote->name);\n> > + } else {\n> > + warning(_(\"could not update remote head\"));\n> > + }\n> > +}\n> > +\n> > static int do_fetch(struct transport *transport,\n> > struct refspec *rs)\n> > {\n> > @@ -1592,6 +1638,7 @@ static int do_fetch(struct transport *transport,\n> > int must_list_refs = 1;\n> > struct fetch_head fetch_head = { 0 };\n> > struct strbuf err = STRBUF_INIT;\n> > + int need_update_head = 0, update_head_config = 0;\n> >\n> > if (tags == TAGS_DEFAULT) {\n> > if (transport->remote->fetch_tags == 2)\n> > @@ -1626,9 +1673,21 @@ static int do_fetch(struct transport *transport,\n> > } else {\n> > struct branch *branch = branch_get(NULL);\n> >\n> > - if (transport->remote->fetch.nr)\n> > + if (transport->remote->fetch.nr) {\n> > +\n> > + if (transport->remote->update_head)\n> > + update_head_config = transport->remote->update_head;\n> > + else\n> > + update_head_config = fetch_update_head;\n> > +\n> > + need_update_head = update_head_config && update_head_config != FETCH_UPDATE_HEAD_NEVER;\n> > +\n> > + if (need_update_head)\n> > + strvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n> > refspec_ref_prefixes(&transport->remote->fetch,\n> > &transport_ls_refs_options.ref_prefixes);\n> > + }\n> > +\n> > if (branch_has_merge_config(branch) &&\n> > !strcmp(branch->remote_name, transport->remote->name)) {\n> > int i;\n> > @@ -1737,6 +1796,9 @@ static int do_fetch(struct transport *transport,\n> >\n> > commit_fetch_head(&fetch_head);\n> >\n> > + if (need_update_head)\n> > + update_head(update_head_config, find_ref_by_name(remote_refs, \"HEAD\"), transport->remote);\n>\n> Some overly long lines here...\n\nNot unique in this document:\n\n117: warning(_(\"rejected %s\nbecause shallow roots are not allowed to be updated\"),\n115: warning(_(\"multiple\nbranches detected, incompatible with --set-upstream\"));\n112: OPT_STRING_LIST('o', \"server-option\", &server_options,\nN_(\"server-specific\"), N_(\"option to transmit\")),\n111: need_update_head = update_head_config &&\nupdate_head_config != FETCH_UPDATE_HEAD_NEVER;\n108: \"you need to specify exactly\none branch with the --set-upstream option\"));\n106: die(_(\"options '%s' and '%s' cannot be\nused together\"), \"--depth\", \"--unshallow\");\n106: update_head(update_head_config,\nfind_ref_by_name(remote_refs, \"HEAD\"), transport->remote);\n103: die(_(\"fetching a group and\nspecifying refspecs does not make sense\"));\n103: die(_(\"options '%s' and '%s' cannot be\nused together\"), \"--deepen\", \"--depth\");\n103: warning(_(\"not setting upstream\nfor a remote remote-tracking branch\"));\n\nBut I seem to recall previous discussions (perhaps in LKML) where\npeople accepted that lines 120-characters long are OK. We don't live\nin the 80's anymore, terminals have more than 80 columns.\n\n> > +\n> > if (set_upstream) {\n> > struct branch *branch = branch_get(\"HEAD\");\n> > struct ref *rm;\n> > diff --git a/remote.c b/remote.c\n> > index 641b083d90..5f3a9aa53e 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -344,6 +344,25 @@ static void read_branches_file(struct remote_state *remote_state,\n> > remote->fetch_tags = 1; /* always auto-follow */\n> > }\n> >\n> > +int parse_update_head(int *r, const char *var, const char *value)\n> > +{\n> > + if (!r)\n> > + return -1;\n> > + else if (!value)\n> > + return config_error_nonbool(var);\n> > + else if (!strcmp(value, \"never\"))\n> > + *r = FETCH_UPDATE_HEAD_NEVER;\n> > + else if (!strcmp(value, \"missing\"))\n> > + *r = FETCH_UPDATE_HEAD_MISSING;\n> > + else if (!strcmp(value, \"always\"))\n> > + *r = FETCH_UPDATE_HEAD_ALWAYS;\n>\n> Ditto, this could really benefit from an enum type, instead of the bare\n> \"int\".\n\nWhat would change other than `int *r` -> `enum name *r`?\n\n> > + else {\n> > + error(_(\"malformed value for %s: %s\"), var, value);\n> > + return error(_(\"must be one of never, missing, or always\"));\n>\n> Shouldn't we use git_die_config() or similar here, to get the line\n> number etc., or do we get that somehow (I can't recall).\n\nThere's plenty of `error()` in config.c, including\n`git_default_push_config`, which this was based on.\n\n> > + }\n> > + return 0;\n> > +}\n> > +\n> > static int handle_config(const char *key, const char *value, void *cb)\n> > {\n> > const char *name;\n> > @@ -473,6 +492,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n> > key, value);\n> > } else if (!strcmp(subkey, \"vcs\")) {\n> > return git_config_string(&remote->foreign_vcs, key, value);\n> > + } else if (!strcmp(subkey, \"updatehead\")) {\n> > + return parse_update_head(&remote->update_head, key, value);\n> > }\n> > return 0;\n> > }\n> > diff --git a/remote.h b/remote.h\n> > index 73638cefeb..9dce42d65d 100644\n> > --- a/remote.h\n> > +++ b/remote.h\n> > @@ -22,6 +22,13 @@ enum {\n> > REMOTE_BRANCHES\n> > };\n> >\n> > +enum {\n> > + FETCH_UPDATE_HEAD_DEFAULT = 0,\n>\n> We tend to only init these to 0 when the default being 0 matters,\n> i.e. we use it as a boolean, but is that the case here?\n\nIn the current version of the code it doesn't matter, but the default\ncould be different later on.\n\nFor example if the default is not specified `!fetch_update_head` the\ncode could do some guessing, like doing \"always\" if the remote is a\nmirror.\n\nI learned this lesson reorganizing the options of `builtin/pull.c` in\na patch that was never merged. [1]\n\n> > +\n> > #endif\n> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> > index dc44da9c79..dbeb2928ae 100755\n> > --- a/t/t5510-fetch.sh\n> > +++ b/t/t5510-fetch.sh\n> > @@ -814,6 +814,37 @@ test_expect_success 'fetch from multiple configured URLs in single remote' '\n> > git fetch multipleurls\n> > '\n> >\n> > +test_cmp_symbolic_ref () {\n> > + git symbolic-ref \"$1\" >actual &&\n> > + echo \"$2\" >expected &&\n> > + test_cmp expected actual\n> > +}\n>\n> Sort of an aside, but this seems to be the Nth use of this pattern in\n> the test suite, e.g. t1401-symbolic-ref.sh repeatedly hardcodes the\n> same.\n>\n> I wonder if a prep commit to stick this in test-lib-functions.sh would\n> be in order, or maybe a \"--symbolic\" argument to \"test_cmp_rev\"?\n\nSure. If I had incline that such a patch would be merged (or this one)\nI would do it, but I have a plethora of cleanup patches just gathering\ndust, so I'd rather not.\n\nCheers.\n\n[1] https://lore.kernel.org/git/20210705123209.1808663-24-felipe.contreras@gmail.com/\n\n-- \nFelipe Contreras\n"},{"id":"474911","messageId":"230406.867cupv3n1.gmgdl@evledraar.gmail.com","threadId":"59546","inReplyTo":"CAMP44s0oBKfp6bXbg_+vp4CuRj_nh8uDBTCeT65z7UCUzj4K0Q@mail.gmail.com","subject":"Re: [PATCH 1/2] Add fetch.updateHead option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-04-06T07:33:55Z","receivedAt":"2023-04-06T08:04:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 05 2023, Felipe Contreras wrote:\n\n> On Wed, Apr 5, 2023 at 4:28 AM Ævar Arnfjörð Bjarmason wrote:\n>>\n>>\n>> On Tue, Apr 04 2023, Felipe Contreras wrote:\n>>\n>> > Users might change the behavior when running \"git fetch\" so that the\n>> > remote's HEAD symbolic ref is updated at certain point.\n>> >\n>> > For example after running \"git remote add\" the remote HEAD is not\n>> > set like it is with \"git clone\".\n>> >\n>> > Setting \"fetch.updatehead = missing\" would probably be a sensible\n>> > default that everyone would want, but for now the default behavior is to\n>> > never update HEAD, so there shouldn't be any functional changes.\n>> >\n>> > For the next major version of Git, we might want to change this default.\n>> >\n>> > Signed-off-by: Felipe Contreras \n>> > ---\n>> > Documentation/config/fetch.txt | 4 +++\n>> > Documentation/config/remote.txt | 3 ++\n>> > builtin/fetch.c | 64 ++++++++++++++++++++++++++++++++-\n>> > remote.c | 21 +++++++++++\n>> > remote.h | 11 ++++++\n>> > t/t5510-fetch.sh | 31 ++++++++++++++++\n>> > 6 files changed, 133 insertions(+), 1 deletion(-)\n>> >\n>> > diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\n>> > index 568f0f75b3..dc147ffb35 100644\n>> > --- a/Documentation/config/fetch.txt\n>> > +++ b/Documentation/config/fetch.txt\n>> > @@ -120,3 +120,7 @@ fetch.bundleCreationToken::\n>> > The creation token values are chosen by the provider serving the specific\n>> > bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to\n>> > remove the value for the `fetch.bundleCreationToken` value before fetching.\n>> > +\n>> > +fetch.updateHead::\n>> > + Defines when to update the remote HEAD symbolic ref. Values are 'never',\n>> > + 'missing' (update only when HEAD is missing), and 'always'.\n>> > diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt\n>> > index 0678b4bcfe..9d739d2ed4 100644\n>> > --- a/Documentation/config/remote.txt\n>> > +++ b/Documentation/config/remote.txt\n>> > @@ -86,3 +86,6 @@ remote..partialclonefilter::\n>> > Changing or clearing this value will only affect fetches for new commits.\n>> > To fetch associated objects for commits already present in the local object\n>> > database, use the `--refetch` option of linkgit:git-fetch[1].\n>> > +\n>> > +remote..updateHead::\n>> > + Defines when to update the remote HEAD symbolic ref. See `fetch.updateHead`.\n>> > diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> > index 7221e57f35..7e93a1aa46 100644\n>> > --- a/builtin/fetch.c\n>> > +++ b/builtin/fetch.c\n>> > @@ -59,6 +59,8 @@ 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>> > +static int fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;\n>> > +\n>> > static int all, append, dry_run, force, keep, multiple, update_head_ok;\n>> > static int write_fetch_head = 1;\n>> > static int verbosity, deepen_relative, set_upstream, refetch;\n>> > @@ -129,6 +131,9 @@ static int git_fetch_config(const char *k, const char *v, void *cb)\n>> > return 0;\n>> > }\n>> >\n>> > + if (!strcmp(k, \"fetch.updatehead\"))\n>> > + return parse_update_head(&fetch_update_head, k, v);\n>> > +\n>> > return git_default_config(k, v, cb);\n>> > }\n>> >\n>> > @@ -1579,6 +1584,47 @@ static int backfill_tags(struct transport *transport,\n>> > return retcode;\n>> > }\n>> >\n>> > +static void update_head(int config, const struct ref *head, const struct remote *remote)\n>>\n>> Here you pass a \"const struct remote\".\n>>\n>> > +{\n>> > + char *ref, *target;\n>> > + const char *r;\n>> > + int flags;\n>> > +\n>> > + if (!head || !head->symref || !remote)\n>> > + return;\n>> > +\n>> > + ref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n>> > + target = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n>>\n>> But here we end up with this cast, as it's not const after all, we're\n>> modifying it.\n>\n> It is a const, and we are not modifying it. `apply_refspecs()` is not\n> saying what it should say: the refspec remains constant.\n>\n> As Patrick explained: `apply_refspecs()` should probably be fixed.\n\nYes, that's a much better fix. I'd assumed that we were altering it, but\na prep change like that to give it \"const\" would be much better, then we\ncan avoid the cast.\n\n>> > +\n>> > + if (!ref || !target) {\n>> > + warning(_(\"could not update remote head\"));\n>> > + return;\n>> > + }\n>> > +\n>> > + r = resolve_ref_unsafe(ref, 0, NULL, &flags);\n>> > +\n>> > + if (r) {\n>> > + if (config == FETCH_UPDATE_HEAD_MISSING) {\n>> > + if (flags & REF_ISSYMREF)\n>> > + /* already present */\n>> > + return;\n>> > + } else if (config == FETCH_UPDATE_HEAD_ALWAYS) {\n>> > + if (!strcmp(r, target))\n>> > + /* already up-to-date */\n>> > + return;\n>>\n>> I think you should name the \"enum\" you're adding below, the one that\n>> contains the new \"FETCH_UPDATE_HEAD_DEFAULT\".\n>>\n>> Then this could be a \"switch\", and the compiler could check the\n>> arguments, i.e. you could pass an enum type instead of an \"int\".\n>\n> Sure, it can be an `enum fetch_update_mode` instead of `int`, but I\n> don't see what value it provides, other than more verbosity. The enum\n> right above is also unnamed, and 'remote->origin' is an int. And it's\n> not the only enum of that kind in the source code.\n>\n> Using a switch is better, but that doesn't require an enum type. The\n> multiple ifs are just a remnant of a previous version of the code.\n\nMore on this below, but it's for self-documentation (makes the code\neasier to follow), and the compiler can notice missing \"case\" arms,\nwhich isn't the case with an \"int\".\n\n>> > + } else\n>>\n>> {} missing, if you keep this, but...\n>>\n>> > + /* should never happen */\n>> > + return;\n>>\n>> ...so, here we're not checking some enum values, but presumably other\n>> things check this, I haven't checked.\n>\n> Yes, the function cannot be called otherwise.\n\n...more on this below...\n>\n>> But for a \"should never happen\", should we make this a \"BUG()\", or is it\n>> user-controlled?\n>\n> Sure, it can be a `BUG()`. It truly should not happen.\n\n...more on this below...\n\n>> > + }\n>> > +\n>> > + if (!create_symref(ref, target, \"remote update head\")) {\n>> > + if (verbosity >= 0)\n>> > + printf(_(\"Updated remote '%s' HEAD\\n\"), remote->name);\n>> > + } else {\n>> > + warning(_(\"could not update remote head\"));\n>> > + }\n>> > +}\n>> > +\n>> > static int do_fetch(struct transport *transport,\n>> > struct refspec *rs)\n>> > {\n>> > @@ -1592,6 +1638,7 @@ static int do_fetch(struct transport *transport,\n>> > int must_list_refs = 1;\n>> > struct fetch_head fetch_head = { 0 };\n>> > struct strbuf err = STRBUF_INIT;\n>> > + int need_update_head = 0, update_head_config = 0;\n>> >\n>> > if (tags == TAGS_DEFAULT) {\n>> > if (transport->remote->fetch_tags == 2)\n>> > @@ -1626,9 +1673,21 @@ static int do_fetch(struct transport *transport,\n>> > } else {\n>> > struct branch *branch = branch_get(NULL);\n>> >\n>> > - if (transport->remote->fetch.nr)\n>> > + if (transport->remote->fetch.nr) {\n>> > +\n>> > + if (transport->remote->update_head)\n>> > + update_head_config = transport->remote->update_head;\n>> > + else\n>> > + update_head_config = fetch_update_head;\n>> > +\n>> > + need_update_head = update_head_config && update_head_config != FETCH_UPDATE_HEAD_NEVER;\n>> > +\n>> > + if (need_update_head)\n>> > + strvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n>> > refspec_ref_prefixes(&transport->remote->fetch,\n>> > &transport_ls_refs_options.ref_prefixes);\n>> > + }\n>> > +\n>> > if (branch_has_merge_config(branch) &&\n>> > !strcmp(branch->remote_name, transport->remote->name)) {\n>> > int i;\n>> > @@ -1737,6 +1796,9 @@ static int do_fetch(struct transport *transport,\n>> >\n>> > commit_fetch_head(&fetch_head);\n>> >\n>> > + if (need_update_head)\n>> > + update_head(update_head_config, find_ref_by_name(remote_refs, \"HEAD\"), transport->remote);\n>>\n>> Some overly long lines here...\n>\n> Not unique in this document:\n\nYes...\n\n> 117: warning(_(\"rejected %s\n> because shallow roots are not allowed to be updated\"),\n> 115: warning(_(\"multiple\n> branches detected, incompatible with --set-upstream\"));\n> 112: OPT_STRING_LIST('o', \"server-option\", &server_options,\n> N_(\"server-specific\"), N_(\"option to transmit\")),\n\n...I think we have an informal exception for longer strings more often than not...\n\n> 111: need_update_head = update_head_config &&\n> update_head_config != FETCH_UPDATE_HEAD_NEVER;\n\n...here's another thing you're adding in these proposed patches, so that doesn't really count...\n\n> 108: \"you need to specify exactly\n> one branch with the --set-upstream option\"));\n> 106: die(_(\"options '%s' and '%s' cannot be\n> used together\"), \"--depth\", \"--unshallow\");\n\n..more strings...\n\n> 106: update_head(update_head_config,\n> find_ref_by_name(remote_refs, \"HEAD\"), transport->remote);\n\n...ditto stuff you're adding...\n\n> 103: die(_(\"fetching a group and\n> specifying refspecs does not make sense\"));\n> 103: die(_(\"options '%s' and '%s' cannot be\n> used together\"), \"--deepen\", \"--depth\");\n> 103: warning(_(\"not setting upstream\n> for a remote remote-tracking branch\"));\n\n...some strings...\n\n> But I seem to recall previous discussions (perhaps in LKML) where\n> people accepted that lines 120-characters long are OK. We don't live\n> in the 80's anymore, terminals have more than 80 columns.\n\nI don't know what the kernel does, but we try to conform to our\nCodingGuidelines, which sets a limit of 80.\n\nBut whatever else we do, we don't generally say that a newly added\nfunction to a given file should be exempted from the preferred coding\nstyle because the file isn't consistently using it.\n\n>> > +\n>> > if (set_upstream) {\n>> > struct branch *branch = branch_get(\"HEAD\");\n>> > struct ref *rm;\n>> > diff --git a/remote.c b/remote.c\n>> > index 641b083d90..5f3a9aa53e 100644\n>> > --- a/remote.c\n>> > +++ b/remote.c\n>> > @@ -344,6 +344,25 @@ static void read_branches_file(struct remote_state *remote_state,\n>> > remote->fetch_tags = 1; /* always auto-follow */\n>> > }\n>> >\n>> > +int parse_update_head(int *r, const char *var, const char *value)\n>> > +{\n>> > + if (!r)\n>> > + return -1;\n>> > + else if (!value)\n>> > + return config_error_nonbool(var);\n>> > + else if (!strcmp(value, \"never\"))\n>> > + *r = FETCH_UPDATE_HEAD_NEVER;\n>> > + else if (!strcmp(value, \"missing\"))\n>> > + *r = FETCH_UPDATE_HEAD_MISSING;\n>> > + else if (!strcmp(value, \"always\"))\n>> > + *r = FETCH_UPDATE_HEAD_ALWAYS;\n>>\n>> Ditto, this could really benefit from an enum type, instead of the bare\n>> \"int\".\n>\n> What would change other than `int *r` -> `enum name *r`?\n\nMore on that below...\n\n>> > + else {\n>> > + error(_(\"malformed value for %s: %s\"), var, value);\n>> > + return error(_(\"must be one of never, missing, or always\"));\n>>\n>> Shouldn't we use git_die_config() or similar here, to get the line\n>> number etc., or do we get that somehow (I can't recall).\n>\n> There's plenty of `error()` in config.c, including\n> `git_default_push_config`, which this was based on.\n\nAh, I see in these cases the config API handles emitting the bad line\nnumber, nevermind.\n\nAs an aside, I think we could avoid some of these \"malformed value\" if\nwe just made git_die_config_linenr() slighly smarter, and had it print\nthe bad value in cases where there's only one value, but that's\nunrelated.\n\n>> > + }\n>> > + return 0;\n>> > +}\n>> > +\n>> > static int handle_config(const char *key, const char *value, void *cb)\n>> > {\n>> > const char *name;\n>> > @@ -473,6 +492,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n>> > key, value);\n>> > } else if (!strcmp(subkey, \"vcs\")) {\n>> > return git_config_string(&remote->foreign_vcs, key, value);\n>> > + } else if (!strcmp(subkey, \"updatehead\")) {\n>> > + return parse_update_head(&remote->update_head, key, value);\n>> > }\n>> > return 0;\n>> > }\n>> > diff --git a/remote.h b/remote.h\n>> > index 73638cefeb..9dce42d65d 100644\n>> > --- a/remote.h\n>> > +++ b/remote.h\n>> > @@ -22,6 +22,13 @@ enum {\n>> > REMOTE_BRANCHES\n>> > };\n>> >\n>> > +enum {\n>> > + FETCH_UPDATE_HEAD_DEFAULT = 0,\n>>\n>> We tend to only init these to 0 when the default being 0 matters,\n>> i.e. we use it as a boolean, but is that the case here?\n>\n> In the current version of the code it doesn't matter, but the default\n> could be different later on.\n>\n> For example if the default is not specified `!fetch_update_head` the\n> code could do some guessing, like doing \"always\" if the remote is a\n> mirror.\n>\n> I learned this lesson reorganizing the options of `builtin/pull.c` in\n> a patch that was never merged. [1]\n>\n>> > +\n>> > #endif\n>> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> > index dc44da9c79..dbeb2928ae 100755\n>> > --- a/t/t5510-fetch.sh\n>> > +++ b/t/t5510-fetch.sh\n>> > @@ -814,6 +814,37 @@ test_expect_success 'fetch from multiple configured URLs in single remote' '\n>> > git fetch multipleurls\n>> > '\n>> >\n>> > +test_cmp_symbolic_ref () {\n>> > + git symbolic-ref \"$1\" >actual &&\n>> > + echo \"$2\" >expected &&\n>> > + test_cmp expected actual\n>> > +}\n>>\n>> Sort of an aside, but this seems to be the Nth use of this pattern in\n>> the test suite, e.g. t1401-symbolic-ref.sh repeatedly hardcodes the\n>> same.\n>>\n>> I wonder if a prep commit to stick this in test-lib-functions.sh would\n>> be in order, or maybe a \"--symbolic\" argument to \"test_cmp_rev\"?\n>\n> Sure. If I had incline that such a patch would be merged (or this one)\n> I would do it, but I have a plethora of cleanup patches just gathering\n> dust, so I'd rather not.\n\nFair enough, thanks.\n\nRe the \"more below\" above, I tried hacking some of what I suggested\nupthread on top of your patches, here's the result of\nthat. Changes/commentary:\n\n * Switched the \"int\" to \"enum\"\n\n * You've prepared the parse_update_head() to accept a NULL \"r\", but as\n this & your other code shows, we never pass it NULL. I don't get why\n we'd have it handle that case, as surely all plausible users are\n \"populate this config variable for me\", no?\n\n * I think better than a BUG() call in the new update_head() we should\n just drop \"need_update_head\" entirely. It ends up just being a\n variable that states \"is missing or always\", so for update_head() we\n can just pass a boolean \"missing?\".\n\n The two added \"switch\" statements are a bit verbose, I mainly\n included them to show what the pre-image is implicitly assuming with\n the \"update_head_config && ...\", and that you init the\n \"update_head_config\" to \"0\", instead of using\n \"FETCH_UPDATE_HEAD_DEFAULT\".\n\n * I renamed \"update_head\" to \"fetch_update_head\" just to have the\n compiler catch cases where we were using the old \"int\", but if you\n find some of this useful we could keep the old name.\n\nHope some of that helps.\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 6bf147b0123..6492e88d779 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -59,7 +59,7 @@ 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-static int fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;\n+static enum fetch_update_head fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;\n \n static int all, append, dry_run, force, keep, multiple, update_head_ok;\n static int write_fetch_head = 1;\n@@ -1584,7 +1584,8 @@ static int backfill_tags(struct transport *transport,\n \treturn retcode;\n }\n \n-static void update_head(int config, const struct ref *head, const struct remote *remote)\n+static void update_head(int fetch_missing, const struct ref *head,\n+\t\t\tstruct remote *remote)\n {\n \tchar *ref, *target;\n \tconst char *r;\n@@ -1594,7 +1595,7 @@ static void update_head(int config, const struct ref *head, const struct remote\n \t\treturn;\n \n \tif (!remote->mirror) {\n-\t\tref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n+\t\tref = apply_refspecs(&remote->fetch, \"refs/heads/HEAD\");\n \t\ttarget = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n \n \t\tif (!ref || !target) {\n@@ -1609,17 +1610,14 @@ static void update_head(int config, const struct ref *head, const struct remote\n \tr = resolve_ref_unsafe(ref, 0, NULL, &flags);\n \n \tif (r) {\n-\t\tif (config == FETCH_UPDATE_HEAD_MISSING) {\n+\t\tif (fetch_missing) {\n \t\t\tif (flags & REF_ISSYMREF)\n \t\t\t\t/* already present */\n \t\t\t\treturn;\n-\t\t} else if (config == FETCH_UPDATE_HEAD_ALWAYS) {\n-\t\t\tif (!strcmp(r, target))\n-\t\t\t\t/* already up-to-date */\n-\t\t\t\treturn;\n-\t\t} else\n-\t\t\t/* should never happen */\n+\t\t} else if (!strcmp(r, target)) {\n+\t\t\t/* already up-to-date */\n \t\t\treturn;\n+\t\t}\n \t}\n \n \tif (!create_symref(ref, target, \"remote update head\")) {\n@@ -1643,7 +1641,7 @@ static int do_fetch(struct transport *transport,\n \tint must_list_refs = 1;\n \tstruct fetch_head fetch_head = { 0 };\n \tstruct strbuf err = STRBUF_INIT;\n-\tint need_update_head = 0, update_head_config = 0;\n+\tenum fetch_update_head update_head_config = FETCH_UPDATE_HEAD_DEFAULT;\n \n \tif (tags == TAGS_DEFAULT) {\n \t\tif (transport->remote->fetch_tags == 2)\n@@ -1680,15 +1678,19 @@ static int do_fetch(struct transport *transport,\n \n \t\tif (transport->remote->fetch.nr) {\n \n-\t\t\tif (transport->remote->update_head)\n-\t\t\t\tupdate_head_config = transport->remote->update_head;\n+\t\t\tif (transport->remote->fetch_update_head != FETCH_UPDATE_HEAD_DEFAULT)\n+\t\t\t\tupdate_head_config = transport->remote->fetch_update_head;\n \t\t\telse\n \t\t\t\tupdate_head_config = fetch_update_head;\n \n-\t\t\tneed_update_head = update_head_config && update_head_config != FETCH_UPDATE_HEAD_NEVER;\n-\n-\t\t\tif (need_update_head)\n+\t\t\tswitch (update_head_config) {\n+\t\t\tcase FETCH_UPDATE_HEAD_MISSING:\n+\t\t\tcase FETCH_UPDATE_HEAD_ALWAYS:\n \t\t\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n+\t\t\tcase FETCH_UPDATE_HEAD_DEFAULT:\n+\t\t\tcase FETCH_UPDATE_HEAD_NEVER:\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t\trefspec_ref_prefixes(&transport->remote->fetch,\n \t\t\t\t\t &transport_ls_refs_options.ref_prefixes);\n \t\t}\n@@ -1801,8 +1803,16 @@ static int do_fetch(struct transport *transport,\n \n \tcommit_fetch_head(&fetch_head);\n \n-\tif (need_update_head)\n-\t\tupdate_head(update_head_config, find_ref_by_name(remote_refs, \"HEAD\"), transport->remote);\n+\tswitch (update_head_config) {\n+\tcase FETCH_UPDATE_HEAD_MISSING:\n+\tcase FETCH_UPDATE_HEAD_ALWAYS:\n+\t\tupdate_head(update_head_config == FETCH_UPDATE_HEAD_MISSING,\n+\t\t\t find_ref_by_name(remote_refs, \"HEAD\"),\n+\t\t\t transport->remote);\n+\tcase FETCH_UPDATE_HEAD_DEFAULT:\n+\tcase FETCH_UPDATE_HEAD_NEVER:\n+\t\tbreak;\n+\t}\n \n \tif (set_upstream) {\n \t\tstruct branch *branch = branch_get(\"HEAD\");\ndiff --git a/remote.c b/remote.c\nindex 5f3a9aa53ec..c05c344d806 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -344,11 +344,10 @@ static void read_branches_file(struct remote_state *remote_state,\n \tremote->fetch_tags = 1; /* always auto-follow */\n }\n \n-int parse_update_head(int *r, const char *var, const char *value)\n+int parse_update_head(enum fetch_update_head *r, const char *var,\n+\t\t const char *value)\n {\n-\tif (!r)\n-\t\treturn -1;\n-\telse if (!value)\n+\tif (!value)\n \t\treturn config_error_nonbool(var);\n \telse if (!strcmp(value, \"never\"))\n \t\t*r = FETCH_UPDATE_HEAD_NEVER;\n@@ -493,7 +492,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t} else if (!strcmp(subkey, \"vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t} else if (!strcmp(subkey, \"updatehead\")) {\n-\t\treturn parse_update_head(&remote->update_head, key, value);\n+\t\treturn parse_update_head(&remote->fetch_update_head, key,\n+\t\t\t\t\t value);\n \t}\n \treturn 0;\n }\ndiff --git a/remote.h b/remote.h\nindex 9dce42d65d0..80b8cc24b6b 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -22,8 +22,8 @@ enum {\n \tREMOTE_BRANCHES\n };\n \n-enum {\n-\tFETCH_UPDATE_HEAD_DEFAULT = 0,\n+enum fetch_update_head {\n+\tFETCH_UPDATE_HEAD_DEFAULT,\n \tFETCH_UPDATE_HEAD_NEVER,\n \tFETCH_UPDATE_HEAD_MISSING,\n \tFETCH_UPDATE_HEAD_ALWAYS,\n@@ -104,7 +104,7 @@ struct remote {\n \tint prune;\n \tint prune_tags;\n \n-\tint update_head;\n+\tenum fetch_update_head fetch_update_head;\n \n \t/**\n \t * The configured helper programs to run on the remote side, for\n@@ -458,6 +458,7 @@ void apply_push_cas(struct push_cas_option *, struct remote *, struct ref *);\n char *relative_url(const char *remote_url, const char *url,\n \t\t const char *up_path);\n \n-int parse_update_head(int *r, const char *var, const char *value);\n+int parse_update_head(enum fetch_update_head *r, const char *var,\n+\t\t const char *value);\n \n #endif\n"},{"id":"474968","messageId":"642f82d4153e5_9afe29469@chronos.notmuch","threadId":"59546","inReplyTo":"230406.867cupv3n1.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] Add fetch.updateHead option","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2023-04-07T02:41:24Z","receivedAt":"2023-04-07T02:41:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Wed, Apr 05 2023, Felipe Contreras wrote:\n> > On Wed, Apr 5, 2023 at 4:28 AM Ævar Arnfjörð Bjarmason wrote:\n> >> On Tue, Apr 04 2023, Felipe Contreras wrote:\n\n> >> > +\n> >> > + if (!ref || !target) {\n> >> > + warning(_(\"could not update remote head\"));\n> >> > + return;\n> >> > + }\n> >> > +\n> >> > + r = resolve_ref_unsafe(ref, 0, NULL, &flags);\n> >> > +\n> >> > + if (r) {\n> >> > + if (config == FETCH_UPDATE_HEAD_MISSING) {\n> >> > + if (flags & REF_ISSYMREF)\n> >> > + /* already present */\n> >> > + return;\n> >> > + } else if (config == FETCH_UPDATE_HEAD_ALWAYS) {\n> >> > + if (!strcmp(r, target))\n> >> > + /* already up-to-date */\n> >> > + return;\n> >>\n> >> I think you should name the \"enum\" you're adding below, the one that\n> >> contains the new \"FETCH_UPDATE_HEAD_DEFAULT\".\n> >>\n> >> Then this could be a \"switch\", and the compiler could check the\n> >> arguments, i.e. you could pass an enum type instead of an \"int\".\n> >\n> > Sure, it can be an `enum fetch_update_mode` instead of `int`, but I\n> > don't see what value it provides, other than more verbosity. The enum\n> > right above is also unnamed, and 'remote->origin' is an int. And it's\n> > not the only enum of that kind in the source code.\n> >\n> > Using a switch is better, but that doesn't require an enum type. The\n> > multiple ifs are just a remnant of a previous version of the code.\n> \n> More on this below, but it's for self-documentation (makes the code\n> easier to follow),\n\nI guess it *can* make the code easier to follow for some people, but only very\nmarginally, and certainly not for me.\n\n> and the compiler can notice missing \"case\" arms,\n> which isn't the case with an \"int\".\n\nYes, but that has never been useful in my experience.\n\n> > But I seem to recall previous discussions (perhaps in LKML) where\n> > people accepted that lines 120-characters long are OK. We don't live\n> > in the 80's anymore, terminals have more than 80 columns.\n> \n> I don't know what the kernel does, but we try to conform to our\n> CodingGuidelines, which sets a limit of 80.\n\nThere's a difference between claiming we try to conform to X, and actually\ntrying to conform to X. I think the evidence above shows it's not the latter.\n\nThat is: the guideline says something which isn't actually true. Or at least:\nif we are trying, we are not trying very hard.\n\n> But whatever else we do, we don't generally say that a newly added\n> function to a given file should be exempted from the preferred coding\n> style because the file isn't consistently using it.\n\nA guideline is not a law.\n\nIf the guideline says \"try to do X\", and a patch doesn't do X, that's not a\nvalid reason to reject it. It is not a prescriptive command, it has no \"shall\"\nor \"must\". It's merely a suggestion.\n\nAnd as I showed above, a suggestion that is clearly not followed to the tee in\nall the code base.\n\n> >> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> >> > index dc44da9c79..dbeb2928ae 100755\n> >> > --- a/t/t5510-fetch.sh\n> >> > +++ b/t/t5510-fetch.sh\n> >> > @@ -814,6 +814,37 @@ test_expect_success 'fetch from multiple configured URLs in single remote' '\n> >> > git fetch multipleurls\n> >> > '\n> >> >\n> >> > +test_cmp_symbolic_ref () {\n> >> > + git symbolic-ref \"$1\" >actual &&\n> >> > + echo \"$2\" >expected &&\n> >> > + test_cmp expected actual\n> >> > +}\n> >>\n> >> Sort of an aside, but this seems to be the Nth use of this pattern in\n> >> the test suite, e.g. t1401-symbolic-ref.sh repeatedly hardcodes the\n> >> same.\n> >>\n> >> I wonder if a prep commit to stick this in test-lib-functions.sh would\n> >> be in order, or maybe a \"--symbolic\" argument to \"test_cmp_rev\"?\n> >\n> > Sure. If I had incline that such a patch would be merged (or this one)\n> > I would do it, but I have a plethora of cleanup patches just gathering\n> > dust, so I'd rather not.\n> \n> Fair enough, thanks.\n> \n> Re the \"more below\" above, I tried hacking some of what I suggested\n> upthread on top of your patches, here's the result of\n> that. Changes/commentary:\n> \n> * Switched the \"int\" to \"enum\"\n\nFor the record: I still don't see any value in doing that.\n\nBut I also don't see any harm, so I'm OK with that change.\n\n> * You've prepared the parse_update_head() to accept a NULL \"r\", but as\n> this & your other code shows, we never pass it NULL. I don't get why\n> we'd have it handle that case, as surely all plausible users are\n> \"populate this config variable for me\", no?\n\nYeah, I don't see any value in checking that, it probably was already there\nfrom the original function I copied the code.\n\n> * I think better than a BUG() call in the new update_head() we should\n> just drop \"need_update_head\" entirely. It ends up just being a\n> variable that states \"is missing or always\", so for update_head() we\n> can just pass a boolean \"missing?\".\n\nActually, `need_update_head` doesn't equal \"is missing or always\": it mostly\ntracks the fact that we sent \"HEAD\" as part of the refspecs sent to the remote.\n\nFor example if you do `git fetch`, that sets `need_update_head`, but if you do\n`git fetch master` it does not (AFAIK).\n\nSo your change is not equivalent, and it would call update_head() unnecessarily\nin many instances where there is no \"HEAD\" coming back from the remote, so\n`struct ref *head` is NULL. That's not a big issue, since the function will\nsimply return in those cases.\n\nBut...\n\nAccording to Jeff King, there are some instances where \"HEAD\" is coming back\nfrom the server, even if we didn't request it, in those cases we would want the\nlocal \"remote/foo/HEAD\" to be updated as well (if configured).\n\nSo your change is not functionally equivalent: it's actually better. The reason\nI didn't implement the logic Jeff King suggested is that I didn't see a way to\ndo it without complicating the code, but your suggestion is the way.\n\n> * I renamed \"update_head\" to \"fetch_update_head\" just to have the\n> compiler catch cases where we were using the old \"int\", but if you\n> find some of this useful we could keep the old name.\n\nI do find a lot of this useful (bar the switch to an enum), but I think the old\nname is better, since it's a configuration that affects commands other than\n`git fetch`, for example `git remote update`.\n\n> Hope some of that helps.\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 6bf147b0123..6492e88d779 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -59,7 +59,7 @@ 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> -static int fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;\n> +static enum fetch_update_head fetch_update_head = FETCH_UPDATE_HEAD_DEFAULT;\n> \n> static int all, append, dry_run, force, keep, multiple, update_head_ok;\n> static int write_fetch_head = 1;\n> @@ -1584,7 +1584,8 @@ static int backfill_tags(struct transport *transport,\n> \treturn retcode;\n> }\n> \n> -static void update_head(int config, const struct ref *head, const struct remote *remote)\n> +static void update_head(int fetch_missing, const struct ref *head,\n> +\t\t\tstruct remote *remote)\n\nThis is good, it simplifies the logic below.\n\n> {\n> \tchar *ref, *target;\n> \tconst char *r;\n> @@ -1594,7 +1595,7 @@ static void update_head(int config, const struct ref *head, const struct remote\n> \t\treturn;\n> \n> \tif (!remote->mirror) {\n> -\t\tref = apply_refspecs((struct refspec *)&remote->fetch, \"refs/heads/HEAD\");\n> +\t\tref = apply_refspecs(&remote->fetch, \"refs/heads/HEAD\");\n> \t\ttarget = apply_refspecs((struct refspec *)&remote->fetch, head->symref);\n\nSmall nit: you didn't drop this casting.\n\n> \n> \t\tif (!ref || !target) {\n> @@ -1609,17 +1610,14 @@ static void update_head(int config, const struct ref *head, const struct remote\n> \tr = resolve_ref_unsafe(ref, 0, NULL, &flags);\n> \n> \tif (r) {\n> -\t\tif (config == FETCH_UPDATE_HEAD_MISSING) {\n> +\t\tif (fetch_missing) {\n> \t\t\tif (flags & REF_ISSYMREF)\n> \t\t\t\t/* already present */\n> \t\t\t\treturn;\n> -\t\t} else if (config == FETCH_UPDATE_HEAD_ALWAYS) {\n> -\t\t\tif (!strcmp(r, target))\n> -\t\t\t\t/* already up-to-date */\n> -\t\t\t\treturn;\n> -\t\t} else\n> -\t\t\t/* should never happen */\n> +\t\t} else if (!strcmp(r, target)) {\n> +\t\t\t/* already up-to-date */\n> \t\t\treturn;\n> +\t\t}\n\nI prefer to have the main logic (always) on top, but otherwise good.\n\n> \t}\n> \n> \tif (!create_symref(ref, target, \"remote update head\")) {\n> @@ -1643,7 +1641,7 @@ static int do_fetch(struct transport *transport,\n> \tint must_list_refs = 1;\n> \tstruct fetch_head fetch_head = { 0 };\n> \tstruct strbuf err = STRBUF_INIT;\n> -\tint need_update_head = 0, update_head_config = 0;\n> +\tenum fetch_update_head update_head_config = FETCH_UPDATE_HEAD_DEFAULT;\n> \n> \tif (tags == TAGS_DEFAULT) {\n> \t\tif (transport->remote->fetch_tags == 2)\n> @@ -1680,15 +1678,19 @@ static int do_fetch(struct transport *transport,\n> \n> \t\tif (transport->remote->fetch.nr) {\n> \n> -\t\t\tif (transport->remote->update_head)\n> -\t\t\t\tupdate_head_config = transport->remote->update_head;\n> +\t\t\tif (transport->remote->fetch_update_head != FETCH_UPDATE_HEAD_DEFAULT)\n\nIn Git codestyle implicit tends to be preferred over explicit (as is in the Linux codesyle)\n\n * `if (p)` over `if (p != NULL)`\n * `if (i)` over `if (i != 0)`\n * `if (!strcmp(...)` over `if (strcmp(...) == 0`\n\nAnd so on, so I think this strongly suggests this is preferred:\n\n if (transport->remote->update_head)\n\nOver\n\n if (transport->remote->fetch_update_head != FETCH_UPDATE_HEAD_DEFAULT)\n\n> +\t\t\t\tupdate_head_config = transport->remote->fetch_update_head;\n> \t\t\telse\n> \t\t\t\tupdate_head_config = fetch_update_head;\n> \n> -\t\t\tneed_update_head = update_head_config && update_head_config != FETCH_UPDATE_HEAD_NEVER;\n> -\n> -\t\t\tif (need_update_head)\n> +\t\t\tswitch (update_head_config) {\n> +\t\t\tcase FETCH_UPDATE_HEAD_MISSING:\n> +\t\t\tcase FETCH_UPDATE_HEAD_ALWAYS:\n> \t\t\t\tstrvec_push(&transport_ls_refs_options.ref_prefixes, \"HEAD\");\n> +\t\t\tcase FETCH_UPDATE_HEAD_DEFAULT:\n> +\t\t\tcase FETCH_UPDATE_HEAD_NEVER:\n\nI would rather have \"default:\" here to catch all the rest.\n\nI suppose this could be clearer to some people (although IMO overly verbose),\nbut this has nothing to do with the enum change, as it can be done with `int\nupdate_head_config`.\n\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> \t\t\trefspec_ref_prefixes(&transport->remote->fetch,\n> \t\t\t\t\t &transport_ls_refs_options.ref_prefixes);\n> \t\t}\n> @@ -1801,8 +1803,16 @@ static int do_fetch(struct transport *transport,\n> \n> \tcommit_fetch_head(&fetch_head);\n> \n> -\tif (need_update_head)\n> -\t\tupdate_head(update_head_config, find_ref_by_name(remote_refs, \"HEAD\"), transport->remote);\n> +\tswitch (update_head_config) {\n> +\tcase FETCH_UPDATE_HEAD_MISSING:\n> +\tcase FETCH_UPDATE_HEAD_ALWAYS:\n> +\t\tupdate_head(update_head_config == FETCH_UPDATE_HEAD_MISSING,\n> +\t\t\t find_ref_by_name(remote_refs, \"HEAD\"),\n> +\t\t\t transport->remote);\n> +\tcase FETCH_UPDATE_HEAD_DEFAULT:\n> +\tcase FETCH_UPDATE_HEAD_NEVER:\n> +\t\tbreak;\n> +\t}\n\nDitto. Although this isn't functionally equivalent, it's actually better.\n\n---\n\nI've integrated the important parts of these changes into v2 and sent that.\n\nI still don't see an incline of this patch ever being merged, so it's probably\njust an exercise, but for the record there it is.\n\nCheers.\n\n-- \nFelipe Contreras"}]}