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

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

Patrick Steinhardt <ps@pks.im> writes:

> In 7ba7c52d76 (upload-pack: fix race condition in error messages,
> 2023-08-10), we have fixed a race in t5516-fetch-push.sh where sometimes
> error messages got intermingled. This was done by splitting up the call
> to `die()` such that we print the error message before writing to the
> remote side, followed by a call to `exit(1)` afterwards.
> ...
> We have found this issue in our CI pipeline in Gitaly, which explicitly
> checks for the error code. It is very much debatable whether we should
> even be doing that in the first place given that error codes are not
> even explicitly documented here. But I think this is worth fixing anyway
> given that it seems like an unintended side effect to me and might be
> biting others, as well.
>
> If you folks agree with my reasoning, then I think we should pick this
> up before Git v2.42.0 is released.

Thanks.

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?

>  t/t5703-upload-pack-ref-in-want.sh | 2 +-
>  upload-pack.c                      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index df74f80061..fe6e056700 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -483,7 +483,7 @@ 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
>  '
>  
> diff --git a/upload-pack.c b/upload-pack.c
> index ece111c629..15f3318d6d 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -782,7 +782,7 @@ static void check_non_tip(struct upload_pack_data *data)
>  			packet_writer_error(&data->writer,
>  					    "upload-pack: not our ref %s",
>  					    oid_to_hex(&o->oid));
> -			exit(1);
> +			exit(128);
>  		}
>  	}
>  }