Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR

Jeff King [off-list ref] writes:

On Thu, Dec 29, 2016 at 07:48:45PM -0500, Jeff King wrote:

quoted

On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:

quoted

Thanks.  Here's the patch again, now with commit messages and a test.
Thanks for the analysis and sorry for the slow turnaround.

Thanks for following up. While working on a similar one recently, I had
the nagging feeling that there was a case we had found that was still to
be dealt with, and I think this was it. :)

The patch to the C code looks good. I have a few comments on the tests:

I happened to run into this problem myself today, so I thought I'd prod
you. I think your patch looks good. Hopefully I didn't scare you off
with my comments. :)

Thanks for prodding.  I'm tempted to just rip out everything other
than the 5-liner fix to transport.c and apply it, expecting that a
follow-up patch with updated tests to come sometime not in too
distant future.

On Tue, Feb 14, 2017 at 11:08:05AM -0800, Junio C Hamano wrote:

Thanks for prodding.  I'm tempted to just rip out everything other
than the 5-liner fix to transport.c and apply it, expecting that a
follow-up patch with updated tests to come sometime not in too
distant future.

I think we can at least include one basic test. Also, as it turns out
the problem I was seeing _wasn't_ the same one Jonathan fixed. There's
another bug. :)

So here's a patch series that fixes my bug, and then a cut-down version
of Jonathan's patch. I'd be happy to see more tests come on top. I don't
think there's a huge rush on getting any of this into master. There are
bugs in the existing code, but they're very hard to trigger in practice
(e.g., a non-repo which happens to have a bunch of repo-like files). It
only becomes an issue in 'next' when we die("BUG") to flush these cases
out.

  [1/2]: remote: avoid reading $GIT_DIR config in non-repo
  [2/2]: remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR

 remote.c                   | 2 +-
 t/t5512-ls-remote.sh       | 9 +++++++++
 t/t5550-http-fetch-dumb.sh | 9 +++++++++
 transport-helper.c         | 5 +++--
 4 files changed, 22 insertions(+), 3 deletions(-)

-Peff

The "git ls-remote" command can be run outside of a
repository, but needs to look up configured remotes. The
config code is smart enough to handle this case itself, but
we also check the historical "branches" and "remotes" paths
in $GIT_DIR. The git_path() function causes us to blindly
look at ".git/remotes", even if we know we aren't in a git
repository.

For now, this is just an unlikely bug (you probably don't
have such a file if you're not in a repository), but it will
become more obvious once we merge b1ef400ee (setup_git_env:
avoid blind fall-back to ".git", 2016-10-20):

  [now]
  $ git ls-remote
  fatal: No remote configured to list refs from.

  [with b1ef400ee]
  $ git ls-remote
  fatal: BUG: setup_git_env called without repository

We can fix this by skipping these sources entirely when
we're outside of a repository.

The test is a little more complex than the demonstration
above. Rather than detect the correct behavior by parsing
the error message, we can actually set up a case where the
remote name we give is a valid repository, but b1ef400ee
would cause us to die in the configuration step.

This test doesn't fail now, but it future-proofs us for the
b1ef400ee change.

Signed-off-by: Jeff King <redacted>
---
 remote.c             | 2 +-
 t/t5512-ls-remote.sh | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index bf1bf2309..9f83fe2c4 100644
--- a/remote.c
+++ b/remote.c

@@ -693,7 +693,7 @@ static struct remote *remote_get_1(const char *name,
 		name = get_default(current_branch, &name_given);
 
 	ret = make_remote(name, 0);
-	if (valid_remote_nick(name)) {
+	if (valid_remote_nick(name) && have_git_dir()) {
 		if (!valid_remote(ret))
 			read_remotes_file(ret);
 		if (!valid_remote(ret))

diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 55fc83fc0..94fc9be9c 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh

@@ -248,4 +248,13 @@ test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-complian
 	test_expect_code 2 git ls-remote --exit-code git://localhost:$JGIT_DAEMON_PORT/empty.git
 '
 
+test_expect_success 'ls-remote works outside repository' '
+	# It is important for this repo to be inside the nongit
+	# area, as we want a repo name that does not include
+	# slashes (because those inhibit some of our configuration
+	# lookups).
+	nongit git init --bare dst.git &&
+	nongit git ls-remote dst.git
+'
+
 test_done

-- 
2.12.0.rc1.479.g59880b11e

From: Jonathan Nieder <redacted>

To push from or fetch to the current repository, remote helpers need
to know what repository that is.  Accordingly, Git sets the GIT_DIR
environment variable to the path to the current repository when
invoking remote helpers.

There is a special case it does not handle: "git ls-remote" and "git
archive --remote" can be run to inspect a remote repository without
being run from any local repository.  GIT_DIR is not useful in this
scenario:

- if we are not in a repository, we don't need to set GIT_DIR to
  override an existing GIT_DIR value from the environment.  If GIT_DIR
  is present then we would be in a repository if it were valid and
  would have called die() if it weren't.

- not setting GIT_DIR may cause a helper to do the usual discovery
  walk to find the repository.  But we know we're not in one, or we
  would have found it ourselves.  So in the worst case it may expend
  a little extra effort to try to find a repository and fail (for
  example, remote-curl would do this to try to find repository-level
  configuration).

So leave GIT_DIR unset in this case.  This makes GIT_DIR easier to
understand for remote helper authors and makes transport code less of
a special case for repository discovery.

Noticed using b1ef400e (setup_git_env: avoid blind fall-back to
".git", 2016-10-20) from 'next':

 $ cd /tmp
 $ git ls-remote https://kernel.googlesource.com/pub/scm/git/git
 fatal: BUG: setup_git_env called without repository

Helped-by: Jeff King [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
Signed-off-by: Jeff King <redacted>
---
I dropped this down to a single test instance, and used the nongit
helper to shorten it.

Possible patches on top:

  - if we want to test this across more protocols, we can. I'm not sure
    I see all that much value in it, given that we know the source of
    the bug.

    We probably _should_ have some kind of standard test-battery that
    hits all protocols, or at the very least hits both dumb/smart http.
    But waiting for that may be making the perfect the enemy of the
    good. So I'm OK with doing it piece-wise for now if people really
    feel we need to cover more protocols.

  - Jonathan's original had some nice remote-ext tests, but they were
    sufficiently complex that they should be spun into their own patch
    with more explanation.

 t/t5550-http-fetch-dumb.sh | 9 +++++++++
 transport-helper.c         | 5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index aeb3a63f7..b69ece1d6 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh

@@ -34,6 +34,15 @@ test_expect_success 'clone http repository' '
 	test_cmp file clone/file
 '
 
+test_expect_success 'list refs from outside any repository' '
+	cat >expect <<-EOF &&
+	$(git rev-parse master)	HEAD
+	$(git rev-parse master)	refs/heads/master
+	EOF
+	nongit git ls-remote "$HTTPD_URL/dumb/repo.git" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'create password-protected repository' '
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/" &&
 	cp -Rf "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \

diff --git a/transport-helper.c b/transport-helper.c
index 91aed35eb..e4fd98238 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -124,8 +124,9 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->git_cmd = 0;
 	helper->silent_exec_failure = 1;
 
-	argv_array_pushf(&helper->env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
-			 get_git_dir());
+	if (have_git_dir())
+		argv_array_pushf(&helper->env_array, "%s=%s",
+				 GIT_DIR_ENVIRONMENT, get_git_dir());
 
 	code = start_command(helper);
 	if (code < 0 && errno == ENOENT)

-- 
2.12.0.rc1.479.g59880b11e