threads / discuss / 58481

fatal: options '--bare' and '--origin foo' cannot be used together

Subject: fatal: options '--bare' and '--origin foo' cannot be used together

## tl;dr

11 messages between Sep 21, 2022 and Sep 22, 2022.

replies: 10people: 3as markdown or json

John A. Leuenhagen· Sep 21, 2022, 19:23 UTC · lore
Hi all,

I often like to change the name of the remote that I clone repositories from, as "origin" is too generic for me when I have more than one remote.

Usually, I only remember after I clone, and wind up doing `git remote rename origin foo`. I'd like to get in the habit of instead doing `git clone -o foo https://link.to/foo.git`. However, when I tried doing this with --bare, I got the error that I've made the subject of this email. Why can't I do this?

When I try to do `git remote rename origin foo` in the bare repository (cloned without -o foo), I get a different error:

>fatal: could not unset 'remote.foo.fetch'

However, `git config remote.foo.fetch` indicates that this value is not set.

Why does this behavior differ from the equivalent commands on a non-bare repository?

-- 
John
Jeff King· Sep 22, 2022, 04:55 UTC · re: John A. Leuenhagen · lore

Re: fatal: options '--bare' and '--origin foo' cannot be used together

On Wed, Sep 21, 2022 at 03:23:33PM -0400, John A. Leuenhagen wrote:
Show 5 quoted lines
> Usually, I only remember after I clone, and wind up doing `git remote
> rename origin foo`. I'd like to get in the habit of instead doing
> `git clone -o foo https://link.to/foo.git`. However, when I tried doing
> this with --bare, I got the error that I've made the subject of this
> email. Why can't I do this?

I don't think there's a fundamental reason they cannot be mixed. In fact, you can do what you want with:

  git -c clone.defaultRemoteName=foo clone ...

(which, btw, might give a better solution than remembering to use "-o" each time, since you can put that in your global git config).

So we are just complaining about the actual "-o" option. The conditional which catches that comes from 8434c2f1af (Build in clone, 2008-04-27), which is not much help. Before that, the same logic was in the shell version, which was added in Junio's e6489a1bdf (clone: do not accept more than one -o option., 2006-01-22), but there's no explanation given.

I do think the remote name is less important in a bare clone, because we don't create refs/remotes/foo/* there. But you might still care about the name of the remote for doing "git fetch foo", etc. So I imagine we could simply loosen the check like:

diff --git a/builtin/clone.c b/builtin/clone.c
index d269d6fec6..ed8d44bb6a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -929,9 +929,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_bare = 1;
 
 	if (option_bare) {
-		if (option_origin)
-			die(_("options '%s' and '%s %s' cannot be used together"),
-			    "--bare", "--origin", option_origin);
 		if (real_git_dir)
 			die(_("options '%s' and '%s' cannot be used together"), "--bare", "--separate-git-dir");
 		option_no_checkout = 1;
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index f6bb02ab94..cf221e92c4 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -42,11 +42,12 @@ test_expect_success 'rejects invalid -o/--origin' '
 
 '
 
-test_expect_success 'disallows --bare with --origin' '
+test_expect_success 'clone --bare -o' '
 
-	test_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&
-	test_debug "cat err" &&
-	test_i18ngrep -e "options .--bare. and .--origin foo. cannot be used together" err
+	git clone -o foo --bare parent clone-bare-o &&
+	(cd parent && pwd) >expect &&
+	git -C clone-bare-o config remote.foo.url >actual &&
+	test_cmp expect actual
 
 '
 

> When I try to do `git remote rename origin foo` in the bare repository
> (cloned without -o foo), I get a different error:
> 
> >fatal: could not unset 'remote.foo.fetch'
> 
> However, `git config remote.foo.fetch` indicates that this value is not
> set.
> 
> Why does this behavior differ from the equivalent commands on a non-bare
> repository?

This second case is IMHO a minor bug in the git-remote code. As
documented in git-clone(1):

  --bare::
     [...]
        Also the branch heads at the remote are copied directly
        to corresponding local branch heads, without mapping
        them to `refs/remotes/origin/`. When this option is
        used, neither remote-tracking branches nor the related
        configuration variables are created.

So it is expected that there is no configured refspec in a bare
repository. But it looks like the remote-renaming code is not prepared
to handle this case. The backtrace for the error is:

  #0  die (err=0x5555558c349a "could not unset '%s'") at usage.c:175
  #1  0x00005555556bce85 in git_config_set_multivar_in_file (config_filename=0x5555559bc700 "config",
      key=0x5555559b5850 "remote.foo.fetch", value=0x0, value_pattern=0x0, flags=1) at config.c:3459
  #2  0x00005555556bcf77 in git_config_set_multivar (key=0x5555559b5850 "remote.foo.fetch", value=0x0,
      value_pattern=0x0, flags=1) at config.c:3485
  #3  0x000055555562bb59 in mv (argc=2, argv=0x7fffffffe550, prefix=0x0) at builtin/remote.c:738
  #4  0x000055555562f7b9 in cmd_remote (argc=3, argv=0x7fffffffe550, prefix=0x0) at builtin/remote.c:1772
  #5  0x0000555555574c53 in run_builtin (p=0x5555559760b0 <commands+2352>, argc=4, argv=0x7fffffffe550) at git.c:466
  #6  0x000055555557506f in handle_builtin (argc=4, argv=0x7fffffffe550) at git.c:721
  #7  0x00005555555752dd in run_argv (argcp=0x7fffffffe3bc, argv=0x7fffffffe3b0) at git.c:788
  #8  0x0000555555575848 in cmd_main (argc=4, argv=0x7fffffffe550) at git.c:921
  #9  0x00005555556694c1 in main (argc=5, argv=0x7fffffffe548) at common-main.c:56

So probably it needs to use the "gently" version of git_config_set_multivar(),
which avoids calling die(), and then check for a return code which
indicates no value was found.

-Peff
Jeff King· Sep 22, 2022, 05:31 UTC · re: Jeff King · lore

Re: fatal: options '--bare' and '--origin foo' cannot be used together

On Thu, Sep 22, 2022 at 12:55:47AM -0400, Jeff King wrote:
Show 6 quoted lines
> This second case is IMHO a minor bug in the git-remote code. As
> documented in git-clone(1):
> [...]
> So probably it needs to use the "gently" version of git_config_set_multivar(),
> which avoids calling die(), and then check for a return code which
> indicates no value was found.

Actually, there is an even simpler fix. So here are two cleaned-up patches which fix both issues you found.

  [1/2]: clone: allow "--bare" with "-o"
  [2/2]: remote: handle rename of remote without fetch refspec
 builtin/clone.c          |  3 ---
 builtin/remote.c         | 48 +++++++++++++++++++++-------------------
 t/t5505-remote.sh        | 11 +++++++++
 t/t5606-clone-options.sh |  9 ++++----
 4 files changed, 41 insertions(+), 30 deletions(-)
-Peff
Jeff King· Sep 22, 2022, 05:32 UTC · re: Jeff King · lore

[PATCH 1/2] clone: allow "--bare" with "-o"

We explicitly forbid the combination of "--bare" with "-o", but there doesn't seem to be any good reason to do so. The original logic came as part of e6489a1bdf (clone: do not accept more than one -o option., 2006-01-22), but that commit does not give any reason.

Furthermore, the equivalent combination via config is allowed:
  git -c clone.defaultRemoteName=foo clone ...

and works as expected. It may be that this combination was considered useless, because a bare clone does not set remote.origin.fetch (and hence there is no refs/remotes/origin hierarchy). But it does set remote.origin.url, and that name is visible to the user via "git fetch origin", etc.

Let's allow the options to be used together, and switch the "forbid" test in t5606 to check that we use the requested name. That test came much later in 349cff76de (clone: add tests for --template and some disallowed option pairs, 2020-09-29), and does not offer any logic beyond "let's test what the code currently does".

Reported-by: John A. Leuenhagen <john@zlima12.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c          | 3 ---
 t/t5606-clone-options.sh | 9 +++++----
 2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index d269d6fec6..ed8d44bb6a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -929,9 +929,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_bare = 1;
 
 	if (option_bare) {
-		if (option_origin)
-			die(_("options '%s' and '%s %s' cannot be used together"),
-			    "--bare", "--origin", option_origin);
 		if (real_git_dir)
 			die(_("options '%s' and '%s' cannot be used together"), "--bare", "--separate-git-dir");
 		option_no_checkout = 1;
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index f6bb02ab94..cf221e92c4 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -42,11 +42,12 @@ test_expect_success 'rejects invalid -o/--origin' '
 
 '
 
-test_expect_success 'disallows --bare with --origin' '
+test_expect_success 'clone --bare -o' '
 
-	test_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&
-	test_debug "cat err" &&
-	test_i18ngrep -e "options .--bare. and .--origin foo. cannot be used together" err
+	git clone -o foo --bare parent clone-bare-o &&
+	(cd parent && pwd) >expect &&
+	git -C clone-bare-o config remote.foo.url >actual &&
+	test_cmp expect actual
 
 '
 
-- 
2.38.0.rc1.580.gfaa349ad1e
Eric Sunshine· Sep 22, 2022, 05:58 UTC · re: Jeff King · lore

Re: [PATCH 1/2] clone: allow "--bare" with "-o"

On Thu, Sep 22, 2022 at 1:33 AM Jeff King <peff@peff.net> wrote:
Show 17 quoted lines
> [...]
> Let's allow the options to be used together, and switch the "forbid"
> test in t5606 to check that we use the requested name. That test came
> much later in 349cff76de (clone: add tests for --template and some
> disallowed option pairs, 2020-09-29), and does not offer any logic
> beyond "let's test what the code currently does".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> @@ -42,11 +42,12 @@ test_expect_success 'rejects invalid -o/--origin' '
> +test_expect_success 'clone --bare -o' '
> +       git clone -o foo --bare parent clone-bare-o &&
> +       (cd parent && pwd) >expect &&
> +       git -C clone-bare-o config remote.foo.url >actual &&
> +       test_cmp expect actual
>  '
Is this safe on Microsoft Windows? My understanding from t/README:
    When a test checks for an absolute path that a git command
    generated, construct the expected value using $(pwd) rather than
    $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference
    on Windows, where the shell (MSYS bash) mangles absolute path
    names. For details, see the commit message of 4114156ae9.

was that you should use $(pwd) rather than raw `pwd` when comparing against a path generated by Git. Is there a gap in my understanding here?

Jeff King· Sep 22, 2022, 06:18 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/2] clone: allow "--bare" with "-o"

On Thu, Sep 22, 2022 at 01:58:36AM -0400, Eric Sunshine wrote:
Show 20 quoted lines
> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> > @@ -42,11 +42,12 @@ test_expect_success 'rejects invalid -o/--origin' '
> > +test_expect_success 'clone --bare -o' '
> > +       git clone -o foo --bare parent clone-bare-o &&
> > +       (cd parent && pwd) >expect &&
> > +       git -C clone-bare-o config remote.foo.url >actual &&
> > +       test_cmp expect actual
> >  '
> 
> Is this safe on Microsoft Windows? My understanding from t/README:
> 
>     When a test checks for an absolute path that a git command
>     generated, construct the expected value using $(pwd) rather than
>     $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference
>     on Windows, where the shell (MSYS bash) mangles absolute path
>     names. For details, see the commit message of 4114156ae9.
> 
> was that you should use $(pwd) rather than raw `pwd` when comparing
> against a path generated by Git. Is there a gap in my understanding
> here?

I think you might be mis-reading the advice here. It is saying to use the "pwd" program, rather than relying on the shell's $PWD variable. So $(pwd) and `pwd` are the same thing (and are what I'm using). The $() I think is just indicating that you'd do:

  foo=$(pwd)

And yes, I think this is a case where using the right one is important (which is why I used the pwd program, and not $pwd in the test).

Or am I missing something else?
-Peff
Eric Sunshine· Sep 22, 2022, 06:26 UTC · re: Jeff King · lore

Re: [PATCH 1/2] clone: allow "--bare" with "-o"

On Thu, Sep 22, 2022 at 2:18 AM Jeff King <peff@peff.net> wrote:
Show 28 quoted lines
> On Thu, Sep 22, 2022 at 01:58:36AM -0400, Eric Sunshine wrote:
> > > +       (cd parent && pwd) >expect &&
> > > +       git -C clone-bare-o config remote.foo.url >actual &&
> > > +       test_cmp expect actual
> >
> > Is this safe on Microsoft Windows? My understanding from t/README:
> >
> >     When a test checks for an absolute path that a git command
> >     generated, construct the expected value using $(pwd) rather than
> >     $PWD, $TEST_DIRECTORY, or $TRASH_DIRECTORY. It makes a difference
> >     on Windows, where the shell (MSYS bash) mangles absolute path
> >     names. For details, see the commit message of 4114156ae9.
> >
> > was that you should use $(pwd) rather than raw `pwd` when comparing
> > against a path generated by Git. Is there a gap in my understanding
> > here?
>
> I think you might be mis-reading the advice here. It is saying to use
> the "pwd" program, rather than relying on the shell's $PWD variable. So
> $(pwd) and `pwd` are the same thing (and are what I'm using). The $() I
> think is just indicating that you'd do:
>
>   foo=$(pwd)
>
> And yes, I think this is a case where using the right one is important
> (which is why I used the pwd program, and not $pwd in the test).
>
> Or am I missing something else?
I was thinking, in particular, about this snippet from t/test-lib.sh:
    # git sees Windows-style pwd
    pwd () {
        builtin pwd -W
    }

If that's inherited by the subshell used in the test, then I suppose all is okay, though I think it would not be inherited.

Jeff King· Sep 22, 2022, 06:35 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/2] clone: allow "--bare" with "-o"

On Thu, Sep 22, 2022 at 02:26:09AM -0400, Eric Sunshine wrote:
Show 21 quoted lines
> > I think you might be mis-reading the advice here. It is saying to use
> > the "pwd" program, rather than relying on the shell's $PWD variable. So
> > $(pwd) and `pwd` are the same thing (and are what I'm using). The $() I
> > think is just indicating that you'd do:
> >
> >   foo=$(pwd)
> >
> > And yes, I think this is a case where using the right one is important
> > (which is why I used the pwd program, and not $pwd in the test).
> >
> > Or am I missing something else?
> 
> I was thinking, in particular, about this snippet from t/test-lib.sh:
> 
>     # git sees Windows-style pwd
>     pwd () {
>         builtin pwd -W
>     }
> 
> If that's inherited by the subshell used in the test, then I suppose
> all is okay, though I think it would not be inherited.

I think it's OK. $() is itself a subshell, and you can find many calls to FOO=$(pwd) or similar. And in general, functions are inherited in subshells:

  $ sh -c 'foo() { echo bar; }; (cd .. && foo)'
  bar

They'd have to be or many things in our test suite would break, since we use test_must_fail, etc, inside subshells.

-Peff
Eric Sunshine· Sep 22, 2022, 06:40 UTC · re: Jeff King · lore

Re: [PATCH 1/2] clone: allow "--bare" with "-o"

On Thu, Sep 22, 2022 at 2:35 AM Jeff King <peff@peff.net> wrote:
Show 20 quoted lines
> On Thu, Sep 22, 2022 at 02:26:09AM -0400, Eric Sunshine wrote:
> > I was thinking, in particular, about this snippet from t/test-lib.sh:
> >
> >     # git sees Windows-style pwd
> >     pwd () {
> >         builtin pwd -W
> >     }
> >
> > If that's inherited by the subshell used in the test, then I suppose
> > all is okay, though I think it would not be inherited.
>
> I think it's OK. $() is itself a subshell, and you can find many calls
> to FOO=$(pwd) or similar. And in general, functions are inherited in
> subshells:
>
>   $ sh -c 'foo() { echo bar; }; (cd .. && foo)'
>   bar
>
> They'd have to be or many things in our test suite would break, since we
> use test_must_fail, etc, inside subshells.

Yep, thanks for the sanity dose. For some reason, my brain was off-kilter, thinking that it was running a shell afresh, not just using a subshell.

Jeff King· Sep 22, 2022, 06:26 UTC · re: Jeff King · lore

Re: [PATCH 1/2] clone: allow "--bare" with "-o"

On Thu, Sep 22, 2022 at 02:18:02AM -0400, Jeff King wrote:
> And yes, I think this is a case where using the right one is important
> (which is why I used the pwd program, and not $pwd in the test).
Er, of course I meant all-caps $PWD in the final sentence. ;)
-Peff
Jeff King· Sep 22, 2022, 05:33 UTC · re: Jeff King · lore

[PATCH 2/2] remote: handle rename of remote without fetch refspec

We return an error when trying to rename a remote that has no fetch refspec:

  $ git config --unset-all remote.origin.fetch
  $ git remote rename origin foo
  fatal: could not unset 'remote.foo.fetch'

To make things even more confusing, we actually _do_ complete the config modification, via git_config_rename_section(). After that we try to rewrite the fetch refspec (to say refs/remotes/foo instead of origin). But our call to git_config_set_multivar() to remove the existing entries fails, since there aren't any, and it calls die().

We could fix this by using the "gently" form of the config call, and checking the error code. But there is an even simpler fix: if we know that there are no refspecs to rewrite, then we can skip that part entirely.

Reported-by: John A. Leuenhagen <john@zlima12.com>
Signed-off-by: Jeff King <peff@peff.net>
---
The diff is a bit noisy due to indentation, but with "-w" you can see
that it's just wrapping this part of the function in a conditional.
 builtin/remote.c  | 48 ++++++++++++++++++++++++-----------------------
 t/t5505-remote.sh | 11 +++++++++++
 2 files changed, 36 insertions(+), 23 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 985b845a18..910f7b9316 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -733,29 +733,31 @@ static int mv(int argc, const char **argv, const char *prefix)
 		return error(_("Could not rename config section '%s' to '%s'"),
 				buf.buf, buf2.buf);
 
-	strbuf_reset(&buf);
-	strbuf_addf(&buf, "remote.%s.fetch", rename.new_name);
-	git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
-	strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name);
-	for (i = 0; i < oldremote->fetch.raw_nr; i++) {
-		char *ptr;
-
-		strbuf_reset(&buf2);
-		strbuf_addstr(&buf2, oldremote->fetch.raw[i]);
-		ptr = strstr(buf2.buf, old_remote_context.buf);
-		if (ptr) {
-			refspec_updated = 1;
-			strbuf_splice(&buf2,
-				      ptr-buf2.buf + strlen(":refs/remotes/"),
-				      strlen(rename.old_name), rename.new_name,
-				      strlen(rename.new_name));
-		} else
-			warning(_("Not updating non-default fetch refspec\n"
-				  "\t%s\n"
-				  "\tPlease update the configuration manually if necessary."),
-				buf2.buf);
-
-		git_config_set_multivar(buf.buf, buf2.buf, "^$", 0);
+	if (oldremote->fetch.raw_nr) {
+		strbuf_reset(&buf);
+		strbuf_addf(&buf, "remote.%s.fetch", rename.new_name);
+		git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);
+		strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name);
+		for (i = 0; i < oldremote->fetch.raw_nr; i++) {
+			char *ptr;
+
+			strbuf_reset(&buf2);
+			strbuf_addstr(&buf2, oldremote->fetch.raw[i]);
+			ptr = strstr(buf2.buf, old_remote_context.buf);
+			if (ptr) {
+				refspec_updated = 1;
+				strbuf_splice(&buf2,
+					      ptr-buf2.buf + strlen(":refs/remotes/"),
+					      strlen(rename.old_name), rename.new_name,
+					      strlen(rename.new_name));
+			} else
+				warning(_("Not updating non-default fetch refspec\n"
+					  "\t%s\n"
+					  "\tPlease update the configuration manually if necessary."),
+					buf2.buf);
+
+			git_config_set_multivar(buf.buf, buf2.buf, "^$", 0);
+		}
 	}
 
 	read_branches();
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 9006196ac6..43b7bcd715 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -902,6 +902,17 @@ test_expect_success 'rename a remote renames repo remote.pushDefault but keeps g
 	)
 '
 
+test_expect_success 'rename handles remote without fetch refspec' '
+	git clone --bare one no-refspec.git &&
+	# confirm assumption that bare clone does not create refspec
+	test_expect_code 5 \
+		git -C no-refspec.git config --unset-all remote.origin.fetch &&
+	git -C no-refspec.git config remote.origin.url >expect &&
+	git -C no-refspec.git remote rename origin foo &&
+	git -C no-refspec.git config remote.foo.url >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rename does not update a non-default fetch refspec' '
 	git clone one four.one &&
 	(
-- 
2.38.0.rc1.580.gfaa349ad1e

← back to recent threads