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

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

From
Jeff King <peff@peff.net>
Date
Dec 30, 2016, 00:48 UTC
Message-ID
<20161230004845.rknafqsyosmyr6z2@sigill.intra.peff.net>
In-Reply-To
<20161230001114.GB7883@aiede.mtv.corp.google.com>
On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:
> 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:
Show 22 quoted lines
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index aeb3a63f7c..a86fca3e6c 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -34,6 +34,21 @@ 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
> +	mkdir lsremote-root &&
> +	(
> +		GIT_CEILING_DIRECTORIES=$(pwd) &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd lsremote-root &&
> +		git ls-remote "$HTTPD_URL/dumb/repo.git" >../actual
> +	) &&
> +	test_cmp expect actual
> +'

Since my recent de95302a4c (t5000: extract nongit function to test-lib-functions.sh, 2016-12-15), this can be shortened to:

  cat >expect <<-EOF &&
  ...
  EOF
  nongit git ls-remote "$HTTPD_URL/dumb/repo.git" >actual &&
  test_cmp expect actual
I think that commit is in 'master' now.

Without my patch to die() in setup_git_env(), I think this would pass with or without your patch, right? I think the current behavior _is_ buggy, but a setup to show off the improvement would be so arcane that I don't think it is worth it. E.g., something like:

  echo '[http]extraHeader = "Foo: Bar" >nongit/.git/config

would probably trigger the use of that config when it shouldn't. But that's really stretching.

Show 22 quoted lines
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 6e5b9e42fb..7ba894f0f4 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -163,6 +163,21 @@ test_expect_success 'redirects send auth to new location' '
>  	expect_askpass both user@host auth/smart/repo.git
>  '
>  
> +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
> +	mkdir lsremote-root &&
> +	(
> +		GIT_CEILING_DIRECTORIES=$(pwd) &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd lsremote-root &&
> +		git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> +	) &&
> +	test_cmp expect actual
> +'

Is this really testing anything that the dumb one isn't? The interesting bit is in invoking git-remote-http, not what it does under the hood. I don't mind too much being thorough, but if we are going to go that route we should probably come up with a standard battery of tests for dumb and smart HTTP, and then invoke them for each case without having to write each one twice.

Show 22 quoted lines
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index 225a022e8a..4573d98e6c 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -35,6 +35,21 @@ test_expect_success 'clone git 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
> +	mkdir lsremote-root &&
> +	(
> +		GIT_CEILING_DIRECTORIES=$(pwd) &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd lsremote-root &&
> +		git ls-remote "$GIT_DAEMON_URL/repo.git" >../actual
> +	) &&
> +	test_cmp expect actual
> +'

This one isn't actually broken now, right? The test is just there to catch future regressions?

If we are being thorough, then would we also care about file-local repos, git-over-ssh, etc?

> diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
> index c6c2661878..7232032cd2 100755
> --- a/t/t5802-connect-helper.sh
> +++ b/t/t5802-connect-helper.sh

This one is quite a big addition. I know this falls under the "while we're at it" line at the end of your commit message, but maybe it's worth pulling the GIT_EXT_SERVICE bits out into their own patch (and explaining briefly what's going on; I had to go look up what GIT_EXT_SERVICE_NOPREFIX even was).

-Peff
Previous: Jeff KingNext: Jeff King
Message 23 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.