On Wed, Aug 16, 2023 at 09:44:04AM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> 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
