git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* fatal: options '--bare' and '--origin foo' cannot be used together
@ 2022-09-21 19:23 John A. Leuenhagen
  2022-09-22  4:55 ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: John A. Leuenhagen @ 2022-09-21 19:23 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: fatal: options '--bare' and '--origin foo' cannot be used together
  2022-09-21 19:23 fatal: options '--bare' and '--origin foo' cannot be used together John A. Leuenhagen
@ 2022-09-22  4:55 ` Jeff King
  2022-09-22  5:31   ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2022-09-22  4:55 UTC (permalink / raw)
  To: John A. Leuenhagen; +Cc: Junio C Hamano, git

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: fatal: options '--bare' and '--origin foo' cannot be used together
  2022-09-22  4:55 ` Jeff King
@ 2022-09-22  5:31   ` Jeff King
  2022-09-22  5:32     ` [PATCH 1/2] clone: allow "--bare" with "-o" Jeff King
  2022-09-22  5:33     ` [PATCH 2/2] remote: handle rename of remote without fetch refspec Jeff King
  0 siblings, 2 replies; 11+ messages in thread
From: Jeff King @ 2022-09-22  5:31 UTC (permalink / raw)
  To: John A. Leuenhagen; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] clone: allow "--bare" with "-o"
  2022-09-22  5:31   ` Jeff King
@ 2022-09-22  5:32     ` Jeff King
  2022-09-22  5:58       ` Eric Sunshine
  2022-09-22  5:33     ` [PATCH 2/2] remote: handle rename of remote without fetch refspec Jeff King
  1 sibling, 1 reply; 11+ messages in thread
From: Jeff King @ 2022-09-22  5:32 UTC (permalink / raw)
  To: John A. Leuenhagen; +Cc: Junio C Hamano, git

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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] remote: handle rename of remote without fetch refspec
  2022-09-22  5:31   ` Jeff King
  2022-09-22  5:32     ` [PATCH 1/2] clone: allow "--bare" with "-o" Jeff King
@ 2022-09-22  5:33     ` Jeff King
  1 sibling, 0 replies; 11+ messages in thread
From: Jeff King @ 2022-09-22  5:33 UTC (permalink / raw)
  To: John A. Leuenhagen; +Cc: Junio C Hamano, git

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 &&
 	(
-- 
@@ -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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] clone: allow "--bare" with "-o"
  2022-09-22  5:32     ` [PATCH 1/2] clone: allow "--bare" with "-o" Jeff King
@ 2022-09-22  5:58       ` Eric Sunshine
  2022-09-22  6:18         ` Jeff King
  0 siblings, 1 reply; 11+ messages in thread
From: Eric Sunshine @ 2022-09-22  5:58 UTC (permalink / raw)
  To: Jeff King; +Cc: John A. Leuenhagen, Junio C Hamano, Git List

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?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] clone: allow "--bare" with "-o"
  2022-09-22  5:58       ` Eric Sunshine
@ 2022-09-22  6:18         ` Jeff King
  2022-09-22  6:26           ` Eric Sunshine
  2022-09-22  6:26           ` Jeff King
  0 siblings, 2 replies; 11+ messages in thread
From: Jeff King @ 2022-09-22  6:18 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: John A. Leuenhagen, Junio C Hamano, Git List

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] clone: allow "--bare" with "-o"
  2022-09-22  6:18         ` Jeff King
@ 2022-09-22  6:26           ` Eric Sunshine
  2022-09-22  6:35             ` Jeff King
  2022-09-22  6:26           ` Jeff King
  1 sibling, 1 reply; 11+ messages in thread
From: Eric Sunshine @ 2022-09-22  6:26 UTC (permalink / raw)
  To: Jeff King; +Cc: John A. Leuenhagen, Junio C Hamano, Git List

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] clone: allow "--bare" with "-o"
  2022-09-22  6:18         ` Jeff King
  2022-09-22  6:26           ` Eric Sunshine
@ 2022-09-22  6:26           ` Jeff King
  1 sibling, 0 replies; 11+ messages in thread
From: Jeff King @ 2022-09-22  6:26 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: John A. Leuenhagen, Junio C Hamano, Git List

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] clone: allow "--bare" with "-o"
  2022-09-22  6:26           ` Eric Sunshine
@ 2022-09-22  6:35             ` Jeff King
  2022-09-22  6:40               ` Eric Sunshine
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2022-09-22  6:35 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: John A. Leuenhagen, Junio C Hamano, Git List

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] clone: allow "--bare" with "-o"
  2022-09-22  6:35             ` Jeff King
@ 2022-09-22  6:40               ` Eric Sunshine
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Sunshine @ 2022-09-22  6:40 UTC (permalink / raw)
  To: Jeff King; +Cc: John A. Leuenhagen, Junio C Hamano, Git List

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-09-22  6:41 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-21 19:23 fatal: options '--bare' and '--origin foo' cannot be used together John A. Leuenhagen
2022-09-22  4:55 ` Jeff King
2022-09-22  5:31   ` Jeff King
2022-09-22  5:32     ` [PATCH 1/2] clone: allow "--bare" with "-o" Jeff King
2022-09-22  5:58       ` Eric Sunshine
2022-09-22  6:18         ` Jeff King
2022-09-22  6:26           ` Eric Sunshine
2022-09-22  6:35             ` Jeff King
2022-09-22  6:40               ` Eric Sunshine
2022-09-22  6:26           ` Jeff King
2022-09-22  5:33     ` [PATCH 2/2] remote: handle rename of remote without fetch refspec Jeff King

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).