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

[Junio C Hamano <gitster@pobox.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].

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