{"thread":{"id":"60100","subject":"[PATCH] upload-pack: fix race condition in error messages","startedAt":"2023-08-10T14:40:57Z","lastAt":"2023-08-17T16:04:07Z","messageCount":12,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Jeff King","Oswald Buddenhagen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480438","messageId":"pull.1572.git.1691678450757.gitgitgadget@gmail.com","threadId":"60100","inReplyTo":null,"subject":"[PATCH] upload-pack: fix race condition in error messages","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-10T14:40:50Z","receivedAt":"2023-08-10T14:40:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nTest t5516-fetch-push.sh has a test 'deny fetch unreachable SHA1,\nallowtipsha1inwant=true' that checks stderr for a specific error\nstring from the remote. In some build environments the error sent\nover the remote connection gets mingled with the error from the\ndie() statement. Since both signals are being output to the same\nfile descriptor (but from parent and child processes), the output\nwe are matching with grep gets split.\n\nTo reduce the risk of this failure, follow this process instead:\n\n1. Write an error message to stderr.\n2. Write an error message across the connection.\n3. exit(1).\n\nThis reorders the events so the error is written entirely before\nthe client receives a message from the remote, removing the race\ncondition.\n\nSigned-off-by: Derrick Stolee \n---\n upload-pack: fix race condition in error messages\n \n Here is another quick patch that we've been holding in the microsoft/git\n fork for years because it helped prevent some test flakiness, especially\n in our more involved functional test environment.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1572%2Fderrickstolee%2Fupload-pack-race-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1572/derrickstolee/upload-pack-race-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1572\n\n upload-pack.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 94751477ab2..7b25129f0f6 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -801,11 +801,12 @@ error:\n \tfor (i = 0; i < data->want_obj.nr; i++) {\n \t\tstruct object *o = data->want_obj.objects[i].item;\n \t\tif (!is_our_ref(o, data->allow_uor)) {\n+\t\t\terror(\"git upload-pack: not our ref %s\",\n+\t\t\t oid_to_hex(&o->oid));\n \t\t\tpacket_writer_error(&data->writer,\n \t\t\t\t\t \"upload-pack: not our ref %s\",\n \t\t\t\t\t oid_to_hex(&o->oid));\n-\t\t\tdie(\"git upload-pack: not our ref %s\",\n-\t\t\t oid_to_hex(&o->oid));\n+\t\t\texit(1);\n \t\t}\n \t}\n }\n\nbase-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\n-- \ngitgitgadget\n"},{"id":"480445","messageId":"xmqqy1iig9if.fsf@gitster.g","threadId":"60100","inReplyTo":"pull.1572.git.1691678450757.gitgitgadget@gmail.com","subject":"Re: [PATCH] upload-pack: fix race condition in error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-10T16:14:16Z","receivedAt":"2023-08-10T16:14:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> To reduce the risk of this failure, follow this process instead:\n>\n> 1. Write an error message to stderr.\n> 2. Write an error message across the connection.\n> 3. exit(1).\n>\n> This reorders the events so the error is written entirely before\n> the client receives a message from the remote, removing the race\n> condition.\n\nMakes sense. error() eventually goes to our own vreportf() that\nflushes, so by the time the call returns, our message should be\nalready out, and the other message has no chance to be mixed with\nit.\n\nWill queue. Thanks.\n\n> Signed-off-by: Derrick Stolee \n> ---\n> upload-pack: fix race condition in error messages\n> \n> Here is another quick patch that we've been holding in the microsoft/git\n> fork for years because it helped prevent some test flakiness, especially\n> in our more involved functional test environment.\n> \n> Thanks, -Stolee\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1572%2Fderrickstolee%2Fupload-pack-race-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1572/derrickstolee/upload-pack-race-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1572\n>\n> upload-pack.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 94751477ab2..7b25129f0f6 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -801,11 +801,12 @@ error:\n> \tfor (i = 0; i < data->want_obj.nr; i++) {\n> \t\tstruct object *o = data->want_obj.objects[i].item;\n> \t\tif (!is_our_ref(o, data->allow_uor)) {\n> +\t\t\terror(\"git upload-pack: not our ref %s\",\n> +\t\t\t oid_to_hex(&o->oid));\n> \t\t\tpacket_writer_error(&data->writer,\n> \t\t\t\t\t \"upload-pack: not our ref %s\",\n> \t\t\t\t\t oid_to_hex(&o->oid));\n> -\t\t\tdie(\"git upload-pack: not our ref %s\",\n> -\t\t\t oid_to_hex(&o->oid));\n> +\t\t\texit(1);\n> \t\t}\n> \t}\n> }\n>\n> base-commit: a82fb66fed250e16d3010c75404503bea3f0ab61\n"},{"id":"480706","messageId":"fe028981d353158e9840eb035194ca15e6a2c15e.1692165840.git.ps@pks.im","threadId":"60100","inReplyTo":"pull.1572.git.1691678450757.gitgitgadget@gmail.com","subject":"[PATCH] upload-pack: fix exit code when denying fetch of unreachable object ID","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-08-16T06:06:59Z","receivedAt":"2023-08-16T06:08:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 7ba7c52d76 (upload-pack: fix race condition in error messages,\n2023-08-10), we have fixed a race in t5516-fetch-push.sh where sometimes\nerror messages got intermingled. This was done by splitting up the call\nto `die()` such that we print the error message before writing to the\nremote side, followed by a call to `exit(1)` afterwards.\n\nThis causes a subtle regression though as `die()` causes us to exit with\nexit code 128, whereas we now call `exit(1)`. It's not really clear\nwhether we want to guarantee any specific error code in this case, and\nneither do we document anything like that. But on the other hand, it\nseems rather clear that this is an unintended side effect of the change\ngiven that this change in behaviour was not mentioned at all.\n\nFix this regression by exiting with 128 again and tighten one of our\ntests to catch such unintended side effects.\n\nSigned-off-by: Patrick Steinhardt \n---\n\nWe have found this issue in our CI pipeline in Gitaly, which explicitly\nchecks for the error code. It is very much debatable whether we should\neven be doing that in the first place given that error codes are not\neven explicitly documented here. But I think this is worth fixing anyway\ngiven that it seems like an unintended side effect to me and might be\nbiting others, as well.\n\nIf you folks agree with my reasoning, then I think we should pick this\nup before Git v2.42.0 is released.\n\nPatrick\n\n t/t5703-upload-pack-ref-in-want.sh | 2 +-\n upload-pack.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\nindex df74f80061..fe6e056700 100755\n--- a/t/t5703-upload-pack-ref-in-want.sh\n+++ b/t/t5703-upload-pack-ref-in-want.sh\n@@ -483,7 +483,7 @@ test_expect_success 'server is initially ahead - no ref in want' '\n \trm -rf local &&\n \tcp -r \"$LOCAL_PRISTINE\" local &&\n \tinconsistency main $(test_oid numeric) &&\n-\ttest_must_fail git -C local fetch 2>err &&\n+\ttest_expect_code 128 git -C local fetch 2>err &&\n \ttest_i18ngrep \"fatal: remote error: upload-pack: not our ref\" err\n '\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex ece111c629..15f3318d6d 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -782,7 +782,7 @@ static void check_non_tip(struct upload_pack_data *data)\n \t\t\tpacket_writer_error(&data->writer,\n \t\t\t\t\t \"upload-pack: not our ref %s\",\n \t\t\t\t\t oid_to_hex(&o->oid));\n-\t\t\texit(1);\n+\t\t\texit(128);\n \t\t}\n \t}\n }\n-- \n2.41.0\n\n"},{"id":"480715","messageId":"xmqqjztvezen.fsf@gitster.g","threadId":"60100","inReplyTo":"fe028981d353158e9840eb035194ca15e6a2c15e.1692165840.git.ps@pks.im","subject":"Re: [PATCH] upload-pack: fix exit code when denying fetch of unreachable object ID","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-16T16:16:00Z","receivedAt":"2023-08-16T16:16:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> In 7ba7c52d76 (upload-pack: fix race condition in error messages,\n> 2023-08-10), we have fixed a race in t5516-fetch-push.sh where sometimes\n> error messages got intermingled. This was done by splitting up the call\n> to `die()` such that we print the error message before writing to the\n> remote side, followed by a call to `exit(1)` afterwards.\n> ...\n> We have found this issue in our CI pipeline in Gitaly, which explicitly\n> checks for the error code. It is very much debatable whether we should\n> even be doing that in the first place given that error codes are not\n> even explicitly documented here. But I think this is worth fixing anyway\n> given that it seems like an unintended side effect to me and might be\n> biting others, as well.\n>\n> If you folks agree with my reasoning, then I think we should pick this\n> up before Git v2.42.0 is released.\n\nThanks.\n\nThe change to the code sounds sensible in that it is a move to\nrestore the status quo, and we know that the original never intended\nto \"fix\" the exit status from 128 to 1. The test change etches the\nstatus quo in stone, which is a bit more than that and might be\ndebatable, but when we someday formally declare that users should\nnot be relying on the exit status that are not documented, we would\nhunt for the uses of test_expect_code in the tests and turn this one\nback, and probably do the same to others that are too strict on the\nexact exit status, so I think that half of the change is OK, at\nleast for now.\n\nComments?\n\n> t/t5703-upload-pack-ref-in-want.sh | 2 +-\n> upload-pack.c | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh\n> index df74f80061..fe6e056700 100755\n> --- a/t/t5703-upload-pack-ref-in-want.sh\n> +++ b/t/t5703-upload-pack-ref-in-want.sh\n> @@ -483,7 +483,7 @@ test_expect_success 'server is initially ahead - no ref in want' '\n> \trm -rf local &&\n> \tcp -r \"$LOCAL_PRISTINE\" local &&\n> \tinconsistency main $(test_oid numeric) &&\n> -\ttest_must_fail git -C local fetch 2>err &&\n> +\ttest_expect_code 128 git -C local fetch 2>err &&\n> \ttest_i18ngrep \"fatal: remote error: upload-pack: not our ref\" err\n> '\n> \n> diff --git a/upload-pack.c b/upload-pack.c\n> index ece111c629..15f3318d6d 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -782,7 +782,7 @@ static void check_non_tip(struct upload_pack_data *data)\n> \t\t\tpacket_writer_error(&data->writer,\n> \t\t\t\t\t \"upload-pack: not our ref %s\",\n> \t\t\t\t\t oid_to_hex(&o->oid));\n> -\t\t\texit(1);\n> +\t\t\texit(128);\n> \t\t}\n> \t}\n> }\n"},{"id":"480718","messageId":"xmqq8rabey3v.fsf@gitster.g","threadId":"60100","inReplyTo":"xmqqjztvezen.fsf@gitster.g","subject":"Re: [PATCH] upload-pack: fix exit code when denying fetch of unreachable object ID","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-16T16:44:04Z","receivedAt":"2023-08-16T16:45:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The change to the code sounds sensible in that it is a move to\n> restore the status quo, and we know that the original never intended\n> to \"fix\" the exit status from 128 to 1. The test change etches the\n> status quo in stone, which is a bit more than that and might be\n> debatable, but when we someday formally declare that users should\n> not be relying on the exit status that are not documented, we would\n> hunt for the uses of test_expect_code in the tests and turn this one\n> back, and probably do the same to others that are too strict on the\n> exact exit status, so I think that half of the change is OK, at\n> least for now.\n>\n> Comments?\n\nAn alternative to make this \"fix\" without setting any policy is to\ndo this. That is, to remove the change to the test part and then to\nrephrase the tail end of the proposed commit log message.\n\nI can go either way. I personally prefer our tests not to be overly\nstrict about behaviors they test, especially the ones we do not\ndocument.\n\n1: 77d0f01405 ! 1: 5f33a843de upload-pack: fix exit code when denying fetch of unreachable object ID\n @@ Commit message\n seems rather clear that this is an unintended side effect of the change\n given that this change in behaviour was not mentioned at all.\n \n - Fix this regression by exiting with 128 again and tighten one of our\n - tests to catch such unintended side effects.\n + Restore the status-quo by exiting with 128. The test in t5703 to\n + ensure that \"git fetch\" fails by using test_must_fail, which does\n + not care between exiting 1 and 128, so this changes will not affect\n + any test.\n \n Signed-off-by: Patrick Steinhardt \n Signed-off-by: Junio C Hamano \n \n - ## t/t5703-upload-pack-ref-in-want.sh ##\n -@@ t/t5703-upload-pack-ref-in-want.sh: test_expect_success 'server is initially ahead - no ref in want' '\n - \trm -rf local &&\n - \tcp -r \"$LOCAL_PRISTINE\" local &&\n - \tinconsistency main $(test_oid numeric) &&\n --\ttest_must_fail git -C local fetch 2>err &&\n -+\ttest_expect_code 128 git -C local fetch 2>err &&\n - \ttest_i18ngrep \"fatal: remote error: upload-pack: not our ref\" err\n - '\n - \n -\n ## upload-pack.c ##\n @@ upload-pack.c: static void check_non_tip(struct upload_pack_data *data)\n \t\t\tpacket_writer_error(&data->writer,\n"},{"id":"480719","messageId":"xmqqsf8iex5v.fsf_-_@gitster.g","threadId":"60100","inReplyTo":"xmqqjztvezen.fsf@gitster.g","subject":"[Leftoverbits] exit code clean-up?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-16T17:04:28Z","receivedAt":"2023-08-16T17:05:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick earlier found that Gitaly's CI pipeline was being overly\npicky and complained about the unintended change of the exit code of\n\"git fetch\" in the affected codepath from 128 to 1 in a recent\nchange that went to 'next', made by 7ba7c52d (upload-pack: fix race\ncondition in error messages, 2023-08-10).\n\nThe thing is, we follow that exiting with 0 is success, and exiting\nwith non-zero means failure, and we generally do not specify which\nnon-zero value is used for the latter in our documentation. This\nparticular case (i.e. \"git fetch\") certainly does not say anything\nabout how failure is signaled to the calling program. \"git help\ngit\" does not have \"EXIT CODES\" section, and it is assumed that the\n\"common sense\" of older generation (gee, this project is more than\n18 years old) that exiting with 0 is success and non-zero is failure\nis shared among its users, which might not be warranted these days.\n\nWe could either\n\n * Be more prescriptive and add \"EXIT CODES\" section to each and\n every document to describe how we fail in the current code.\n\nor\n\n * Describe \"In general, 0 is success, non-zero is failure, but some\n commands may signal more than that with its non-zero exit codes\"\n in \"git help git\", and add \"EXIT CODES\" section to the manual\n page of the commands whose exit codes matter (there are a\n handful, like \"git diff --exit-code\" that explicitly says \"1\" is\n the signal that it found difference as opposed to it failing).\n\nI'd prefer if community agrees that we should do the latter, but I\nam OK if the community consensus goes the other way.\n\nIf we were to go the former, the task becomes larger but it would be\nembarrassingly parallelizable. Folks with extra time on their hand\ncan lend their eyes to tackle each command, find what exit code we\nuse in the current code, add \"EXIT CODES\" section to the documentation,\nand extend test coverage to ensure their findings will stay unless\nwe deliberately change it in the future.\n\nIf we were to go the latter, the task will be significantly smaller.\nWe need to come up with a careful phrasing to add to \"git help git\"\nand/or \"git help cli\" to give the general principle of zero vs\nnon-zero whose exact values are left unspecified and should not be\ndepended upon. We also need to identify the special cases where the\nexact values have meanings (like the \"git diff --exit-code\" example\nabove), describe them by adding \"EXIT CODES\" section to the manual\npages of these selected commands, and extend test coverage to ensure\nthese values are kept intact across future changes.\n\nComments?\n"},{"id":"480739","messageId":"xmqqa5uqckwd.fsf@gitster.g","threadId":"60100","inReplyTo":"CABQH79pick0c1UVc+W8n2QtVmSJAjqXcJGtYSm0aahAFDNvE1g@mail.gmail.com","subject":"Re: [PATCH] upload-pack: fix exit code when denying fetch of unreachable object ID","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T05:12:18Z","receivedAt":"2023-08-17T05:13:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n[jc: the message I am responding to may not be on the list archive,\n as it was multipart/alternative with text/html in it, but I think\n the main point from you can be seen by others only from the parts\n I quoted here].\n\n> While I don't think we should document that the exit code has\n> a special meaning for the builtin, adding the test will help\n> prevent another accidental change in the future. If the patch is\n> worth taking (to fix the accidental change) then I think the test\n> should stay, so we don't make this change accidentally again.\n\nI think my stance is a bit more nuanced, in that the first half of\nthe patch to make us exit with 128 is worth taking, simply because\nwe did not have to and did not intend to change the exit status, but\nthe other half of the patch, using test_expect_code in the test\nsuite, sends a wrong message that somehow exact value of non-zero\nexit status in this particular case matters.\n\nTo put it another way, if your patch to shuffle the calls for two\nerror messages, concluded with a call to exit(), were written in the\nideal world, you would have passed 128 to exit(), *and* you wouldn't\nhave added any test that says \"fetch should exit with 128 and not 1\nwhen it fails\". I aimed to massage Patrick's patch so that the\noriginal patch from you will become that patch in the ideal world\nwhen it is squashed in.\n\n> To my view, test cases can change in the future as long as\n> there is good justification in doing so. Having existing tests\n> helps to demonstrate a change in behavior.\n\nI agree with that 100%, but the thing is that the error shuffling\npatch will not escape 'next' until the upcoming release, at which\ntime we can rewind and redo 'next'. I think the first half of\nPatrick's fix would be a good material to squash into that patch,\nwhich would make the result identical to the one that would have\nbeen written in the ideal world I described above.\n\nAnd the other half would not have a place to be in that patch in the\nideal world. IOW, there is no \"change in behaviour\" we want to\ndemonstrate here, as we will pretend nothing bad happened after the\nupcoming release ;-)\n\nThanks.\n"},{"id":"480740","messageId":"20230817052742.GB3006160@coredump.intra.peff.net","threadId":"60100","inReplyTo":"xmqq8rabey3v.fsf@gitster.g","subject":"Re: [PATCH] upload-pack: fix exit code when denying fetch of unreachable object ID","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-17T05:27:42Z","receivedAt":"2023-08-17T05:28:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 16, 2023 at 09:44:04AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > The change to the code sounds sensible in that it is a move to\n> > restore the status quo, and we know that the original never intended\n> > to \"fix\" the exit status from 128 to 1. The test change etches the\n> > status quo in stone, which is a bit more than that and might be\n> > debatable, but when we someday formally declare that users should\n> > not be relying on the exit status that are not documented, we would\n> > hunt for the uses of test_expect_code in the tests and turn this one\n> > back, and probably do the same to others that are too strict on the\n> > exact exit status, so I think that half of the change is OK, at\n> > least for now.\n> >\n> > Comments?\n> \n> An alternative to make this \"fix\" without setting any policy is to\n> do this. That is, to remove the change to the test part and then to\n> rephrase the tail end of the proposed commit log message.\n> \n> I can go either way. I personally prefer our tests not to be overly\n> strict about behaviors they test, especially the ones we do not\n> document.\n\nFWIW, my gut feeling agrees with you. I do not mind restoring the\nprevious \"128\" exit code for consistency and continuity, and there is no\nreason to prefer \"1\" here instead. But I don't know that it's something\nwe should be promising or keeping track of with a test.\n\nIn fact, I would say that most of these die() calls in upload-pack are\npointless. The stderr of the server-side process is frequently not even\nvisible to the user (because it is on the other side of an http, etc,\nconnection). So in practice the message is going to /dev/null or perhaps\npolluting some daemon log.\n\nFrom the perspective of a server operator, I would even go so far as to\nsuggest that upload-pack should return \"0\" here. The client broke\nprotocol, but we told them over the correct channel and then exited\ncleanly. There is no error on the server side. One could argue that a\nlarge-scale server operator may want to keep track of protocol breaks\nlike this, because they could be a sign of a bug. But they'd probably be\nbetter off with instrumenting upload-pack (or any proxy that sits in\nfront of it) to count messages coming over the ERR channel.\n\nI've never really made patches in that direction because for the most\npart the existing die() calls aren't hurting anything, and nobody cares\nmuch about the exit code either way. But adding a more specific test\nfeels like going in the wrong direction (yes, I know t5516 is already\nchecking for a failed code, but modulo the race problems we have with\nreading ERR packets, we really would be better off checking what the\nclient sees, or talking directly to upload-pack itself, the way t5530\ndoes).\n\n-Peff\n"},{"id":"480741","messageId":"20230817053627.GC3006160@coredump.intra.peff.net","threadId":"60100","inReplyTo":"xmqqsf8iex5v.fsf_-_@gitster.g","subject":"Re: [Leftoverbits] exit code clean-up?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-08-17T05:36:27Z","receivedAt":"2023-08-17T05:37:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 16, 2023 at 10:04:28AM -0700, Junio C Hamano wrote:\n\n> We could either\n> \n> * Be more prescriptive and add \"EXIT CODES\" section to each and\n> every document to describe how we fail in the current code.\n> \n> or\n> \n> * Describe \"In general, 0 is success, non-zero is failure, but some\n> commands may signal more than that with its non-zero exit codes\"\n> in \"git help git\", and add \"EXIT CODES\" section to the manual\n> page of the commands whose exit codes matter (there are a\n> handful, like \"git diff --exit-code\" that explicitly says \"1\" is\n> the signal that it found difference as opposed to it failing).\n> \n> I'd prefer if community agrees that we should do the latter, but I\n> am OK if the community consensus goes the other way.\n\nI left some notes on upload-pack specifically elsewhere in the thread,\nin which I argue that we should definitely not lock ourselves into its\ncurrent behavior.\n\nBut in the more general sense, yeah, I think that trying to document\nspecific exit codes for each command is a mistake. It is not just \"let's\nfind which exit codes they use and document them\". I suspect it is a\nrat's nest of unplanned behaviors that come from unexpected die() calls\ndeep in the stack. We would not necessarily want to make promises about\nwhat is happening in those, nor do I think it would even be sensible to\nfind every possible exit.\n\nWe _could_ document \"128 means something really unexpected happened and\nwe called die() deep in the code\". But even that seems misleading to me,\nas we also die() for everyday shallow things (like \"the name you gave is\nnot valid\"). The value really means very little in practice, and the\nbiggest reason not to change it is that we know it doesn't conflict with\nany codes that programs _do_ promise are meaningful (like \"1\" from \"diff\n--exit-code\").\n\nSo saying \"0 is success, non-zero is failure, and some commands may\ndocument specific codes\" is the closest thing to the reality of what we\nas developers know and have planned. (Of course another project is not\njust to figure out the possible situations/codes but to catalogue and\norganize them. But that seems like an order of magnitude more work, if\nnot several orders).\n\n-Peff\n"},{"id":"480746","messageId":"ZN3nSsGOHMDQZ438@ugly","threadId":"60100","inReplyTo":"xmqqsf8iex5v.fsf_-_@gitster.g","subject":"Re: [Leftoverbits] exit code clean-up?","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-17T09:24:26Z","receivedAt":"2023-08-17T09:25:21Z","isPatch":false,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Wed, Aug 16, 2023 at 10:04:28AM -0700, Junio C Hamano wrote:\n>\"git help git\" does not have \"EXIT CODES\" section, and it is assumed \n>that the \"common sense\" of older generation [...] that exiting with 0 \n>is success and non-zero is failure is shared among its users, which \n>might not be warranted these days.\n>\nwell, that's actually a standard (exit(3) has things to say about it), \nand given how shell scripts treat exit codes, there is no wiggle room \nhere. just about every shell intro tutorial explains it.\n\n>We could either\n>\n> * Be more prescriptive and add \"EXIT CODES\" section to each and\n> every document to describe how we fail in the current code.\n>\n>or\n>\n> * Describe \"In general, 0 is success, non-zero is failure, but some\n> commands may signal more than that with its non-zero exit codes\"\n> in \"git help git\", and add \"EXIT CODES\" section to the manual\n> page of the commands whose exit codes matter (there are a\n> handful, like \"git diff --exit-code\" that explicitly says \"1\" is\n> the signal that it found difference as opposed to it failing).\n>\ni'd go with the second, with some minor modifications:\n- 1 is the by far most common non-zero error code (and it matches \n EXIT_FAILURE on all relevant systems), so it's ok to state that. it \n may be wise to actually check that commands don't deviate from it \n needlessly.\n- the canonical name of the section appears to be \"EXIT STATUS\"\n\nregards\n"},{"id":"480747","messageId":"ZN3xXfJEETBIn7nu@tanuki","threadId":"60100","inReplyTo":"xmqqa5uqckwd.fsf@gitster.g","subject":"Re: [PATCH] upload-pack: fix exit code when denying fetch of unreachable object ID","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-08-17T10:07:25Z","receivedAt":"2023-08-17T10:08:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 16, 2023 at 10:12:18PM -0700, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n> [jc: the message I am responding to may not be on the list archive,\n> as it was multipart/alternative with text/html in it, but I think\n> the main point from you can be seen by others only from the parts\n> I quoted here].\n> \n> > While I don't think we should document that the exit code has\n> > a special meaning for the builtin, adding the test will help\n> > prevent another accidental change in the future. If the patch is\n> > worth taking (to fix the accidental change) then I think the test\n> > should stay, so we don't make this change accidentally again.\n> \n> I think my stance is a bit more nuanced, in that the first half of\n> the patch to make us exit with 128 is worth taking, simply because\n> we did not have to and did not intend to change the exit status, but\n> the other half of the patch, using test_expect_code in the test\n> suite, sends a wrong message that somehow exact value of non-zero\n> exit status in this particular case matters.\n> \n> To put it another way, if your patch to shuffle the calls for two\n> error messages, concluded with a call to exit(), were written in the\n> ideal world, you would have passed 128 to exit(), *and* you wouldn't\n> have added any test that says \"fetch should exit with 128 and not 1\n> when it fails\". I aimed to massage Patrick's patch so that the\n> original patch from you will become that patch in the ideal world\n> when it is squashed in.\n\nI tend to agree with Derrick -- if we think that it is important enough\nto restore the exit code, whether that change was intentional or not,\nthen I think it makes sense to also add a test. The benefit of that test\nwouldn't be to say \"This is cast into stone\", but rather to indicate to\nthe developer that a change that they have just been doing has an\nunintentional side effect.\n\nThe problem I see with my own stance though is that if you extend it to\nthe extreme, every single `test_must_fail` would need to do exact error\ncode checking. The benefit of this would be kind of dubious though as\nlong as we do not decide to attach meaning to specific error codes.\n\nIn general I often wish that we had better ways to transport the\ncircumstances of why a specific command has failed to the caller. In\nGitaly, we often have to fall back to parsing the standard error stream\nof a command in order to figure out the failure cause, which does not\nexactly feel great given that these are rather intended to be consumed\nby a user rather than a program.\n\nWhether that information should be transported via exit codes though...\nI don't know. An exit code can only convey so much information and they\noften feel fragile to me. Documenting them explicitly would of course\nalready go a long way, but that wouldn't quite help the fact that this\nmechanism still can't convey more information than \"The command has\nfailed because of a specific root cause\". Many commands perform more\nthan a single unit of work though, so even if we know the root cause we\nstill wouldn't necessarily know where exactly it has failed. \n\nOne way to fix this would be to give commands a way to return structured\nerror data to the caller instead of relying on exit codes. But that is\nof course a bigger topic, and I feel like I'm digressing.\n\nPatrick\n\n> > To my view, test cases can change in the future as long as\n> > there is good justification in doing so. Having existing tests\n> > helps to demonstrate a change in behavior.\n> \n> I agree with that 100%, but the thing is that the error shuffling\n> patch will not escape 'next' until the upcoming release, at which\n> time we can rewind and redo 'next'. I think the first half of\n> Patrick's fix would be a good material to squash into that patch,\n> which would make the result identical to the one that would have\n> been written in the ideal world I described above.\n> \n> And the other half would not have a place to be in that patch in the\n> ideal world. IOW, there is no \"change in behaviour\" we want to\n> demonstrate here, as we will pretend nothing bad happened after the\n> upcoming release ;-)\n> \n> Thanks.\n"},{"id":"480749","messageId":"xmqq4jkxd5by.fsf@gitster.g","threadId":"60100","inReplyTo":"20230817053627.GC3006160@coredump.intra.peff.net","subject":"Re: [Leftoverbits] exit code clean-up?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-17T16:03:13Z","receivedAt":"2023-08-17T16:04:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We _could_ document \"128 means something really unexpected happened and\n> we called die() deep in the code\". But even that seems misleading to me,\n> as we also die() for everyday shallow things (like \"the name you gave is\n> not valid\"). The value really means very little in practice, and the\n> biggest reason not to change it is that we know it doesn't conflict with\n> any codes that programs _do_ promise are meaningful (like \"1\" from \"diff\n> --exit-code\").\n\nYeah, I forgot to say that we should mention 128 to tell the users\nthat it is a meaningless positive number chosen to signal a general\nerror and it is set sufficiently high so that it won't conflict with\na range of low positive numbers certain subcommands use to convey\nspecific meaning in their errors. And you said it nicely above.\n\nWith that clarified, my vote still goes to the \"do not overly tied\nto what the current implementation happens to do\" route.\n\nThanks.\n"}]}