Re: [PATCH] upload-pack: fix exit code when denying fetch of unreachable object ID

[Jeff King <peff@peff.net>]

On Wed, Aug 16, 2023 at 09:44:04AM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The change to the code sounds sensible in that it is a move to
> > restore the status quo, and we know that the original never intended
> > to "fix" the exit status from 128 to 1.  The test change etches the
> > status quo in stone, which is a bit more than that and might be
> > debatable, but when we someday formally declare that users should
> > not be relying on the exit status that are not documented, we would
> > hunt for the uses of test_expect_code in the tests and turn this one
> > back, and probably do the same to others that are too strict on the
> > exact exit status, so I think that half of the change is OK, at
> > least for now.
> >
> > Comments?
> 
> An alternative to make this "fix" without setting any policy is to
> do this.  That is, to remove the change to the test part and then to
> rephrase the tail end of the proposed commit log message.
> 
> I can go either way.  I personally prefer our tests not to be overly
> strict about behaviors they test, especially the ones we do not
> document.

FWIW, my gut feeling agrees with you. I do not mind restoring the
previous "128" exit code for consistency and continuity, and there is no
reason to prefer "1" here instead. But I don't know that it's something
we should be promising or keeping track of with a test.

In fact, I would say that most of these die() calls in upload-pack are
pointless. The stderr of the server-side process is frequently not even
visible to the user (because it is on the other side of an http, etc,
connection). So in practice the message is going to /dev/null or perhaps
polluting some daemon log.

From the perspective of a server operator, I would even go so far as to
suggest that upload-pack should return "0" here. The client broke
protocol, but we told them over the correct channel and then exited
cleanly. There is no error on the server side. One could argue that a
large-scale server operator may want to keep track of protocol breaks
like this, because they could be a sign of a bug. But they'd probably be
better off with instrumenting upload-pack (or any proxy that sits in
front of it) to count messages coming over the ERR channel.

I've never really made patches in that direction because for the most
part the existing die() calls aren't hurting anything, and nobody cares
much about the exit code either way. But adding a more specific test
feels like going in the wrong direction (yes, I know t5516 is already
checking for a failed code, but modulo the race problems we have with
reading ERR packets, we really would be better off checking what the
client sees, or talking directly to upload-pack itself, the way t5530
does).

-Peff