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

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

From
Jeff King <peff@peff.net>
Date
Feb 14, 2017, 20:36 UTC
Message-ID
<20170214203619.62plnss65mdwf3na@sigill.intra.peff.net>
In-Reply-To
<20170214203117.xnln6ahb3l32agqb@sigill.intra.peff.net>
From: Jonathan Nieder <jrnieder@gmail.com>

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 <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
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
Previous: Jeff KingNext: Junio C Hamano
Message 28 of 29 in “stop blind fallback to ".git"”
  1. 0/7 stop blind fallback to ".git"Jeff King, Oct 20, 2016
  2. 1/7 read info/{attributes,exclude} only when in repositoryJeff King, Oct 20, 2016
  3. Duy NguyenOct 25, 2016
  4. Jeff KingOct 25, 2016
  5. 2/7 test-*-cache-tree: setup git dirJeff King, Oct 20, 2016
  6. 3/7 find_unique_abbrev: use 4-buffer ringJeff King, Oct 20, 2016
  7. 4/7 diff_unique_abbrev: rename to diff_aligned_abbrevJeff King, Oct 20, 2016
  8. 5/7 diff_aligned_abbrev: use "struct oid"Jeff King, Oct 20, 2016
  9. 6/7 diff: handle sha1 abbreviations outside of repositoryJeff King, Oct 20, 2016
  10. Jeff KingOct 20, 2016
  11. 7/7 setup_git_env: avoid blind fall-back to ".git"Jeff King, Oct 20, 2016
  12. Duy NguyenOct 25, 2016
  13. Jeff KingOct 25, 2016
  14. Duy NguyenOct 26, 2016
  15. Jeff KingOct 26, 2016
  16. Duy NguyenOct 26, 2016
  17. Jeff KingOct 26, 2016
  18. Jonathan NiederNov 22, 2016
  19. Jeff KingNov 22, 2016
  20. remote helpers: avoid blind fall-back to ".git" when setting GIT_DIRJonathan Nieder, Dec 30, 2016
  21. Stefan BellerDec 30, 2016
  22. Jeff KingDec 30, 2016
  23. Jeff KingDec 30, 2016
  24. Jeff KingFeb 14, 2017
  25. Junio C HamanoFeb 14, 2017
  26. Jeff KingFeb 14, 2017
  27. 1/2 remote: avoid reading $GIT_DIR config in non-repoJeff King, Feb 14, 2017
  28. 2/2 remote helpers: avoid blind fall-back to ".git" when setting GIT_DIRJeff King, Feb 14, 2017
  29. Junio C HamanoNov 22, 2016

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.