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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King  writes:

> On Thu, Dec 29, 2016 at 07:48:45PM -0500, Jeff King wrote:
>
>> 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:
>
> 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.

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

[Jeff King]

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

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

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

-Peff

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

[Jeff King]

On Thu, Dec 29, 2016 at 04:37:30PM -0800, Stefan Beller wrote:

> > +   mkdir lsremote-root &&
> > +   (
> > +   GIT_CEILING_DIRECTORIES=$(pwd) &&
> > +   export GIT_CEILING_DIRECTORIES &&
> > +   cd lsremote-root &&
> > +   git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> > +   ) &&
> 
> We could avoid the subshell via
> 
> GIT_CEILING_DIRECTORIES=$(pwd) \
> git -C lsremote-root lsremote ... >actual
> 
> Not sure if it is worth to trade off a block of code (and an extra shell
> at run time) for an overly long line.
> 
> The rest looks good to me.

I mentioned elsewhere that we now have a "nongit" function to do this as
a one-liner. It might be worth applying your optimization to that
function, so it would take effect in may places.

-Peff

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

[Jeff King]

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

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

[Stefan Beller]

> +   mkdir lsremote-root &&
> +   (
> +   GIT_CEILING_DIRECTORIES=$(pwd) &&
> +   export GIT_CEILING_DIRECTORIES &&
> +   cd lsremote-root &&
> +   git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> +   ) &&

We could avoid the subshell via

GIT_CEILING_DIRECTORIES=$(pwd) \
git -C lsremote-root lsremote ... >actual

Not sure if it is worth to trade off a block of code (and an extra shell
at run time) for an overly long line.

The rest looks good to me.

Thanks,
Stefan