git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: git-clone --config remote.origin.fetch regression

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
May 29, 2019, 13:20 UTC
Message-ID
<20190529132017.GM951@szeder.dev>
In-Reply-To
<CAFQ2z_O7rupGS4rntiUwJCHUj3bO0U88bVAToveEo_uU0Eu96w@mail.gmail.com>
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
Previous: Han-Wen Nienhuys
Message 2 of 2 in “git-clone --config remote.origin.fetch regression”
  1. Han-Wen NienhuysMay 28, 2019
  2. SZEDER GáborMay 29, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.