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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4231 bytes --]

On Wed, Aug 16, 2023 at 10:12:18PM -0700, Junio C Hamano wrote:
> Derrick Stolee <derrickstolee@github.com> writes:
> 
> [jc: the message I am responding to may not be on the list archive,
>  as it was multipart/alternative with text/html in it, but I think
>  the main point from you can be seen by others only from the parts
>  I quoted here].
> 
> > While I don't think we should document that the exit code has
> > a special meaning for the builtin, adding the test will help
> > prevent another accidental change in the future. If the patch is
> > worth taking (to fix the accidental change) then I think the test
> > should stay, so we don't make this change accidentally again.
> 
> I think my stance is a bit more nuanced, in that the first half of
> the patch to make us exit with 128 is worth taking, simply because
> we did not have to and did not intend to change the exit status, but
> the other half of the patch, using test_expect_code in the test
> suite, sends a wrong message that somehow exact value of non-zero
> exit status in this particular case matters.
> 
> To put it another way, if your patch to shuffle the calls for two
> error messages, concluded with a call to exit(), were written in the
> ideal world, you would have passed 128 to exit(), *and* you wouldn't
> have added any test that says "fetch should exit with 128 and not 1
> when it fails".  I aimed to massage Patrick's patch so that the
> original patch from you will become that patch in the ideal world
> when it is squashed in.

I tend to agree with Derrick -- if we think that it is important enough
to restore the exit code, whether that change was intentional or not,
then I think it makes sense to also add a test. The benefit of that test
wouldn't be to say "This is cast into stone", but rather to indicate to
the developer that a change that they have just been doing has an
unintentional side effect.

The problem I see with my own stance though is that if you extend it to
the extreme, every single `test_must_fail` would need to do exact error
code checking. The benefit of this would be kind of dubious though as
long as we do not decide to attach meaning to specific error codes.

In general I often wish that we had better ways to transport the
circumstances of why a specific command has failed to the caller. In
Gitaly, we often have to fall back to parsing the standard error stream
of a command in order to figure out the failure cause, which does not
exactly feel great given that these are rather intended to be consumed
by a user rather than a program.

Whether that information should be transported via exit codes though...
I don't know. An exit code can only convey so much information and they
often feel fragile to me. Documenting them explicitly would of course
already go a long way, but that wouldn't quite help the fact that this
mechanism still can't convey more information than "The command has
failed because of a specific root cause". Many commands perform more
than a single unit of work though, so even if we know the root cause we
still wouldn't necessarily know where exactly it has failed. 

One way to fix this would be to give commands a way to return structured
error data to the caller instead of relying on exit codes. But that is
of course a bigger topic, and I feel like I'm digressing.

Patrick

> > To my view, test cases can change in the future as long as
> > there is good justification in doing so. Having existing tests
> > helps to demonstrate a change in behavior.
> 
> I agree with that 100%, but the thing is that the error shuffling
> patch will not escape 'next' until the upcoming release, at which
> time we can rewind and redo 'next'.  I think the first half of
> Patrick's fix would be a good material to squash into that patch,
> which would make the result identical to the one that would have
> been written in the ideal world I described above.
> 
> And the other half would not have a place to be in that patch in the
> ideal world.  IOW, there is no "change in behaviour" we want to
> demonstrate here, as we will pretend nothing bad happened after the
> upcoming release ;-)
> 
> Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]