{"thread":{"id":"53861","subject":"Git bisect run should check for the existence of the script","startedAt":"2020-07-15T08:08:32Z","lastAt":"2020-07-17T19:31:04Z","messageCount":5,"participants":["Jean Abou Samra","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"401577","messageId":"9cea8076-be96-19c5-c368-288193daedd8@abou-samra.fr","threadId":"53861","inReplyTo":null,"subject":"Git bisect run should check for the existence of the script","fromName":"Jean Abou Samra","fromEmail":"jean@abou-samra.fr","sentAt":"2020-07-15T08:08:28Z","receivedAt":"2020-07-15T08:08:32Z","isPatch":false,"sender":{"key":"jean@abou-samra.fr","avatar":null},"body":"Hi,\n\nWhen using 'git bisect run', if the name of the script is misspelled or \nif the script was not made executable, 'git bisect' considers it to be a \nfailure and stops at the first revision after the one claimed good. It \nwould be better in my eyes to error out.\n\n$ git bisect start\n$ git bisect bad HEAD\n$ git bisect good HEAD~10\nBisecting: 4 revisions left to test after this (roughly 2 steps)\n[344dce312a0cf86d5a5772d54843cc179acaf6e3] bpo-41228: Fix /a/are/ in \nmonthcalendar() descripton (GH-21372)\n$ git bisect run ./non-existent.sh\nrunning ./non-existent.sh\n/usr/lib/git-core/git-bisect: 247: ./non-existent.sh: not found\nBisecting: 2 revisions left to test after this (roughly 1 step)\n[6fc732a2116e2c42b0431bb7e2a21719351af755] Fix typo in docs: 'created by \nth' -> 'created by the' (GH-21384)\nrunning ./non-existent.sh\n/usr/lib/git-core/git-bisect: 247: ./non-existent.sh: not found\nBisecting: 0 revisions left to test after this (roughly 0 steps)\n[8182cc2e68a3c6ea5d5342fed3f1c76b0521fbc1] bpo-39573: Use the Py_TYPE() \nmacro (GH-21433)\nrunning ./non-existent.sh\n/usr/lib/git-core/git-bisect: 247: ./non-existent.sh: not found\n8182cc2e68a3c6ea5d5342fed3f1c76b0521fbc1 is the first bad commit\ncommit 8182cc2e68a3c6ea5d5342fed3f1c76b0521fbc1\nAuthor: Victor Stinner <vstinner@python.org>\nDate:   Fri Jul 10 12:40:38 2020 +0200\n\n     bpo-39573: Use the Py_TYPE() macro (GH-21433)\n\n     Replace obj->ob_type with Py_TYPE(obj).\n\n  Modules/_elementtree.c       | 2 +-\n  Objects/abstract.c           | 4 ++--\n  Objects/genericaliasobject.c | 2 +-\n  Objects/unicodeobject.c      | 4 ++--\n  PC/_msi.c                    | 6 +++---\n  PC/winreg.c                  | 4 ++--\n  Tools/scripts/combinerefs.py | 2 +-\n  7 files changed, 12 insertions(+), 12 deletions(-)\nbisect run success\n\n\nBest regards,\nJean Abou Samra\n\n"},{"id":"401590","messageId":"xmqq1rlclte2.fsf@gitster.c.googlers.com","threadId":"53861","inReplyTo":"9cea8076-be96-19c5-c368-288193daedd8@abou-samra.fr","subject":"Re: Git bisect run should check for the existence of the script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-15T14:55:49Z","receivedAt":"2020-07-15T14:56:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jean Abou Samra <jean@abou-samra.fr> writes:\n\n> $ git bisect run ./non-existent.sh\n> running ./non-existent.sh\n> /usr/lib/git-core/git-bisect: 247: ./non-existent.sh: not found\n> Bisecting: 2 revisions left to test after this (roughly 1 step)\n> [6fc732a2116e2c42b0431bb7e2a21719351af755] Fix typo in docs: 'created\n> by th' -> 'created by the' (GH-21384)\n> running ./non-existent.sh\n> /usr/lib/git-core/git-bisect: 247: ./non-existent.sh: not found\n> Bisecting: 0 revisions left to test after this (roughly 0 steps)\n\nYes, it would be nice if \"git bisect run\" can reliably tell that it\ngot a \"not found\" error and not a \"test performed by the script did\nnot pass\" and stop at the first failure.\n\nOn the other hand, the \"./non-existent.sh\" script could be part of\nthe tracked contents (i.e. some revisions have it and the working\ntree has it when they get checked out, some revisions don't and the\nworking tree does not have it), and the user is trying to find the\nfirst revision that stopped having a working script in its tree.  In\nsuch a case, the script that does not exist and the script that fails\nneed to be treated the same way by \"git bisect run\" as failures.  \n\nSo... I dunno.\n"},{"id":"401701","messageId":"8c683b56-d17a-78ce-67bb-477e5b307df9@abou-samra.fr","threadId":"53861","inReplyTo":"xmqq1rlclte2.fsf@gitster.c.googlers.com","subject":"Re: Git bisect run should check for the existence of the script","fromName":"Jean Abou Samra","fromEmail":"jean@abou-samra.fr","sentAt":"2020-07-17T15:09:12Z","receivedAt":"2020-07-17T15:09:18Z","isPatch":false,"sender":{"key":"jean@abou-samra.fr","avatar":null},"body":"\nLe 15/07/2020 à 16:55, Junio C Hamano a écrit :\n> Jean Abou Samra <jean@abou-samra.fr> writes:\n>\n>> $ git bisect run ./non-existent.sh\n>> running ./non-existent.sh\n>> /usr/lib/git-core/git-bisect: 247: ./non-existent.sh: not found\n>> Bisecting: 2 revisions left to test after this (roughly 1 step)\n>> [6fc732a2116e2c42b0431bb7e2a21719351af755] Fix typo in docs: 'created\n>> by th' -> 'created by the' (GH-21384)\n>> running ./non-existent.sh\n>> /usr/lib/git-core/git-bisect: 247: ./non-existent.sh: not found\n>> Bisecting: 0 revisions left to test after this (roughly 0 steps)\n> Yes, it would be nice if \"git bisect run\" can reliably tell that it\n> got a \"not found\" error and not a \"test performed by the script did\n> not pass\" and stop at the first failure.\n>\n> On the other hand, the \"./non-existent.sh\" script could be part of\n> the tracked contents (i.e. some revisions have it and the working\n> tree has it when they get checked out, some revisions don't and the\n> working tree does not have it), and the user is trying to find the\n> first revision that stopped having a working script in its tree.  In\n> such a case, the script that does not exist and the script that fails\n> need to be treated the same way by \"git bisect run\" as failures.\n>\n> So... I dunno.\n\nPerhapsa --not-found-as-failure option could help?\n\nBest,\nJean Abou Samra\n\n"},{"id":"401708","messageId":"xmqqeepadk8q.fsf@gitster.c.googlers.com","threadId":"53861","inReplyTo":"8c683b56-d17a-78ce-67bb-477e5b307df9@abou-samra.fr","subject":"Re: Git bisect run should check for the existence of the script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-17T19:17:25Z","receivedAt":"2020-07-17T19:17:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jean Abou Samra <jean@abou-samra.fr> writes:\n\n> Le 15/07/2020 à 16:55, Junio C Hamano a écrit :\n>> Jean Abou Samra <jean@abou-samra.fr> writes:\n>>\n>>> $ git bisect run ./non-existent.sh\n>>> running ./non-existent.sh\n>>> /usr/lib/git-core/git-bisect: 247: ./non-existent.sh: not found\n> ...\n>> On the other hand, the \"./non-existent.sh\" script could be part of\n>> the tracked contents (i.e. some revisions have it and the working\n>> tree has it when they get checked out, some revisions don't and the\n>> working tree does not have it), and the user is trying to find the\n>> first revision that stopped having a working script in its tree.  In\n>> such a case, the script that does not exist and the script that fails\n>> need to be treated the same way by \"git bisect run\" as failures.\n>>\n>> So... I dunno.\n>\n> Perhapsa --not-found-as-failure option could help?\n\nDo you mean a new option must be passed if the end-user expects the\nscript to always exist across revisions, or the script is not\ntracked to begin with?  It feels somewhat backwards and the effort\nby the end-user to always type the option is better spent to make\nsure there is no typo on the command line.\n\nBesides, we need to take into account that \"bisect run\" takes an\narbitrary shell snippet, e.g. the user may try to switch between two\nscripts, test-1 and test-2, based on some condition, and wrote this:\n\n  $ git bisect run \"if some-condition; then ./test-1; else ./test-3; fi\"\n\nbut made a typo in the name of test-2.  It would be noticed after\nattempting to run the \"if ... fi\" as a scriptlet by the returned\nstatus from it being 127 (command not found), which is not treated\nany specially by \"git bisect run\".  So a typo in the script name in\nthe above example cannot be distinguished from this error where the\nuser wanted to run test-1 under some condition, but wanted to\ndeclare that the revision is bad if that condition did not hold,\ni.e.\n\n  $ git bisect run \"if some-condition; then ./test-1; else (exit 127); fi\"\n\nEven if we somehow could tell these two apart, we cannot pick the\nfirst scriptlet apart and guess which substring in it were meant as\na filename with a typo.\n\nIt may be possible to forbid the general use of exit code 127 and\ninstead reserve it to signal \"we know that the test script is so\nbroken (this includes the case where it does not even exist) that\nusing it with 'bisect run' is meaningless; please stop immediately.\"\nand nothing else, just like exit code 125 is reserved for \"this\nrevision is not testable and cannot say good or bad\", but making\nsuch a change retroactively is not to be done very lightly.\n\nSo, again, I dunno.\n\n git-bisect.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 7a8f796251..5c03bf8533 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -122,7 +122,7 @@ bisect_run () {\n \t\tres=$?\n \n \t\t# Check for really bad run error.\n-\t\tif [ $res -lt 0 -o $res -ge 128 ]\n+\t\tif [ $res -lt 0 -o $res -ge 128 -o $res -eq 127 ]\n \t\tthen\n \t\t\teval_gettextln \"bisect run failed:\n exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n"},{"id":"401709","messageId":"xmqqa6zydjm4.fsf@gitster.c.googlers.com","threadId":"53861","inReplyTo":"xmqqeepadk8q.fsf@gitster.c.googlers.com","subject":"Re: Git bisect run should check for the existence of the script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-17T19:30:59Z","receivedAt":"2020-07-17T19:31:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Do you mean a new option must be passed if the end-user expects the\n> script to always exist across revisions, or the script is not\n> tracked to begin with?  It feels somewhat backwards and the effort\n> by the end-user to always type the option is better spent to make\n> sure there is no typo on the command line.\n\nI forgot to follow up on this part.\n\nIt does not change the conclusion that it needs to be done carefully\nif we chose to retroactivelyreserve return code 127 for our own use,\nbut such a backward incompatible change can easily be worked around\nif users relied on the current behaviour that a missing (tracked)\nscript would mark the revision \"bad\" without being a fatal error in\n\"git bisect run\".  Instead of\n\n    git bisect run \"./tracked-script\"\n\nthey can just do\n\n    git bisect run \"test -f ./tracked-script && ./tracked-script\"\n\nand the problem is solved ;-)\n"}]}