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

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

[John A. Leuenhagen]

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

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

[Jeff King]

On Wed, Sep 21, 2022 at 03:23:33PM -0400, John A. Leuenhagen wrote:

> 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

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

[Jeff King]

On Thu, Sep 22, 2022 at 12:55:47AM -0400, Jeff King wrote:

> 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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Eric Sunshine]

On Thu, Sep 22, 2022 at 1:33 AM Jeff King <peff@peff.net> wrote:
> [...]
> 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?

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

[Jeff King]

On Thu, Sep 22, 2022 at 01:58:36AM -0400, Eric Sunshine wrote:

> > 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

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

[Eric Sunshine]

On Thu, Sep 22, 2022 at 2:18 AM Jeff King <peff@peff.net> wrote:
> 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.

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

[Jeff King]

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

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

[Jeff King]

On Thu, Sep 22, 2022 at 02:26:09AM -0400, Eric Sunshine wrote:

> > 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

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

[Eric Sunshine]

On Thu, Sep 22, 2022 at 2:35 AM Jeff King <peff@peff.net> wrote:
> 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.