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