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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2023, 05:12 UTC
Message-ID: <xmqqa5uqckwd.fsf@gitster.g>
In-Reply-To: <CABQH79pick0c1UVc+W8n2QtVmSJAjqXcJGtYSm0aahAFDNvE1g@mail.gmail.com>

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].

Show 5 quoted lines

> 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.

> 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.