{"thread":{"id":"18280","subject":"[PATCH 7/7] bisect: use \"rev-list --bisect-skip\" and remove \"filter_skipped\" function","startedAt":"2009-03-12T07:51:40Z","lastAt":"2009-03-13T05:29:25Z","messageCount":3,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"107777","messageId":"20090312085140.c22cd336.chriscool@tuxfamily.org","threadId":"18280","inReplyTo":null,"subject":"[PATCH 7/7] bisect: use \"rev-list --bisect-skip\" and remove \"filter_skipped\" function","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-03-12T07:51:40Z","receivedAt":"2009-03-12T07:51:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Use the new \"--bisect-skip\" option of \"git rev-list\" that should be\nfaster and safer instead of the old \"filter_skipped\" shell function.\n\nAs the output is a little bit different we have to change the code\nthat interpret the results. But these changes improve code clarity.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n git-bisect.sh |   89 ++++++++------------------------------------------------\n 1 files changed, 13 insertions(+), 76 deletions(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex e313bde..31e7cff 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -279,76 +279,13 @@ bisect_auto_next() {\n \tbisect_next_check && bisect_next || :\n }\n \n-filter_skipped() {\n+eval_and_string_together() {\n \t_eval=\"$1\"\n-\t_skip=\"$2\"\n-\n-\tif [ -z \"$_skip\" ]; then\n-\t\teval \"$_eval\" | {\n-\t\t\twhile read line\n-\t\t\tdo\n-\t\t\t\techo \"$line &&\"\n-\t\t\tdone\n-\t\t\techo ':'\n-\t\t}\n-\t\treturn\n-\tfi\n \n-\t# Let's parse the output of:\n-\t# \"git rev-list --bisect-vars --bisect-all ...\"\n \teval \"$_eval\" | {\n-\t\tVARS= FOUND= TRIED=\n-\t\twhile read hash line\n+\t\twhile read line\n \t\tdo\n-\t\t\tcase \"$VARS,$FOUND,$TRIED,$hash\" in\n-\t\t\t1,*,*,*)\n-\t\t\t\t# \"bisect_foo=bar\" read from rev-list output.\n-\t\t\t\techo \"$hash &&\"\n-\t\t\t\t;;\n-\t\t\t,*,*,---*)\n-\t\t\t\t# Separator\n-\t\t\t\t;;\n-\t\t\t,,,bisect_rev*)\n-\t\t\t\t# We had nothing to search.\n-\t\t\t\techo \"bisect_rev= &&\"\n-\t\t\t\tVARS=1\n-\t\t\t\t;;\n-\t\t\t,,*,bisect_rev*)\n-\t\t\t\t# We did not find a good bisect rev.\n-\t\t\t\t# This should happen only if the \"bad\"\n-\t\t\t\t# commit is also a \"skip\" commit.\n-\t\t\t\techo \"bisect_rev='$TRIED' &&\"\n-\t\t\t\tVARS=1\n-\t\t\t\t;;\n-\t\t\t,,*,*)\n-\t\t\t\t# We are searching.\n-\t\t\t\tTRIED=\"${TRIED:+$TRIED|}$hash\"\n-\t\t\t\tcase \"$_skip\" in\n-\t\t\t\t*$hash*) ;;\n-\t\t\t\t*)\n-\t\t\t\t\techo \"bisect_rev=$hash &&\"\n-\t\t\t\t\techo \"bisect_tried='$TRIED' &&\"\n-\t\t\t\t\tFOUND=1\n-\t\t\t\t\t;;\n-\t\t\t\tesac\n-\t\t\t\t;;\n-\t\t\t,1,*,bisect_rev*)\n-\t\t\t\t# We have already found a rev to be tested.\n-\t\t\t\tVARS=1\n-\t\t\t\t;;\n-\t\t\t,1,*,*)\n-\t\t\t\t;;\n-\t\t\t*)\n-\t\t\t\t# Unexpected input\n-\t\t\t\techo \"die 'filter_skipped error'\"\n-\t\t\t\tdie \"filter_skipped error \" \\\n-\t\t\t\t    \"VARS: '$VARS' \" \\\n-\t\t\t\t    \"FOUND: '$FOUND' \" \\\n-\t\t\t\t    \"TRIED: '$TRIED' \" \\\n-\t\t\t\t    \"hash: '$hash' \" \\\n-\t\t\t\t    \"line: '$line'\"\n-\t\t\t\t;;\n-\t\t\tesac\n+\t\t\techo \"$line &&\"\n \t\tdone\n \t\techo ':'\n \t}\n@@ -356,10 +293,12 @@ filter_skipped() {\n \n exit_if_skipped_commits () {\n \t_tried=$1\n-\tif expr \"$_tried\" : \".*[|].*\" > /dev/null ; then\n+\t_bad=$2\n+\tif test -n \"$_tried\" ; then\n \t\techo \"There are only 'skip'ped commit left to test.\"\n \t\techo \"The first bad commit could be any of:\"\n \t\techo \"$_tried\" | tr '[|]' '[\\012]'\n+\t\ttest -n \"$_bad\" && echo \"$_bad\"\n \t\techo \"We cannot bisect more!\"\n \t\texit 2\n \tfi\n@@ -490,28 +429,26 @@ bisect_next() {\n \ttest \"$?\" -eq \"1\" && return\n \n \t# Get bisection information\n-\tBISECT_OPT=''\n-\ttest -n \"$skip\" && BISECT_OPT='--bisect-all'\n-\teval=\"git rev-list --bisect-vars $BISECT_OPT $good $bad --\" &&\n+\tskip=$(echo $skip | tr ' ' ',')\n+\teval=\"git rev-list --bisect-skip=$skip $good $bad --\" &&\n \teval=\"$eval $(cat \"$GIT_DIR/BISECT_NAMES\")\" &&\n-\teval=$(filter_skipped \"$eval\" \"$skip\") &&\n+\teval=$(eval_and_string_together \"$eval\") &&\n \teval \"$eval\" || exit\n \n \tif [ -z \"$bisect_rev\" ]; then\n+\t\t# We should exit here only if the \"bad\"\n+\t\t# commit is also a \"skip\" commit (see above).\n+\t\texit_if_skipped_commits \"$bisect_tried\"\n \t\techo \"$bad was both good and bad\"\n \t\texit 1\n \tfi\n \tif [ \"$bisect_rev\" = \"$bad\" ]; then\n-\t\texit_if_skipped_commits \"$bisect_tried\"\n+\t\texit_if_skipped_commits \"$bisect_tried\" \"$bad\"\n \t\techo \"$bisect_rev is first bad commit\"\n \t\tgit diff-tree --pretty $bisect_rev\n \t\texit 0\n \tfi\n \n-\t# We should exit here only if the \"bad\"\n-\t# commit is also a \"skip\" commit (see above).\n-\texit_if_skipped_commits \"$bisect_rev\"\n-\n \tbisect_checkout \"$bisect_rev\" \"$bisect_nr revisions left to test after this (roughly $bisect_steps steps)\"\n }\n \n-- \n1.6.2.83.g012a16.dirty\n"},{"id":"107780","messageId":"7viqmfkuyt.fsf@gitster.siamese.dyndns.org","threadId":"18280","inReplyTo":"20090312085140.c22cd336.chriscool@tuxfamily.org","subject":"Re: [PATCH 7/7] bisect: use \"rev-list --bisect-skip\" and remove \"filter_skipped\" function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-12T08:26:34Z","receivedAt":"2009-03-12T08:26:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"These (except for the first one which I do not think belongs to the\nseries) look more or less straightforward changes.\n\nOne worrysome thing that the series introduces is that you used to hold\nall the skipped ones in a shell variable $skip and fed it internally to\nthe filter_skipped shell function, but now you give them from the command\nline.  When you have tons of skipped commits, this can easily bust the\ncommand line length limit on some system, while the shell may still be Ok\nwith such a large string variable.\n\nBecause the operations in rev-list related to bisect are very closely tied\nto the implementation of the bisect Porcelain anyway, I am wondering if it\nmakes more sense to just feed the name of the toplevel refname hierarchy,\ni.e. \"refs/bisect\", as a rev-list parameter and let rev-list enumerate\nwhat is skipped, which commits are good and which ones are bad.\n"},{"id":"107902","messageId":"200903130629.25452.chriscool@tuxfamily.org","threadId":"18280","inReplyTo":"7viqmfkuyt.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 7/7] bisect: use \"rev-list --bisect-skip\" and remove \"filter_skipped\" function","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-03-13T05:29:25Z","receivedAt":"2009-03-13T05:29:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le jeudi 12 mars 2009, Junio C Hamano a écrit :\n> These (except for the first one which I do not think belongs to the\n> series) look more or less straightforward changes.\n\nI added the first one to the series because I use strbuf_split with the new \nbehavior in the series.\n\n> One worrysome thing that the series introduces is that you used to hold\n> all the skipped ones in a shell variable $skip and fed it internally to\n> the filter_skipped shell function, but now you give them from the command\n> line.  When you have tons of skipped commits, this can easily bust the\n> command line length limit on some system, while the shell may still be Ok\n> with such a large string variable.\n\nYeah. I will send a patch (8/7) that makes \"git rev-list\" read the skipped \ncommits from stdin when \"--bisect-skip\" is passed, and that changes \ngit-bisect.sh accordingly. If this behavior is ok, then I can reorder the \nseries if you want.\n\n> Because the operations in rev-list related to bisect are very closely\n> tied to the implementation of the bisect Porcelain anyway, I am wondering\n> if it makes more sense to just feed the name of the toplevel refname\n> hierarchy, i.e. \"refs/bisect\", as a rev-list parameter and let rev-list\n> enumerate what is skipped, which commits are good and which ones are bad.\n\nYeah that could be an improvement, but then the enumeration will happen once \nin git-bisect.sh and once in \"git rev-list\", instead of just once in \ngit-bisect.sh. And we also have to deal with the other command line \narguments passed to \"git rev-list\" on top of the enumerated commits from \nthe refname hierachy, so I don't think that would be very consistent.\n\nBut perhaps we could create a new \"git rev-bisect\" builtin plumbing command \nthat could do that. And later we could improve this new command so that it \nchecks merge bases (like what is done in bisect_next in git-bisect.sh) and \nso that it also checks out the source code of the new revision to be tested \n(like what is done in bisect_checkout in git-bisect.sh).\n\nThanks,\nChristian.\n"}]}