patch, 7 partsfix transfer.hiderefs with smart http

I'm experimenting with using transfer.hiderefs on a server, and it's rather easy to cause a git client to hang when fetching from such a repo over smart http. Details are in the first patch.

There are 7 patches here, but the entirety of the fix is contained in the first one. The rest are cleanups and test enhancements I found along the way. I moved the fix to the front of the series as we probably want it to go to "maint", but the others can wait (being mostly test modifications, they should not cause regressions, but they'd possibly want more cooking time in case I broke the test suite for somebody).

The patches are:

  [1/7]: upload-pack: fix transfer.hiderefs over smart-http

    The fix.

  [2/7]: upload-pack: do not check NULL return of lookup_unknown_object

    A nearby cleanup.

  [3/7]: t: translate SIGINT to an exit
  [4/7]: t: redirect stderr GIT_TRACE to descriptor 4
  [5/7]: t: pass GIT_TRACE through Apache

    These all solve irritations I had when trying to debug the test.

  [6/7]: t5541: move run_with_cmdline_limit to test-lib.sh
  [7/7]: t5551: make EXPENSIVE test cheaper

    I had thought at first that the problem was related to large http
    fetches, but it turned out not to be. But I think these cleanups
    are a good thing, as they increase the default test coverage.

-Peff

When upload-pack advertises the refs (either for a normal, non-stateless request, or for the initial contact in a stateless one), we call for_each_ref with the send_ref function as its callback. send_ref, in turn, calls mark_our_ref, which checks whether the ref is hidden, and sets OUR_REF or HIDDEN_REF on the object as appropriate. If it is hidden, mark_our_ref also returns "1" to signal send_ref that the ref should not be advertised.

If we are not advertising refs, (i.e., the follow-up invocation by an http client to send its "want" lines), we use mark_our_ref directly as a callback to for_each_ref. Its marking does the right thing, but when it then returns "1" to for_each_ref, the latter interprets this as an error and stops iterating. As a result, we skip marking all of the refs that come lexicographically after it. Any "want" lines from the client asking for those objects will fail, as they were not properly marked with OUR_REF.

To solve this, we introduce a wrapper callback around mark_our_ref which always returns 0 (even if the ref is hidden, we want to keep iterating). We also tweak the signature of mark_our_ref to exclude unnecessary parameters that were present only to conform to the callback interface. This should make it less likely for somebody to accidentally use it as a callback in the future.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5551-http-fetch-smart.sh | 11 +++++++++++
 upload-pack.c               | 14 ++++++++++----
 2 files changed, 21 insertions(+), 4 deletions(-)

Jeff King <peff@peff.net> writes:

Nicely described in a way that the reason of the breakage and the fix is obvious to those who already know what the codepath is supposed to be doing.

I especially love this kind of future-proofing ;-).

Thanks, will queue.

We check whether the return value of lookup_unknown_object is NULL, but some code paths dereference it before our check. This turns out not to be capable of causing a segfault, though. The lookup_unknown_object function will never return NULL, since the whole point is to allocate an object struct if it does not find an existing one. So the code here is not wrong, it is just confusing. Let's just drop the NULL check.

Signed-off-by: Jeff King <peff@peff.net>
---
 upload-pack.c | 2 --
 1 file changed, 2 deletions(-)

Right now if a test script receives SIGINT (e.g., because a test was hanging and the user hit ^C), the shell exits immediately. This can be annoying if the test script did any global setup, like starting apache or git-daemon, as it will not have an opportunity to clean up after itself. A subsequent run of the test won't be able to start its own daemon, and will either fail or skip the tests.

Instead, let's trap SIGINT to make sure we do a clean shutdown, and just chain it to a normal exit (which will trigger any cleanup).

Signed-off-by: Jeff King <peff@peff.net>
---
Possibly we should catch other signals here, too, but SIGINT is the one
that has been biting me over the years (I would sometimes have a stray
apache process hanging around, and I only recently figured out what
caused it).

I think "trap ... INT" is the most portable way to say this. I would have thought numbers were more portable, but they are actually an XSI add-on. Likewise, "SIGINT" is allowed in POSIX but not required. "INT" is part of POSIX.

This is the cleanest way I could come up with to implement this. We could also just include "INT" on the same line as the die, but:

  1. Then every script which sets a handler (like lib-httpd.sh) would
     need to be modified to munge the INT trap, too.

  2. It double-calls die, then. We die from the INT handler, which
     triggers the EXIT handler. I guess we could clear the handler
     inside die() if we needed to.

So I'd rather go this route, unless there turns out to be a weird portability problem.

 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

If you run a test script like:

  GIT_TRACE=1 ./t0061-run-command.sh

you may get test failures, because some tests capture and check the stderr output from git commands (and with GIT_TRACE set to 1, the trace output will be included there).

When we see GIT_TRACE set like this, we print a warning to the user. However, we can do even better than that by just pointing it to descriptor 4, which all tests leave connected to the test script's stderr. That's likely what the user intended (and any scripts that do want to see GIT_TRACE output will set GIT_TRACE themselves).

Not only does this avoid false negatives in the tests, but it means the user will actually see trace output for git calls that redirect their stderr (whereas before, it was sometimes confusingly buried in a file).

Signed-off-by: Jeff King <peff@peff.net>
---
I only today figured out the GIT_TRACE=4 trick. Perhaps everybody else
did long ago, and I am just slow. :)

My first inclination was to mention it in the warning message below, but I cannot see a downside to automatically redirecting it. Even outside of test_expect_* we have the "4>&2" redirection, so we should hit even badly written scripts which call git outside of that environment.

 t/test-lib.sh | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Apache removes GIT_TRACE from the environment before running git-http-backend. This can make it hard to debug the server side of an http session. Let's let it through.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/lib-httpd.sh          | 1 +
 t/lib-httpd/apache.conf | 1 +
 2 files changed, 2 insertions(+)

We use this to test http pushing with a restricted commandline. Other scripts (like t5551, which does http fetching) will want to use it, too.

Signed-off-by: Jeff King <peff@peff.net>
---
As we discussed a while ago, this is the exact same code that
run_with_limited_stack uses in t7004. However, I think they are
conceptually two different things, and I could imagine a platform where
they have distinct implementations. So I did not refactor t7004 to make
use of this.

 t/t5541-http-push-smart.sh | 6 ------
 t/test-lib.sh              | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

On Fri, Mar 13, 2015 at 12:53 AM, Jeff King <peff@peff.net> wrote:

Perhaps this commentary should be moved to the commit message so that the next person who notices that run_with_limited_stack() is the same will understand why it was left alone.

We create 50,000 tags to check that we don't overflow the command-line of fetch-pack. But by using run_with_cmdline_limit, we can get the same effect with a much smaller number of tags. This makes the test fast enough that we can drop the EXPENSIVE prereq, which means people will actually run it.

It was not documented to do so, but this test was also the only test of a clone-over-http that requires multiple POSTs during the conversation. We can continue to test that by dropping http.postbuffer to its minimum size, and checking that we get two POSTs.

Signed-off-by: Jeff King <peff@peff.net>
---
This is still a slightly expensive test, but it's on par with what the
push test does, so hopefully it's OK. And it bothers me that we didn't
exercise the multiple-POST code at all in the test suite, as I assume
basically nobody runs the tests with "--long".

I don't think we can shrink it any more. We enforce a hard minimum of LARGE_PACKET_MAX on http.postbuffer, and that assumption runs through the code. Lots of things break if you try to circumvent it, and I didn't bother digging further.

 t/t5551-http-fetch-smart.sh | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

On Fri, Mar 13, 2015 at 12:41:01AM -0400, Jeff King wrote:

A note on this hang. What happens is that upload-pack sees a bogus "want" line and calls die(). The client then sits there forever, but I'm not exactly sure what it's waiting for.

This series fixes the bug that caused us to erroneously call die() in upload-pack, so the hang is "fixed". But there are other reasons to call die(); it would probably be a good thing if the client side noticed the problem and aborted.

-Peff

On Fri, Mar 13, 2015 at 11:59 AM, Jeff King <peff@peff.net> wrote:

Maybe we could install a custom die handler that also sends "ERR" line to the client before dying. Even with old clients where ERR lines are not recognized, they would see that as a sign of error and abort. The only thing to be careful is not sending ERR while we're in the middle of sending some pkt-line, and that only happens when die() is called inside packet_write() and we can catch that easily. This is for upload-pack only as the client side can also use packet_buf_write(), a bit harder to know if some pkt-line is being sent.