{"thread":{"id":"57212","subject":"[PATCH] submodule--helper: check clone.defaultRemoteName in print-default-remote","startedAt":"2022-01-08T00:52:39Z","lastAt":"2022-01-08T01:00:05Z","messageCount":2,"participants":["Sean Barag via GitGitGadget","Sean Barag"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"445768","messageId":"pull.1112.git.1641603153667.gitgitgadget@gmail.com","threadId":"57212","inReplyTo":null,"subject":"[PATCH] submodule--helper: check clone.defaultRemoteName in print-default-remote","fromName":"Sean Barag via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-08T00:52:33Z","receivedAt":"2022-01-08T00:52:39Z","isPatch":true,"sender":{"key":"sean@barag.org","avatar":"https://avatars.githubusercontent.com/u/665775?v=4"},"body":"From: Sean Barag \n\n`git submodule update --remote` previously assumed that submodules in\ndetached-HEAD state (or with no remote configured via\nbranch.$branchname.remote) should always fetch from \"origin\". While this\nwas often true, users could rename the \"origin\" remote within that\nsubmodule. This would result in a pair of fatal errors:\n\n fatal: Needed a single revision\n fatal: Unable to find current origin/HEAD revision in submodule path\n 'path/to/submodule'\n\nSince de9ed3ef37 (clone: allow configurable default for `-o`/`--origin`,\n2020-10-01) (released in git 2.30) however, that edge-case was trivially\neasy to get into. Simply set `clone.defaultRemoteName`, clone a repo\nwith submodules, run `git submodule update --init`, and the next `git\nsubmodule update --remote` would fail. Support clone.defaultRemoteName\nin submodules by checking for the configured default remote name before\nfalling back to \"origin\".\n\nSigned-off-by: Sean Barag \n---\n [RFC PATCH] submodule--helper: check clone.defaultRemoteName in\n print-default-remote\n \n What used to be an edge case with git submodule update --remote got\n significantly easier to reproduce in 2.30+ with clone.defaultRemoteName\n set in git config. git-submodule.sh relies on git submodule--helper\n print-default-remote to determine which remote to fetch from, which\n falls back to \"origin\" when an explicit remote can't be found (e.g.\n detached HEAD states). Where clone.defaultRemoteName is set, however,\n that fallback to \"origin\" is no longer appropriate, because the \"origin\"\n remote doesn't exist. Either way, this results in a fatal error:\n \n fatal: Needed a single revision\n fatal: Unable to find current origin/HEAD revision in submodule path\n 'path/to/submodule'\n \n \n This RFC patch adds a check for clone.defaultRemoteName in\n submodule-helper's get_default_remote(), but would love some feedback\n before this becomes a proper [PATCH] email. This works in my initial\n testing and in the added test case, but it feels like I'm missing\n something.\n \n Should I add test cases that don't rely on detached HEAD state perhaps?\n I'm not thrilled with the duplication of git_config_get_string calls for\n clone.defaultRemoteName, but extracting that logic to a static char\n *get_clone_default_remote_name() felt worse. I'd be happy to have any\n feedback y'all can provide :)\n \n From a high-level, I suppose this leaves the pre-2.30 edge case\n unresolved: users can still rename a submodule's remote to something\n else (something != clone.defaultRemoteName if it's set, or something !=\n \"origin\" if it's not set) and trigger this failure. Perhaps a long-term\n solution would involve querying the list of remotes for each submodule,\n and if there's only one remote assuming it's the correct one to fetch\n from, with the proposed solution as a fallback for multi-remote\n situations.\n \n Anyway, please let me know what I can do to clean this up, make it more\n robust, etc. before it becomes a more formal patch submission. Thanks in\n advance! Sean Barag\n \n Related conversations:\n \n * The original bug report:\n https://lore.kernel.org/git/2d58fe40-9e8c-4653-8170-5411fd3cf6f4@www.fastmail.com\n \n * An adjacent conversation about submodule--helper and remote names:\n https://lore.kernel.org/git/ae3baa2a-2f30-8e8a-f3cf-d8210e7225b0@gmail.com\n \n Philippe Blain levraiphilippeblain@gmail.com, Atharva Raykar\n raykar.ath@gmail.com, Christian Couder christian.couder@gmail.com,\n Shourya Shukla periperidip@gmail.com, Emily Shaffer\n emilyshaffer@google.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1112%2Fsjbarag%2Fcheck-clone.defaultremotename-in-submodule-helper-print-default-remote-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1112/sjbarag/check-clone.defaultremotename-in-submodule-helper-print-default-remote-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1112\n\n builtin/submodule--helper.c | 35 +++++++++++++++++++++++++----------\n t/t7400-submodule-basic.sh | 18 ++++++++++++++++++\n 2 files changed, 43 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 9b25a508e6a..2d1468508ff 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -38,18 +38,33 @@ static char *get_default_remote(void)\n \tif (!refname)\n \t\tdie(_(\"No such ref: %s\"), \"HEAD\");\n \n-\t/* detached HEAD */\n-\tif (!strcmp(refname, \"HEAD\"))\n-\t\treturn xstrdup(\"origin\");\n+\t/* detached HEAD should use the configured default remote name or fall back to \"origin\" */\n+\tif (!strcmp(refname, \"HEAD\")) {\n+\t\tstrbuf_addstr(&sb, \"clone.defaultRemoteName\");\n \n-\tif (!skip_prefix(refname, \"refs/heads/\", &refname))\n-\t\tdie(_(\"Expecting a full ref name, got %s\"), refname);\n+\t\tif (!git_config_get_string(sb.buf, &dest))\n+\t\t\tret = dest;\n+\t\telse\n+\t\t\tret = xstrdup(\"origin\");\n+\t} else {\n+\t\tif (!skip_prefix(refname, \"refs/heads/\", &refname))\n+\t\t\tdie(_(\"Expecting a full ref name, got %s\"), refname);\n \n-\tstrbuf_addf(&sb, \"branch.%s.remote\", refname);\n-\tif (git_config_get_string(sb.buf, &dest))\n-\t\tret = xstrdup(\"origin\");\n-\telse\n-\t\tret = dest;\n+\t\tstrbuf_addf(&sb, \"branch.%s.remote\", refname);\n+\t\tif (!git_config_get_string(sb.buf, &dest))\n+\t\t\t/* use configured remote name if one is present */\n+\t\t\tret = dest;\n+\t\telse {\n+\t\t\t/* otherwise use configured *default* remote name, falling back to \"origin\" */\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\tstrbuf_addstr(&sb, \"clone.defaultRemoteName\");\n+\n+\t\t\tif (!git_config_get_string(sb.buf, &dest))\n+\t\t\t\tret = dest;\n+\t\t\telse\n+\t\t\t\tret = xstrdup(\"origin\");\n+\t\t}\n+\t}\n \n \tstrbuf_release(&sb);\n \treturn ret;\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex e7cec2e457a..ce0801321a9 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -1416,4 +1416,22 @@ test_expect_success 'recursive clone respects -q' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'submodule update --remote respects clone.defaultRemoteName' '\n+\ttest_when_finished \"rm -rf multisuper_clone\" &&\n+\n+\t# expect is intentionally empty.\n+\t# no output expected, since submodule \"sub0\" will be up to date\n+\t> expect &&\n+\n+\tgit clone multisuper multisuper_clone &&\n+\tgit -C multisuper_clone submodule update --init -- sub0 &&\n+\tgit -C multisuper_clone/sub0 remote rename origin upstream &&\n+\tgit \\\n+\t\t-C multisuper_clone \\\n+\t\t-c clone.defaultRemoteName=upstream \\\n+\t\tsubmodule update --remote -- sub0 \\\n+\t\t2>&1 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: e83ba647f7c61cf945690d6a0bd8c172a6498dc8\n-- \ngitgitgadget\n"},{"id":"445770","messageId":"20220108010002.43869-1-barag@cockroachlabs.com","threadId":"57212","inReplyTo":"pull.1112.git.1641603153667.gitgitgadget@gmail.com","subject":"Re: [RFC PATCH] submodule--helper: check clone.defaultRemoteName in print-default-remote","fromName":"Sean Barag","fromEmail":"barag@cockroachlabs.com","sentAt":"2022-01-08T01:00:02Z","receivedAt":"2022-01-08T01:00:05Z","isPatch":true,"sender":{"key":"barag@cockroachlabs.com","avatar":null},"body":"Whoops, my CC list got mangled. Fixed.\n"}]}