Re: git-clone --config remote.origin.fetch regression
On Tue, May 28, 2019 at 07:34:58PM -0300, Han-Wen Nienhuys wrote:
Show 19 quoted lines
> (see also https://github.com/google/zoekt/issues/81)
>
> It looks like git 2.21 included a regression. The command
>
> git clone --bare --progress \
> --config "remote.origin.fetch=+refs/heads/*:refs/heads/*" \
> https://github.com/google/zoekt.git \
> /tmp/zoekt-git2.20.git
>
> would succeed with git 2.20, but fails with
>
> fatal: multiple updates for ref 'refs/heads/master' not allowed
>
> in git 2.21, probably caused by commit 515be83.
>
> Should I call git in another way? I originally included
> "remote.origin.fetch=+refs/heads/*:refs/heads/*" to avoid getting
> Gerrit refs (refs/changes/*), but maybe I should use a different
> incantation?
On first sight I was wondering why you don't use '--mirror', but yeah, that would fetch 'refs/changes/*' as well (the whole 'refs/*', really). In the meantime a workaround would be to run separate commands to accomplish what 'git clone' would do under the hood:
git init --bare zoekt.git
cd zoekt.git
git config remote.origin.url https://github.com/google/zoekt.git
git config remote.origin.fetch '+refs/heads/*:refs/heads/*'
git fetch
This is indeed caused by 515be83382 (clone: respect additional configured fetch refspecs during initial fetch, 2018-11-14): since then the same refspec comes up twice in remote->fetch, once from the configuration that you specified on the command line and once from the default refspec that 'git clone' would have used anyway, and in the end 'git clone' tries to write each ref twice, once for each of those two refspecs, in a single ref transaction, hence the error.
I'm not quite sure how to properly handle the situation. The patch at the bottom makes your case (and the case in an earlier report [1]) work by omitting 'git clone's default refspec if one of the configured refspecs have the same destination side as the default refspec. I think this is a step in the right-ish direction, but there are some open questions:
- Should it only check the destination side of the refspecs, or the
source as well? IOW, in case of e.g. git clone --bare \
-c 'remote.origin.fetch=refs/foo/*:refs/heads/*' ... should it only fetch from 'refs/foo/*' or both from 'refs/foo/*'
and 'refs/heads/*'? With the patch below it's only 'refs/foo/*'.
If we were to fetch from both, then I expect trouble when both
'refs/foo/feature' and 'refs/heads/feature' happen to exist, but
this is a more general issue not limited to 'git clone -c ...'. - Even if it were to check the source side of the refspec, it should
ignore the optional leading '+' in the refspecs, because otherwise
the command # no leading '+' in the additional configured refspec
git clone --bare \
-c 'remote.origin.fetch=refs/heads/*:refs/heads/*' ... would lead to the same error, because the default refspec for
'--bare' is '+refs/heads/*:refs/heads/*'. (Sidenote: 'git clone's default refspec always has the leading
'+', but I think that's unnecessary, because during cloning there
are no existing refs to be updated in the first place.) - What should be written to the configuration? Writing the default
refspec configuration uses a different logic than assembling the
default refspec for the initial fetch. As a result, even if the
default refspec is not used in the initial fetch, the command # no leading '+' in the additional configured refspec
git clone \
-c 'remote.origin.fetch=refs/heads/*:refs/remotes/origin/*' ... will still write it to the config, resulting in:
$ git config --get-all remote.origin.fetch
refs/heads/*:refs/remotes/origin/*
+refs/heads/*:refs/remotes/origin/* Alas, we can't simply avoid writing the default refspec to the
configuration if it matches one of the additional configured
refspecs from the command line, because there is git -c remote.origin.fetch=<refspec> clone ...
as well...
- Alternatively, we could make ref transactions a bit more lax about
duplicated entries, and ignore multiple updates to the same ref if
all of those ref updates want to do the same thing, i.e. have the
same old and new OID. However, the issue with writing the
configuration would still remain.[1] https://public-inbox.org/git/20190307214447.GA4909@chabuduo/
--- >8 ---
Subject: [PATCH] [PoC] clone: avoid redundant default refspec
---
builtin/clone.c | 19 ++++++++++++++++++-
t/t5611-clone-config.sh | 15 +++++++++++++++
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 85b0d3155d..f104510cfe 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -907,6 +907,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
struct remote *remote;
int err = 0, complete_refs_before_fetch = 1;
int submodule_progress;
+ int i, add_default_refspec = 1;
+ const char *default_refspec_dst;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
@@ -1093,7 +1095,22 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
branch_top.buf);
- refspec_append(&remote->fetch, default_refspec.buf);
+ /*
+ * Don't add the default refspec if the user specified an additional
+ * refspec in the configuration whose destination matches the
+ * destination of the default refspec, because then both would want
+ * to update the same ref(s), leading to "multiple updates" error
+ * from the refs transaction.
+ * TODO: Or should it check both the source and the destination?
+ */
+ default_refspec_dst = strchr(default_refspec.buf, ':') + 1;
+ for (i = 0; i < remote->fetch.nr; i++)
+ if (!strcmp(default_refspec_dst, remote->fetch.items[i].dst)) {
+ add_default_refspec = 0;
+ break;
+ }
+ if (add_default_refspec)
+ refspec_append(&remote->fetch, default_refspec.buf);
transport = transport_get(remote, remote->url[0]);
transport_set_verbosity(transport, option_verbosity, option_progress);
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 60c1ba951b..e63eec2894 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -92,6 +92,21 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
test_cmp expect actual
'
+test_expect_success 'clone -c remote.origin.fetch=<refspec> matches the default refspec' '
+ rm -rf child &&
+ git clone -c "remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*" \
+ . child &&
+ # TODO: look, the same refspec is stored in the config twice:
+ git -C child config --get-all remote.origin.fetch &&
+ git -C child for-each-ref --format="%(refname)" >actual &&
+ cat >expect <<-\EOF &&
+ refs/heads/master
+ refs/remotes/origin/HEAD
+ refs/remotes/origin/master
+ EOF
+ test_cmp expect actual
+'
+
# Tests for the hidden file attribute on windows
is_hidden () {
# Use the output of `attrib`, ignore the absolute path
--
2.22.0.rc1.423.g9b4f2abbc5
Han-Wen Nienhuys