{"thread":{"id":"54845","subject":"Feature request: 'git bisect run' option to redundantly check start and end commits","startedAt":"2020-12-18T12:32:12Z","lastAt":"2020-12-19T17:32:24Z","messageCount":7,"participants":["Ed Avis","Felipe Contreras","Christian Couder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"412554","messageId":"PH0PR11MB48875130A7DF30A7394DD3619DC30@PH0PR11MB4887.namprd11.prod.outlook.com","threadId":"54845","inReplyTo":null,"subject":"Feature request: 'git bisect run' option to redundantly check start and end commits","fromName":"Ed Avis","fromEmail":"ed.avis@qmaw.com","sentAt":"2020-12-18T12:22:04Z","receivedAt":"2020-12-18T12:32:12Z","isPatch":false,"sender":{"key":"ed.avis@qmaw.com","avatar":null},"body":"When kicking off a 'git bisect run', I have manually chosen good and bad commits, but I would like to double check that the command given to run really does succeed for the good commit and fail for the bad one. Of course I can switch to those commits and run it manually, but mistakes can happen. It's frustrating to set up a bisection and then find at the end that the first bad commit is the one immediately after 'good', because the command string just failed every time. An optional startup check of the two endpoints would only be a small slowdown in most cases, but could save a lot of time.\n\n% git bisect start\n% git bisect good abcde\n% git bisect bad bcdef\n% git bisect run --check-endpoints my gnarly command\nChecking initial 'bad' commit bcdef\nrunning my gnarly command\ncommand fails, as expected\nChecking initial 'good' commit abcde\nrunning my gnarly command\nError: the command fails for the initial 'good' commit\n\nI think that it should check the 'bad' commit first, on the assumption that a failing command usually finishes quicker than a successful one.\n\n(In my case, what went wrong is that I had put quotation marks around 'my gnarly command' when passing it on the command line. I was thinking of git rebase --exec, which requires a single quoted argument for the command to run, whereas git bisect run takes it as separate words. I think that an option to recheck the two endpoints before starting the bisection would be a useful idiot-proofing and make git bisect a nicer tool. If nothing else, it's good to be reassured that the command you gave works, since when debugging an unexplained problem, you can start to doubt these things...)\n\n--\nEd Avis \nPlease ignore any confidentiality stuff below this point.\n\nThis email and any files transmitted with it are CONFIDENTIAL and are intended solely for the use of the individual(s) or entity to whom they are addressed. Any unauthorized copying, disclosure or distribution of the material within this email is strictly forbidden. Any views or opinions presented within this email are solely those of the author and do not necessarily represent those of QMA Wadhwani LLP (QMAW) unless otherwise specifically stated. An electronic message is not binding on its sender. Any message referring to a binding agreement must be confirmed in writing and duly signed. If you have received this email in error, please notify the sender immediately and delete the original. Telephone, electronic and other communications and conversations with QMAW and/or its associated persons may be recorded and retained. QMAW is authorized and regulated by the Financial Conduct Authority. QMAW (registered in England No. OC303168) has its registered office at 9th Floor Orion House, 5 Upper St Martin's Lane, London, WC2H 9EA.\n\nPlease note that your personal information may be stored and processed in any country where we have facilities or in which we engage service providers. If you provide personal information to us by email or otherwise, you consent to the transfer of that information to countries outside of your country of residence and these countries may have different data protection rules than your country.’\n\nTo learn about our privacy policies, please use this link to read the PGIM Privacy Notice.\n"},{"id":"412567","messageId":"5fdccc25255b7_1273af2086c@natae.notmuch","threadId":"54845","inReplyTo":"PH0PR11MB48875130A7DF30A7394DD3619DC30@PH0PR11MB4887.namprd11.prod.outlook.com","subject":"RE: Feature request: 'git bisect run' option to redundantly check start and end commits","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-18T15:35:01Z","receivedAt":"2020-12-18T15:35:51Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ed Avis wrote:\n> When kicking off a 'git bisect run', I have manually chosen good and\n> bad commits, but I would like to double check that the command given\n> to run really does succeed for the good commit and fail for the bad\n> one. Of course I can switch to those commits and run it manually, but\n> mistakes can happen. It's frustrating to set up a bisection and then\n> find at the end that the first bad commit is the one immediately after\n> 'good', because the command string just failed every time. An\n> optional startup check of the two endpoints would only be a small\n> slowdown in most cases, but could save a lot of time.\n\nI like this idea.\n\nI for one have to think twice if I should do the extra check manually or\nnot, and the biggest reason why I usually don't is because it would not\nbe automated.\n\nWith an option like that I would have no excuse.\n\nHere's a quick patch to implement such feature (it doesn't apply\ndirectly, it's mostly for human eyes).\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 1f3f6e9fc5..e8adeab008 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -109,13 +109,7 @@ bisect_replay () {\n \tgit bisect--helper --bisect-auto-next || exit\n }\n \n-bisect_run () {\n-\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n-\n-\ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n-\n-\twhile true\n-\tdo\n+run_command () {\n \tcommand=\"$@\"\n \teval_gettextln \"running \\$command\"\n \t\"$@\"\n@@ -140,6 +134,41 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \telse\n \t\tstate=\"$TERM_GOOD\"\n \tfi\n+}\n+\n+run_check () {\n+\trev=$1\n+\tterm=$2\n+\tshift 2\n+\tgit checkout $rev\n+\trun_command \"$@\"\n+\techo \"# recheck $term ($state): $(git show-branch $rev)\" >>\"$GIT_DIR/BISECT_LOG\"\n+\t[[ \"$state\" != \"$term\" ]] &&\n+\tdie \"$(eval_gettext \"check failed, expected \\$rev to be \\$term\")\"\n+}\n+\n+bisect_run () {\n+\tgit bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit\n+\n+\tif [ \"$1\" = \"--recheck\" ]\n+\tthen\n+\t\tshift\n+\t\ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n+\n+\t\trev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)\n+\t\trun_check $rev \"$TERM_BAD\" \"$@\"\n+\n+\t\trev=$(git for-each-ref --format=\"%(objectname)\" \"refs/bisect/$TERM_GOOD-*\")\n+\t\trun_check $rev \"$TERM_GOOD\" \"$@\"\n+\n+\t\tgit bisect--helper --bisect-next || exit\n+\telse\n+\t\ttest -n \"$*\" || die \"$(gettext \"bisect run failed: no command provided.\")\"\n+\tfi\n+\n+\twhile true\n+\tdo\n+\t\trun_command \"$@\"\n \n \t\tgit bisect--helper --bisect-state $state >\"$GIT_DIR/BISECT_RUN\"\n \t\tres=$?\n\n-- \nFelipe Contreras\n"},{"id":"412572","messageId":"CAP8UFD1XMA7y6qXXS8h2HZXTuC8nGVkxTiMG3ZA54XJJg=gAsQ@mail.gmail.com","threadId":"54845","inReplyTo":"5fdccc25255b7_1273af2086c@natae.notmuch","subject":"Re: Feature request: 'git bisect run' option to redundantly check start and end commits","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-18T16:16:15Z","receivedAt":"2020-12-18T16:17:09Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Dec 18, 2020 at 4:39 PM Felipe Contreras\n wrote:\n>\n> Ed Avis wrote:\n> > When kicking off a 'git bisect run', I have manually chosen good and\n> > bad commits, but I would like to double check that the command given\n> > to run really does succeed for the good commit and fail for the bad\n> > one. Of course I can switch to those commits and run it manually, but\n> > mistakes can happen. It's frustrating to set up a bisection and then\n> > find at the end that the first bad commit is the one immediately after\n> > 'good', because the command string just failed every time. An\n> > optional startup check of the two endpoints would only be a small\n> > slowdown in most cases, but could save a lot of time.\n>\n> I like this idea.\n\nI like this idea too.\n\n> I for one have to think twice if I should do the extra check manually or\n> not, and the biggest reason why I usually don't is because it would not\n> be automated.\n>\n> With an option like that I would have no excuse.\n>\n> Here's a quick patch to implement such feature (it doesn't apply\n> directly, it's mostly for human eyes).\n\nThere has been a lot of work over the years to port code from shell in\ngit-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,\nif you plan to implement this feature, if you could do it directly in\nbuiltin/bisect--helper.c.\n\nThanks,\nChristian.\n"},{"id":"412576","messageId":"5fdce3a04a8b6_12eac7208b@natae.notmuch","threadId":"54845","inReplyTo":"CAP8UFD1XMA7y6qXXS8h2HZXTuC8nGVkxTiMG3ZA54XJJg=gAsQ@mail.gmail.com","subject":"Re: Feature request: 'git bisect run' option to redundantly check start and end commits","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-18T17:15:12Z","receivedAt":"2020-12-18T17:15:58Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Christian Couder wrote:\n> On Fri, Dec 18, 2020 at 4:39 PM Felipe Contreras\n> wrote:\n> > Here's a quick patch to implement such feature (it doesn't apply\n> > directly, it's mostly for human eyes).\n> \n> There has been a lot of work over the years to port code from shell in\n> git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,\n> if you plan to implement this feature, if you could do it directly in\n> builtin/bisect--helper.c.\n\nYeah, I'm aware and I might be willing to do that--which probably would\nrequire moving the whole of \"bisect run\" to C--if there was an\nindication that such a patch would actually be merged.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"412624","messageId":"CAP8UFD3AHZppV2puere=zKyRjHhywCYQcO6q4yU7jJCfF0HZJg@mail.gmail.com","threadId":"54845","inReplyTo":"5fdce3a04a8b6_12eac7208b@natae.notmuch","subject":"Re: Feature request: 'git bisect run' option to redundantly check start and end commits","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-12-19T07:46:31Z","receivedAt":"2020-12-19T07:48:33Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Dec 18, 2020 at 6:15 PM Felipe Contreras\n wrote:\n>\n> Christian Couder wrote:\n> > On Fri, Dec 18, 2020 at 4:39 PM Felipe Contreras\n> > wrote:\n> > > Here's a quick patch to implement such feature (it doesn't apply\n> > > directly, it's mostly for human eyes).\n> >\n> > There has been a lot of work over the years to port code from shell in\n> > git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,\n> > if you plan to implement this feature, if you could do it directly in\n> > builtin/bisect--helper.c.\n>\n> Yeah, I'm aware and I might be willing to do that--which probably would\n> require moving the whole of \"bisect run\" to C--if there was an\n> indication that such a patch would actually be merged.\n\nI think it would likely be merged, as it would just be finishing the\nporting git bisect to C saga.\n\nThere has been work on this by a number of GSoC students and Outreachy\ninterns. I am adding Miriam in Cc who is the last one to have been\nworking on this. I think her most current work is:\n\nhttps://gitlab.com/mirucam/git/-/commits/git-bisect-work4.3.1/\n\nwhich contains a commit to port bisect_run to C.\n\nI am not sure if she was planning on continuing the port by sending\nthis to the mailing list. Hopefully she will reply to this thread.\n\nBest,\nChristian.\n"},{"id":"412628","messageId":"5fdde67aa2e2e_1de0de208db@natae.notmuch","threadId":"54845","inReplyTo":"CAP8UFD3AHZppV2puere=zKyRjHhywCYQcO6q4yU7jJCfF0HZJg@mail.gmail.com","subject":"Re: Feature request: 'git bisect run' option to redundantly check start and end commits","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2020-12-19T11:39:38Z","receivedAt":"2020-12-19T11:45:39Z","isPatch":false,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Christian Couder wrote:\n> On Fri, Dec 18, 2020 at 6:15 PM Felipe Contreras\n> wrote:\n> >\n> > Christian Couder wrote:\n> > > On Fri, Dec 18, 2020 at 4:39 PM Felipe Contreras\n> > > wrote:\n> > > > Here's a quick patch to implement such feature (it doesn't apply\n> > > > directly, it's mostly for human eyes).\n> > >\n> > > There has been a lot of work over the years to port code from shell in\n> > > git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,\n> > > if you plan to implement this feature, if you could do it directly in\n> > > builtin/bisect--helper.c.\n> >\n> > Yeah, I'm aware and I might be willing to do that--which probably would\n> > require moving the whole of \"bisect run\" to C--if there was an\n> > indication that such a patch would actually be merged.\n> \n> I think it would likely be merged, as it would just be finishing the\n> porting git bisect to C saga.\n\nMy experience tells me otherwise. We'll see.\n\n-- \nFelipe Contreras\n"},{"id":"412655","messageId":"xmqqh7ohpurs.fsf@gitster.c.googlers.com","threadId":"54845","inReplyTo":"CAP8UFD3AHZppV2puere=zKyRjHhywCYQcO6q4yU7jJCfF0HZJg@mail.gmail.com","subject":"Re: Feature request: 'git bisect run' option to redundantly check start and end commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T17:31:35Z","receivedAt":"2020-12-19T17:32:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Fri, Dec 18, 2020 at 6:15 PM Felipe Contreras\n> wrote:\n>>\n>> > There has been a lot of work over the years to port code from shell in\n>> > git-bisect.sh to C in builtin/bisect--helper.c. So it would be nice,\n>> > if you plan to implement this feature, if you could do it directly in\n>> > builtin/bisect--helper.c.\n>>\n>> Yeah, I'm aware and I might be willing to do that--which probably would\n>> require moving the whole of \"bisect run\" to C--if there was an\n>> indication that such a patch would actually be merged.\n>\n> I think it would likely be merged, as it would just be finishing the\n> porting git bisect to C saga.\n\nI agree that the issue such a patch series would try to address\n(i.e. \"reimplement 'bisect run' in C\") may be worth solving, and I\ndo not offhand see why it would become a reason of not merging.\n\nBut the \"cause\" is mere prerequisite to consider a particular\niteration of such a series for inclusion. I think it is\nirresponsible to say \"would likely\" before seeing any patch to base\nour assessment on.\n\nIf the design is poorly done (e.g. it does not solve the problem it\naims to solve well, it makes it hard to explain to end users what is\ngoing on [*1*], or makes a poor design choice and paints us in a\ncorner we cannot easily escape from, ...), or if the execution is\npoorly done (e.g. it is unmaintainable, it breaks other things by\nmistake, it makes future changes harder, ...), it would not be\npossible for the project to take the series.\n\nUntil reviewers comments aimed to help by pointing out these defects\nare addressed, that is. And if the series stops getting updated to\naddress such defects, it may end up getting not merged at all.\n\n\n[Footnote]\n\n*1* This obviously applies only for a series that makes externally\n visible behaviour change. \"rewrite 'bisect run' in C\" may not\n have end-user visible effect (and if it does, such a regression\n is a reason to say \"this is not yet ready\").\n"}]}