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

[Jeff King <peff@peff.net>]

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:

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

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

> 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