From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 25 Oct 2015 17:38:01 GMT
Subject: Re: [RFC] t5516 "75 - deny fetch unreachable SHA1, allowtipsha1inwant=true" flaky?
Message-ID: <xmqq8u6qluh2.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CFEB6E3F-48A0-41D8-A8FD-D48B806461DB@gmail.com>

Lars Schneider <larsxschneider@gmail.com> writes:

> (1) Make upload-pack wait for a response (with timeout) before it
> closes the pipe. However, I believe this would not be in line with
> the general Git philosophy stated in "git.c" (added in 7559a1be):
> "Many parts of Git have subprograms communicate via pipe, expect
> the upstream of a pipe to die with SIGPIPE when the downstream of
> a pipe does not need to read all that is written."

While I think "waiting" with "timeout" is undesirable for the
upload-pack / fetch-pack communication, you are rejecting it for a
wrong reason.

What you quoted is not even a Git philosophy.  Yes, many parts do
want to behave like that, and the "restore" needs to make sure that
such a behaviour happens by fixing the environment given to us by
the process spawning us.

That does not mean some parts of the system, if they want to run
SIGPIPE ignored, must not do so because it goes "against the
philosophy".  

It only means that these parts of the system, if they choose to,
must arrange to do so themselves.  That "restore" thing is merely to
give us a known good state to start from.

> (2) Ignore SIGPIPE errors when "fetch-pack" sends a "done" using
> "sigchain_push(SIGPIPE, SIG_IGN)" /
> "sigchain_pop(SIGPIPE)". However, then the check_pipe function
> (added in 756e676c) kicks in and we would need to work around that
> as well.

I am not sure if we are looking at the issue the right way once we
start saying 'do this _when_ fetch sends ...'.

The communication between fetch and upload goes bidirectionally in
an overlapping fashion, and at any point in the communication, not
limited to "after fetch sends 'done'", the other side can decide to
disconnect while one side is sending.  If you are lucky, you will
finish sending the current batch and try to read from the other side
and notice "the remote end hung up unexpectedly", and if you are
unlucky, you find your write cannot go through due to broken pipe.

Dying with SIGPIPE would not give us a chance to clean things up
(i.e. react to the "not our ref" error in a more application
specific way), so the current behaviour has a room for improvement,
but I suspect that changes required for "dying more nicely" would be
rather large [*1*]; I am not convinced if it is worth the effort.

That leaves us to something along this line...

> (3) Add a method "test_must_fail_or_die" to
> "test-lib-functions.sh". This method accepts exit codes 129<x<192,
> too. Use the new method in t5516.

... but I have to wonder if 129<x<192 is loosening too much, given
that the only error we expect, other than the orderly shutdown, is
reception of sigpipe.


[Footnote]

*1* We'd need to update fetch-pack.c:send_request() so that it and
    its helpers use a variant of write() that does not die with
    SIGPIPE and instead returns an error status to the caller, and
    update all the callers to react to an error, which may involve
    jumping to the receive codepath with the hope that some early
    response is already waiting for us to read and gives us an error
    message from the remote side, or something along that line.  And
    we'd probably need to update the other direction, i.e. push and
    receive, for symmetry.