Re: Non-zero exit code of custom filter process is not handled

From: Jeff King <peff@peff.net>
Date: Apr 27, 2023, 08:40 UTC
Message-ID: <20230427084034.GA1479657@coredump.intra.peff.net>
In-Reply-To: <a7cf2f3bbc6dcd8758e188f79d31e6e0bff44ea3e768fce482309ba532205f6b@mahalux.com>

On Wed, Apr 26, 2023 at 04:21:31PM +0200, Michael Voříšek - ČVUT FJFI wrote:

> When a custom filter process exits with a non-zero code, the git currently
> tries to decode the response, even if it should fail and let the user know
> the problem is not the payload, but instead of the filter process.

Isn't there an ordering problem here? We will not see the exit code until we wait() for the process. And we will not wait for it to finish until we get EOF on the pipe we are reading from. So we will read what it says (if anything) before checking the exit code.

What would normally happen is something like:

  1. The process encounters an error. It prints a message to stderr
     (which is usually pointing at the terminal or a log), says nothing
     else to stdout, and then exits with a failing code.

  2. Git sees EOF on the pipe, which has been closed.

  3. Git calls wait() and sees the failing exit code.

> You can reproduce this with setting a git filter to "php non-existent.php",
> in this case the filter (php binary) will exit with non-zero code. Currently
> "fatal: protocol error: bad line length character: Coul" is printed which is
> very hard to understand as even not the whole error string is shown.

The problem here is that php prints "Could not open input file: non-existent.php" to stdout, not stderr. So it really is sending garbage over the pipe from which Git expects it to be speaking a particular protocol. Whether it exits with a non-zero code or not, it has broken the protocol and Git is correct to complain.

It might be helpful in such a case if the pkt-line reader showed the whole string it received, rather than just the first four bytes which it is trying to parse (though I suspect in some cases it may require extra read() calls, as the data may still be in the pipe buffer).

-Peff