[PATCH] upload-pack: fix exit code when denying fetch of unreachable object ID
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Aug 16, 2023, 06:06 UTC
- Message-ID
- <fe028981d353158e9840eb035194ca15e6a2c15e.1692165840.git.ps@pks.im>
- In-Reply-To
- <pull.1572.git.1691678450757.gitgitgadget@gmail.com>
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.
This causes a subtle regression though as `die()` causes us to exit with exit code 128, whereas we now call `exit(1)`. It's not really clear whether we want to guarantee any specific error code in this case, and neither do we document anything like that. But on the other hand, it 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.
Signed-off-by: Patrick Steinhardt <ps@pks.im> ---
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.
Patrick
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); } } }
-- 2.41.0