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

[Junio C Hamano <gitster@pobox.com>]

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.

1:  77d0f01405 ! 1:  5f33a843de upload-pack: fix exit code when denying fetch of unreachable object ID
    @@ Commit message
         seems rather clear that this is an unintended side effect of the change
         given that this change in behaviour was not mentioned at all.
     
    -    Fix this regression by exiting with 128 again and tighten one of our
    -    tests to catch such unintended side effects.
    +    Restore the status-quo by exiting with 128.  The test in t5703 to
    +    ensure that "git fetch" fails by using test_must_fail, which does
    +    not care between exiting 1 and 128, so this changes will not affect
    +    any test.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
    - ## t/t5703-upload-pack-ref-in-want.sh ##
    -@@ t/t5703-upload-pack-ref-in-want.sh: test_expect_success 'server is initially ahead - no ref in want' '
    - 	rm -rf local &&
    - 	cp -r "$LOCAL_PRISTINE" local &&
    - 	inconsistency main $(test_oid numeric) &&
    --	test_must_fail git -C local fetch 2>err &&
    -+	test_expect_code 128 git -C local fetch 2>err &&
    - 	test_i18ngrep "fatal: remote error: upload-pack: not our ref" err
    - '
    - 
    -
      ## upload-pack.c ##
     @@ upload-pack.c: static void check_non_tip(struct upload_pack_data *data)
      			packet_writer_error(&data->writer,