{"thread":{"id":"58880","subject":"What are cherry-pick's exit codes?","startedAt":"2022-12-01T07:03:35Z","lastAt":"2022-12-01T14:47:03Z","messageCount":5,"participants":["Sean Allred","Ævar Arnfjörð Bjarmason","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"468303","messageId":"878rjry5a7.fsf@gmail.com","threadId":"58880","inReplyTo":null,"subject":"What are cherry-pick's exit codes?","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2022-12-01T06:39:35Z","receivedAt":"2022-12-01T07:03:35Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"Hi folks,\n\nWe're developing some internal tooling wrapping git-cherry-pick and need\nto be able to distinguish its different error codes. Problem is: these\nexit codes don't seem to be documented in git-cherry-pick.txt.\n\nLooking at the source, I found myself down the rabbit-hole very quickly.\nI'm not too familiar with the coding patterns quite yet -- but I'm\npretty sure I eventually found myself redirected to git-commit in one\ncase. At that point, I thought it better to ask here.\n\nI'd like to document these exit codes in the manpage and I'm more than\nhappy to submit the patch, but I thought I'd confirm my understanding\nfirst since it's based purely on reading the cherry-pick tests:\n\nExit code:\n\n  - 0: success, sequencer complete -- no conflicts\n\n  - 1: 'success', sequencer incomplete -- conflicts encountered\n\n  - 127: fatal -- lots of reasons -- I'm guessing this is value for the\n    'return -1' and 'return error(...)' statements speckled throughout\n    the code, but it's been a long time since I cared about two's\n    complement so I may be wrong here.\n\n  - 128: fatal -- sequence is interrupted, possibly due to some other\n    fatal error, e.g., 'commit doesn't exist' or 'mainline parent number\n    doesn't exist'\n\n  - 129: fatal -- there was nothing to cherry-pick at all (e.g. empty\n    range)\n\nI'm reasonably confident about 0/1 just anecdotally -- I'm less sure\nabout everything else.\n\nObviously the actual text put in the manpage should be friendlier and\npossibly vaguer for clarity (paradoxical, perhaps, but it seems more\ndirect to say '0 for success, 1 for conflicts, and anything else is a\nfatal error'), but I wanted to make sure that I have an actually-\naccurate understanding rather than something only surface-level.\n\nTwo questions:\n\n  1. Are the exit codes actually documented somewhere already that\n     should simply be linked from git-cherry-pick.txt?\n\n  2. If not, is the above listing the exit codes accurate and complete?\n\nThanks!\n\n--\nSean Allred\n"},{"id":"468312","messageId":"221201.86zgc7gv4x.gmgdl@evledraar.gmail.com","threadId":"58880","inReplyTo":"878rjry5a7.fsf@gmail.com","subject":"Re: What are cherry-pick's exit codes?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-01T12:19:58Z","receivedAt":"2022-12-01T12:34:44Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 01 2022, Sean Allred wrote:\n\n> Hi folks,\n>\n> We're developing some internal tooling wrapping git-cherry-pick and need\n> to be able to distinguish its different error codes. Problem is: these\n> exit codes don't seem to be documented in git-cherry-pick.txt.\n>\n> Looking at the source, I found myself down the rabbit-hole very quickly.\n> I'm not too familiar with the coding patterns quite yet -- but I'm\n> pretty sure I eventually found myself redirected to git-commit in one\n> case. At that point, I thought it better to ask here.\n\nThis is definitely worth doing, and there's a bit of a rabbit hole here,\nso it'l also good you asked.\n\n> I'd like to document these exit codes in the manpage and I'm more than\n> happy to submit the patch, but I thought I'd confirm my understanding\n> first since it's based purely on reading the cherry-pick tests:\n>\n> Exit code:\n>\n>   - 0: success, sequencer complete -- no conflicts\n>\n>   - 1: 'success', sequencer incomplete -- conflicts encountered\n\nIn general I think you'd do well to follow the template of what I did in\n9144ba4cf52 (remote: add meaningful exit code on missing/existing,\n2020-10-27).\n\nIn particular, I don't think we should exhaustively document exit codes\nfor a given command, as we really have a hard time controlling all the\npossible values.\n\nRather, we should define specific non-zero values as having specific\nmeanings, as I did with \"git remote\" in that commit.\n\nWe'd also ideally leave \"exit(1)\" alone, and if we need to disambiguate\nit start with \"exit(2)\", but maybe it's easy to make it unambiguous in\nthis case, or it's already unambiguous...\n\n\n>   - 127: fatal -- lots of reasons -- I'm guessing this is value for the\n>     'return -1' and 'return error(...)' statements speckled throughout\n>     the code, but it's been a long time since I cared about two's\n>     complement so I may be wrong here.\n\nIf it's \"return -1\" you generally end up with 255 in your shell,\nalthough that's unportable both for the C language, and for POSIX.\n\nSee 19d75948ef7 (common-main.c: move non-trace2 exit() behavior out of\ntrace2.c, 2022-06-02) for some code dealing with that (where we fake up\nthe -1 to 255, for logging).\n\nBut from looking at cmd_cherry_pick() we catch any <0 return values and\ndie() on them, but maybe we exit() elsewhere? Do you have an example of\nthese.\n\n>   - 128: fatal -- sequence is interrupted, possibly due to some other\n>     fatal error, e.g., 'commit doesn't exist' or 'mainline parent number\n>     doesn't exist'\n>\n>   - 129: fatal -- there was nothing to cherry-pick at all (e.g. empty\n>     range)\n\nI think as in 9144ba4cf52 (remote: add meaningful exit code on\nmissing/existing, 2020-10-27) it's good to just leave these\nundocumented.\n\nWe typically return these when we invoke die() or usage_with_options()\n(or similar).\n\nSo, if we are documenting them (which would be a good change, as an\naside) that probably belongs in gitcli(7), we could then reference that\nfrom other man pages.\n\n> I'm reasonably confident about 0/1 just anecdotally -- I'm less sure\n> about everything else.\n>\n> Obviously the actual text put in the manpage should be friendlier and\n> possibly vaguer for clarity (paradoxical, perhaps, but it seems more\n> direct to say '0 for success, 1 for conflicts, and anything else is a\n> fatal error'), but I wanted to make sure that I have an actually-\n> accurate understanding rather than something only surface-level.\n>\n> Two questions:\n>\n>   1. Are the exit codes actually documented somewhere already that\n>      should simply be linked from git-cherry-pick.txt?\n\nNo, just when we promise specific codes for specific\ncommands. E.g. git-remote, git-config etc. come to mind.\n\n>   2. If not, is the above listing the exit codes accurate and complete?\n\nI don't know, but skimming the code I don't see the \"return 1\", unless\nby the above \" - 1\" you mean \"minus one\", i.e. 255.\n\nI.e. cmd_cherry_pick() calls run_sequencer(), which on error seems to\nreturn -1 for most things, which we then coerce to that die().\n\nBut I haven't looked in much detail, so...\n"},{"id":"468318","messageId":"87zgc7w811.fsf@gmail.com","threadId":"58880","inReplyTo":"221201.86zgc7gv4x.gmgdl@evledraar.gmail.com","subject":"Re: What are cherry-pick's exit codes?","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2022-12-01T13:38:42Z","receivedAt":"2022-12-01T13:47:13Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nÆvar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> In general I think you'd do well to follow the template of what I did in\n> 9144ba4cf52 (remote: add meaningful exit code on missing/existing,\n> 2020-10-27).\n>\n> In particular, I don't think we should exhaustively document exit codes\n> for a given command, as we really have a hard time controlling all the\n> possible values.\n>\n> Rather, we should define specific non-zero values as having specific\n> meanings, as I did with \"git remote\" in that commit.\n\nDefinitely agree (this is roughly what I've had in my WIP patch), and\nI'm glad to see precedent for an explicit 'exit status' section.\n\n> We'd also ideally leave \"exit(1)\" alone, and if we need to disambiguate\n> it start with \"exit(2)\", but maybe it's easy to make it unambiguous in\n> this case, or it's already unambiguous...\n\nI'm not 100% sure what you mean here, though. Do you mean that if\nsomething returns an exit code of '1', we shouldn't touch it without\nmore specific review since it's more likely someone will be depending on\nthat value?\n\nI think in my case, at least for what I see right now, the more urgent\nneed is getting exit codes documented -- but I can see where doing so\nmight create a situation where we can't improve the exit codes in the\nfuture -- thus implying that the 'improving' and the 'documenting' may\nneed to be done in one go.\n\n>>   - 127: fatal -- lots of reasons -- I'm guessing this is value for the\n>>     'return -1' and 'return error(...)' statements speckled throughout\n>>     the code, but it's been a long time since I cared about two's\n>>     complement so I may be wrong here.\n>\n> If it's \"return -1\" you generally end up with 255 in your shell,\n> although that's unportable both for the C language, and for POSIX.\n>\n> See 19d75948ef7 (common-main.c: move non-trace2 exit() behavior out of\n> trace2.c, 2022-06-02) for some code dealing with that (where we fake up\n> the -1 to 255, for logging).\n>\n> But from looking at cmd_cherry_pick() we catch any <0 return values and\n> die() on them, but maybe we exit() elsewhere? Do you have an example of\n> these.\n\nNo, I don't think so -- at least not by the point we hit\n'run_git_command()'.\n\n>>   - 128: fatal -- sequence is interrupted, possibly due to some other\n>>     fatal error, e.g., 'commit doesn't exist' or 'mainline parent number\n>>     doesn't exist'\n>>\n>>   - 129: fatal -- there was nothing to cherry-pick at all (e.g. empty\n>>     range)\n>\n> I think as in 9144ba4cf52 (remote: add meaningful exit code on\n> missing/existing, 2020-10-27) it's good to just leave these\n> undocumented.\n>\n> We typically return these when we invoke die() or usage_with_options()\n> (or similar).\n>\n> So, if we are documenting them (which would be a good change, as an\n> aside) that probably belongs in gitcli(7), we could then reference that\n> from other man pages.\n\nAh, gitcli(7) makes sense for these statuses. I'll see about submitting\na patch for review later tonight / this week; I'm cautiously optimistic\nthat it will be straightforward.\n\n>>   2. If not, is the above listing the exit codes accurate and complete?\n>\n> I don't know, but skimming the code I don't see the \"return 1\", unless\n> by the above \" - 1\" you mean \"minus one\", i.e. 255.\n\nI do indeed mean 'negative one'.\n\n> I.e. cmd_cherry_pick() calls run_sequencer(), which on error seems to\n> return -1 for most things, which we then coerce to that die().\n>\n> But I haven't looked in much detail, so...\n\nSeems like I (or someone) has some pretty 'fun' code tracing in my\nfuture...\n\nWhat's the next step here? Submit a patch with my best effort and suss\nout the problems during review?\n\n--\nSean Allred\n"},{"id":"468321","messageId":"40ffe56e-cec2-7130-ca4f-2186a1b7681c@dunelm.org.uk","threadId":"58880","inReplyTo":"878rjry5a7.fsf@gmail.com","subject":"Re: What are cherry-pick's exit codes?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-01T14:43:14Z","receivedAt":"2022-12-01T14:43:20Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Sean\n\nOn 01/12/2022 06:39, Sean Allred wrote:\n> Hi folks,\n> \n> We're developing some internal tooling wrapping git-cherry-pick and need\n> to be able to distinguish its different error codes. Problem is: these\n> exit codes don't seem to be documented in git-cherry-pick.txt.\n> \n> Looking at the source, I found myself down the rabbit-hole very quickly.\n> I'm not too familiar with the coding patterns quite yet -- but I'm\n> pretty sure I eventually found myself redirected to git-commit in one\n> case. At that point, I thought it better to ask here.\n> \n> I'd like to document these exit codes in the manpage and I'm more than\n> happy to submit the patch, but I thought I'd confirm my understanding\n> first since it's based purely on reading the cherry-pick tests:\n> \n> Exit code:\n> \n>    - 0: success, sequencer complete -- no conflicts\n\nI believe this is correct.\n\n>    - 1: 'success', sequencer incomplete -- conflicts encountered\n\nOne can get exit code 1 without conflicts. One example is when it cannot \ncherry-pick a commit because it would overwrite an untracked file. \nAnother example is when a picked commit would be empty because the \nchanges are already in HEAD.\n\n>    - 127: fatal -- lots of reasons -- I'm guessing this is value for the\n>      'return -1' and 'return error(...)' statements speckled throughout\n>      the code, but it's been a long time since I cared about two's\n>      complement so I may be wrong here.\n> \n>    - 128: fatal -- sequence is interrupted, possibly due to some other\n>      fatal error, e.g., 'commit doesn't exist' or 'mainline parent number\n>      doesn't exist'\n> \n>    - 129: fatal -- there was nothing to cherry-pick at all (e.g. empty\n>      range)\n\nA high exit code from die() indicates something bad happened but I'm not \nsure one can rely on the exact value to tell you what happened.\n\nBest Wishes\n\nPhillip\n\n> I'm reasonably confident about 0/1 just anecdotally -- I'm less sure\n> about everything else.\n> \n> Obviously the actual text put in the manpage should be friendlier and\n> possibly vaguer for clarity (paradoxical, perhaps, but it seems more\n> direct to say '0 for success, 1 for conflicts, and anything else is a\n> fatal error'), but I wanted to make sure that I have an actually-\n> accurate understanding rather than something only surface-level.\n> \n> Two questions:\n> \n>    1. Are the exit codes actually documented somewhere already that\n>       should simply be linked from git-cherry-pick.txt?\n> \n>    2. If not, is the above listing the exit codes accurate and complete?\n> \n> Thanks!\n> \n> --\n> Sean Allred\n"},{"id":"468322","messageId":"87v8mvw599.fsf@gmail.com","threadId":"58880","inReplyTo":"40ffe56e-cec2-7130-ca4f-2186a1b7681c@dunelm.org.uk","subject":"Re: What are cherry-pick's exit codes?","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2022-12-01T14:45:16Z","receivedAt":"2022-12-01T14:47:03Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nPhillip Wood <phillip.wood123@gmail.com> writes:\n> One can get exit code 1 without conflicts. One example is when it\n> cannot cherry-pick a commit because it would overwrite an untracked\n> file. Another example is when a picked commit would be empty because\n> the changes are already in HEAD.\n\nThese are good cases to know/test, thank you!\n\n--\nSean Allred\n"}]}