{"thread":{"id":"51193","subject":"git-clone --config remote.origin.fetch regression","startedAt":"2019-05-28T22:35:13Z","lastAt":"2019-05-29T13:20:25Z","messageCount":2,"participants":["Han-Wen Nienhuys","SZEDER Gábor"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"376368","messageId":"CAFQ2z_O7rupGS4rntiUwJCHUj3bO0U88bVAToveEo_uU0Eu96w@mail.gmail.com","threadId":"51193","inReplyTo":null,"subject":"git-clone --config remote.origin.fetch regression","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2019-05-28T22:34:58Z","receivedAt":"2019-05-28T22:35:13Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"(see also https://github.com/google/zoekt/issues/81)\n\nIt looks like git 2.21 included a regression. The command\n\ngit clone --bare --progress \\\n  --config \"remote.origin.fetch=+refs/heads/*:refs/heads/*\" \\\n  https://github.com/google/zoekt.git \\\n  /tmp/zoekt-git2.20.git\n\nwould succeed with git 2.20, but fails with\n\n fatal: multiple updates for ref 'refs/heads/master' not allowed\n\nin git 2.21, probably caused by commit 515be83.\n\nShould I call git in another way? I originally included\n\"remote.origin.fetch=+refs/heads/*:refs/heads/*\" to avoid getting\nGerrit refs (refs/changes/*), but maybe I should use a different\nincantation?\n\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"376398","messageId":"20190529132017.GM951@szeder.dev","threadId":"51193","inReplyTo":"CAFQ2z_O7rupGS4rntiUwJCHUj3bO0U88bVAToveEo_uU0Eu96w@mail.gmail.com","subject":"Re: git-clone --config remote.origin.fetch regression","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-05-29T13:20:17Z","receivedAt":"2019-05-29T13:20:25Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, May 28, 2019 at 07:34:58PM -0300, Han-Wen Nienhuys wrote:\n> (see also https://github.com/google/zoekt/issues/81)\n> \n> It looks like git 2.21 included a regression. The command\n> \n> git clone --bare --progress \\\n>   --config \"remote.origin.fetch=+refs/heads/*:refs/heads/*\" \\\n>   https://github.com/google/zoekt.git \\\n>   /tmp/zoekt-git2.20.git\n> \n> would succeed with git 2.20, but fails with\n> \n>  fatal: multiple updates for ref 'refs/heads/master' not allowed\n> \n> in git 2.21, probably caused by commit 515be83.\n> \n> Should I call git in another way? I originally included\n> \"remote.origin.fetch=+refs/heads/*:refs/heads/*\" to avoid getting\n> Gerrit refs (refs/changes/*), but maybe I should use a different\n> incantation?\n\nOn first sight I was wondering why you don't use '--mirror', but yeah,\nthat would fetch 'refs/changes/*' as well (the whole 'refs/*',\nreally).  In the meantime a workaround would be to run separate\ncommands to accomplish what 'git clone' would do under the hood:\n\n  git init --bare zoekt.git\n  cd zoekt.git\n  git config remote.origin.url https://github.com/google/zoekt.git\n  git config remote.origin.fetch '+refs/heads/*:refs/heads/*'\n  git fetch\n\n\nThis is indeed caused by 515be83382 (clone: respect additional\nconfigured fetch refspecs during initial fetch, 2018-11-14): since \nthen the same refspec comes up twice in remote->fetch, once from the\nconfiguration that you specified on the command line and once from the\ndefault refspec that 'git clone' would have used anyway, and in the\nend 'git clone' tries to write each ref twice, once for each of those\ntwo refspecs, in a single ref transaction, hence the error.\n\nI'm not quite sure how to properly handle the situation.  The patch at\nthe bottom makes your case (and the case in an earlier report [1])\nwork by omitting 'git clone's default refspec if one of the configured\nrefspecs have the same destination side as the default refspec.  I\nthink this is a step in the right-ish direction, but there are some\nopen questions:\n\n  - Should it only check the destination side of the refspecs, or the\n    source as well?  IOW, in case of e.g.\n\n      git clone --bare \\\n                -c 'remote.origin.fetch=refs/foo/*:refs/heads/*' ...\n\n    should it only fetch from 'refs/foo/*' or both from 'refs/foo/*'\n    and 'refs/heads/*'?  With the patch below it's only 'refs/foo/*'.\n    If we were to fetch from both, then I expect trouble when both\n    'refs/foo/feature' and 'refs/heads/feature' happen to exist, but\n    this is a more general issue not limited to 'git clone -c ...'.\n\n  - Even if it were to check the source side of the refspec, it should\n    ignore the optional leading '+' in the refspecs, because otherwise\n    the command\n\n      # no leading '+' in the additional configured refspec\n      git clone --bare \\\n                -c 'remote.origin.fetch=refs/heads/*:refs/heads/*' ...\n\n    would lead to the same error, because the default refspec for\n    '--bare' is '+refs/heads/*:refs/heads/*'.\n\n    (Sidenote: 'git clone's default refspec always has the leading\n    '+', but I think that's unnecessary, because during cloning there\n    are no existing refs to be updated in the first place.)\n\n  - What should be written to the configuration?  Writing the default\n    refspec configuration uses a different logic than assembling the\n    default refspec for the initial fetch.  As a result, even if the\n    default refspec is not used in the initial fetch, the command\n\n      # no leading '+' in the additional configured refspec\n      git clone \\\n        -c 'remote.origin.fetch=refs/heads/*:refs/remotes/origin/*' ...\n\n    will still write it to the config, resulting in:\n\n      $ git config --get-all remote.origin.fetch\n      refs/heads/*:refs/remotes/origin/*\n      +refs/heads/*:refs/remotes/origin/*\n\n    Alas, we can't simply avoid writing the default refspec to the\n    configuration if it matches one of the additional configured\n    refspecs from the command line, because there is\n\n      git -c remote.origin.fetch=<refspec> clone ...\n\n    as well...\n\n  - Alternatively, we could make ref transactions a bit more lax about\n    duplicated entries, and ignore multiple updates to the same ref if\n    all of those ref updates want to do the same thing, i.e. have the\n    same old and new OID.  However, the issue with writing the\n    configuration would still remain.\n\n\n[1] https://public-inbox.org/git/20190307214447.GA4909@chabuduo/\n\n --- >8 ---\n\nSubject: [PATCH] [PoC] clone: avoid redundant default refspec\n\n---\n builtin/clone.c         | 19 ++++++++++++++++++-\n t/t5611-clone-config.sh | 15 +++++++++++++++\n 2 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 85b0d3155d..f104510cfe 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -907,6 +907,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstruct remote *remote;\n \tint err = 0, complete_refs_before_fetch = 1;\n \tint submodule_progress;\n+\tint i, add_default_refspec = 1;\n+\tconst char *default_refspec_dst;\n \n \tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n \n@@ -1093,7 +1095,22 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \n \tstrbuf_addf(&default_refspec, \"+%s*:%s*\", src_ref_prefix,\n \t\t    branch_top.buf);\n-\trefspec_append(&remote->fetch, default_refspec.buf);\n+\t/*\n+\t * Don't add the default refspec if the user specified an additional\n+\t * refspec in the configuration whose destination matches the\n+\t * destination of the default refspec, because then both would want\n+\t * to update the same ref(s), leading to \"multiple updates\" error\n+\t * from the refs transaction.\n+\t * TODO: Or should it check both the source and the destination?\n+\t */\n+\tdefault_refspec_dst = strchr(default_refspec.buf, ':') + 1;\n+\tfor (i = 0; i < remote->fetch.nr; i++)\n+\t\tif (!strcmp(default_refspec_dst, remote->fetch.items[i].dst)) {\n+\t\t\tadd_default_refspec = 0;\n+\t\t\tbreak;\n+\t\t}\n+\tif (add_default_refspec)\n+\t\trefspec_append(&remote->fetch, default_refspec.buf);\n \n \ttransport = transport_get(remote, remote->url[0]);\n \ttransport_set_verbosity(transport, option_verbosity, option_progress);\ndiff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh\nindex 60c1ba951b..e63eec2894 100755\n--- a/t/t5611-clone-config.sh\n+++ b/t/t5611-clone-config.sh\n@@ -92,6 +92,21 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'clone -c remote.origin.fetch=<refspec> matches the default refspec' '\n+\trm -rf child &&\n+\tgit clone -c \"remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*\" \\\n+\t\t. child &&\n+\t# TODO: look, the same refspec is stored in the config twice:\n+\tgit -C child config --get-all remote.origin.fetch &&\n+\tgit -C child for-each-ref --format=\"%(refname)\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/master\n+\trefs/remotes/origin/HEAD\n+\trefs/remotes/origin/master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n # Tests for the hidden file attribute on windows\n is_hidden () {\n \t# Use the output of `attrib`, ignore the absolute path\n-- \n2.22.0.rc1.423.g9b4f2abbc5\n"}]}