{"thread":{"id":"14388","subject":"[PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","startedAt":"2008-07-10T03:41:52Z","lastAt":"2008-07-22T06:15:32Z","messageCount":18,"participants":["Christian Couder","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"82775","messageId":"20080710054152.b051989c.chriscool@tuxfamily.org","threadId":"14388","inReplyTo":null,"subject":"[PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-07-10T03:41:52Z","receivedAt":"2008-07-10T03:41:52Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Before this patch, \"git bisect\", when it was given some good revs that\nare not ancestor of the bad rev, didn't check if the merge bases were\ngood. \"git bisect\" just supposed that the user knew what he was doing,\nand that, when he said the revs were good, he knew that it meant that\nall the revs in the history leading to the good revs were also\nconsidered good.\n\nBut in pratice, the user may not know that a good rev is not an\nancestor of the bad rev, or he may not know/remember that all revs\nleading to the good rev will be considered good. So he may give a good\nrev that is a sibling, instead of an ancestor, of the bad rev, when in\nfact there can be one rev becoming good in the branch of the good rev\n(because the bug was already fixed there for example) instead of one\nrev becoming bad in the branch of the bad rev.\n\nFor example, if there is the following history:\n\nA-B-C-D\n \\E-F\n\nand we launch \"git bisect start D F\" then only C and D would have been\nconsidered as possible first bad commit before this patch. This may be\nwrong because A, B and E may be bad too if the bug exists everywhere\nexcept in F that fixes it.\n\nThe purpose of this patch is to detect when \"git bisect\" is passed\nsome good revs that are not ancestor of the bad rev, and then to first\nask the user to test the merge bases between the good and bad revs.\n\nIf the merge bases are good then all is fine, we can continue\nbisecting. Otherwise, if one merge base is bad, it means that the\nassumption that all revs leading to the good one are good too is\nwrong and we error out. In the case where one merge base is skipped we\nissue a warning and then continue bisecting anyway.\n\nThese checks will also catch the case where good and bad have been\nmistaken. This means that we can remove the check that was done latter\non the output of \"git rev-list --bisect-vars\".\n\nIn the current implementation though, only one merge base between a\ngood and a bad rev is tested.\n\nSigned-off-by: Christian Couder \n---\n git-bisect.sh | 137 +++++++++++++++++++++++++++++++++++--------\n t/t6030-bisect-porcelain.sh | 41 +++++++++++++\n 2 files changed, 153 insertions(+), 25 deletions(-)\n\n\tLe mardi 1 juillet 2008, Junio C Hamano a écrit :\n\t> Aside from the \"test a trial merge\" idea I floated in the other message,\n\t> when we detect such a fork, perhaps we can suggest testing the merge base\n\t> version (B in your picture) first? We would immediately know as the user\n\t> would say \"B is bad\" if the topology is problematic.\n\t>\n\t> Then, we can suggest the user that breakage at D may not be a regression\n\t> but a longstanding bug that was recently fixed somewhere between B and F.\n\n\tHere is a patch to do that.\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 991b2ef..50f912f 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -242,33 +242,18 @@ bisect_auto_next() {\n \tbisect_next_check && bisect_next || :\n }\n \n-eval_rev_list() {\n-\t_eval=\"$1\"\n-\n-\teval $_eval\n-\tres=$?\n-\n-\tif [ $res -ne 0 ]; then\n-\t\techo >&2 \"'git rev-list --bisect-vars' failed:\"\n-\t\techo >&2 \"maybe you mistake good and bad revs?\"\n-\t\texit $res\n-\tfi\n-\n-\treturn $res\n-}\n-\n filter_skipped() {\n \t_eval=\"$1\"\n \t_skip=\"$2\"\n \n \tif [ -z \"$_skip\" ]; then\n-\t\teval_rev_list \"$_eval\"\n+\t\teval \"$_eval\"\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_rev_list \"$_eval\" | while read hash line\n+\teval \"$_eval\" | while read hash line\n \tdo\n \t\tcase \"$VARS,$FOUND,$TRIED,$hash\" in\n \t\t\t# We display some vars.\n@@ -331,20 +316,122 @@ exit_if_skipped_commits () {\n \tfi\n }\n \n+checkout() {\n+\t_rev=\"$1\"\n+\t_msg=\"$2\"\n+\techo \"Bisecting: $_msg\"\n+\tgit checkout -q \"$_rev\" || exit\n+\tgit show-branch \"$_rev\"\n+}\n+\n+is_among() {\n+\t_rev=\"$1\"\n+\t_list=\"$2\"\n+\tcase \"$_list\" in *$_rev*) return 0 ;; esac\n+\treturn 1\n+}\n+\n+is_merge_base_ok() {\n+\tgrep \"^$1 $2 ok$\" \"$GIT_DIR/BISECT_MERGE_BASES\" >/dev/null 2>&1\n+}\n+\n+mark_merge_base_ok() {\n+\techo \"$1 $2 ok\" >> \"$GIT_DIR/BISECT_MERGE_BASES\"\n+}\n+\n+is_testing_merge_base() {\n+\tgrep \"^testing $1$\" \"$GIT_DIR/BISECT_MERGE_BASES\" >/dev/null 2>&1\n+}\n+\n+mark_testing_merge_base() {\n+\techo \"testing $1\" >> \"$GIT_DIR/BISECT_MERGE_BASES\"\n+}\n+\n+handle_bad_merge_base() {\n+\t_badmb=\"$1\"\n+\t_g=\"$2\"\n+\tif is_testing_merge_base \"$_badmb\"; then\n+\t\tcat >&2 <&2 <&2 < \"$GIT_DIR/BISECT_ANCESTORS_OK\"\n+}\n+\n bisect_next() {\n \tcase \"$#\" in 0) ;; *) usage ;; esac\n \tbisect_autostart\n \tbisect_next_check good\n \n+\t# Get bad, good and skipped revs\n+\tbad=$(git rev-parse --verify refs/bisect/bad) &&\n+\tgood=$(git for-each-ref --format='^%(objectname)' \\\n+\t\t\"refs/bisect/good-*\" | tr '\\012' ' ') &&\n \tskip=$(git for-each-ref --format='%(objectname)' \\\n-\t\t\"refs/bisect/skip-*\" | tr '\\012' ' ') || exit\n+\t\t\"refs/bisect/skip-*\" | tr '\\012' ' ') &&\n \n+\t# Maybe some merge bases must be tested first\n+\tcheck_good_are_ancestors_of_bad \"$bad\" \"$good\" \"$skip\" || exit\n+\ttest \"$checkout_done\" -eq \"1\" && checkout_done='' && return\n+\n+\t# Get bisection information\n \tBISECT_OPT=''\n \ttest -n \"$skip\" && BISECT_OPT='--bisect-all'\n-\n-\tbad=$(git rev-parse --verify refs/bisect/bad) &&\n-\tgood=$(git for-each-ref --format='^%(objectname)' \\\n-\t\t\"refs/bisect/good-*\" | tr '\\012' ' ') &&\n \teval=\"git rev-list --bisect-vars $BISECT_OPT $good $bad --\" &&\n \teval=\"$eval $(cat \"$GIT_DIR/BISECT_NAMES\")\" &&\n \teval=$(filter_skipped \"$eval\" \"$skip\") &&\n@@ -365,9 +452,7 @@ bisect_next() {\n \t# commit is also a \"skip\" commit (see above).\n \texit_if_skipped_commits \"$bisect_rev\"\n \n-\techo \"Bisecting: $bisect_nr revisions left to test after this\"\n-\tgit checkout -q \"$bisect_rev\" || exit\n-\tgit show-branch \"$bisect_rev\"\n+\tcheckout \"$bisect_rev\" \"$bisect_nr revisions left to test after this\"\n }\n \n bisect_visualize() {\n@@ -414,6 +499,8 @@ bisect_clean_state() {\n \tdo\n \t\tgit update-ref -d $ref $hash || exit\n \tdone\n+\trm -f \"$GIT_DIR/BISECT_MERGE_BASES\" &&\n+\trm -f \"$GIT_DIR/BISECT_ANCESTORS_OK\" &&\n \trm -f \"$GIT_DIR/BISECT_LOG\" &&\n \trm -f \"$GIT_DIR/BISECT_NAMES\" &&\n \trm -f \"$GIT_DIR/BISECT_RUN\" &&\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 0626544..bf24b9a 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -350,6 +350,47 @@ test_expect_success 'bisect does not create a \"bisect\" branch' '\n \tgit branch -D bisect\n '\n \n+# This creates a \"side\" branch to test \"siblings\" cases.\n+test_expect_success 'side branch creation' '\n+\tgit bisect reset &&\n+\tgit checkout -b side $HASH4 &&\n+\tadd_line_into_file \"5(side): first line on a side branch\" hello &&\n+\tSIDE_HASH5=$(git rev-parse --verify HEAD) &&\n+\tadd_line_into_file \"6(side): second line on a side branch\" hello &&\n+\tSIDE_HASH6=$(git rev-parse --verify HEAD) &&\n+\tadd_line_into_file \"7(side): third line on a side branch\" hello &&\n+\tSIDE_HASH7=$(git rev-parse --verify HEAD)\n+'\n+\n+test_expect_success 'good merge base when good and bad are siblings' '\n+\tgit bisect start \"$HASH7\" \"$SIDE_HASH7\" > my_bisect_log.txt &&\n+\tgrep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgrep $HASH4 my_bisect_log.txt &&\n+\tgit bisect good > my_bisect_log.txt &&\n+\ttest_must_fail grep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgrep $HASH6 my_bisect_log.txt &&\n+\tgit bisect reset\n+'\n+test_expect_success 'skipped merge base when good and bad are siblings' '\n+\tgit bisect start \"$SIDE_HASH7\" \"$HASH7\" > my_bisect_log.txt &&\n+\tgrep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgrep $HASH4 my_bisect_log.txt &&\n+\tgit bisect skip > my_bisect_log.txt 2>&1 &&\n+\tgrep \"Warning\" my_bisect_log.txt &&\n+\tgrep $SIDE_HASH6 my_bisect_log.txt &&\n+\tgit bisect reset\n+'\n+\n+test_expect_success 'bad merge base when good and bad are siblings' '\n+\tgit bisect start \"$HASH7\" HEAD > my_bisect_log.txt &&\n+\tgrep \"merge base must be tested\" my_bisect_log.txt &&\n+\tgrep $HASH4 my_bisect_log.txt &&\n+\ttest_must_fail git bisect bad > my_bisect_log.txt 2>&1 &&\n+\tgrep \"merge base $HASH4 is bad\" my_bisect_log.txt &&\n+\tgrep \"fixed between $HASH4 and $SIDE_HASH7\" my_bisect_log.txt &&\n+\tgit bisect reset\n+'\n+\n #\n #\n test_done\n-- \n1.5.6.2.221.g6ad9a\n"},{"id":"82802","messageId":"alpine.DEB.1.00.0807101201210.3135@eeepc-johanness","threadId":"14388","inReplyTo":"20080710054152.b051989c.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-10T10:04:35Z","receivedAt":"2008-07-10T10:04:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 10 Jul 2008, Christian Couder wrote:\n\n> Before this patch, \"git bisect\", when it was given some good revs that \n> are not ancestor of the bad rev, didn't check if the merge bases were \n> good. \"git bisect\" just supposed that the user knew what he was doing, \n> and that, when he said the revs were good, he knew that it meant that \n> all the revs in the history leading to the good revs were also \n> considered good.\n\nWell, it is not completely relying on the user.\n\nThe common scenario before a bisect is this: something used to work _all \nthe time_, and all of a sudden, it does not anymore.\n\nSo it is expected that there is no fix in the history. Not in the current \nbranch, not in the \"good\" branch, not wherever.\n\nIn that case, you are literally guaranteed that all ancestors of a good \ncommit are good, too, because if there was a bad one, there would be a \nfix, too.\n\nThe whole idea of \"bisect\" relies on that idea, that any ancestor of a \ngood commit is good. Otherwise you'd have to check the commits one by \none, not in a bisecting manner.\n\nCiao,\nDscho\n"},{"id":"82837","messageId":"200807102126.37567.chriscool@tuxfamily.org","threadId":"14388","inReplyTo":"alpine.DEB.1.00.0807101201210.3135@eeepc-johanness","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-07-10T19:26:37Z","receivedAt":"2008-07-10T19:26:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nLe jeudi 10 juillet 2008, Johannes Schindelin a écrit :\n> Hi,\n>\n> On Thu, 10 Jul 2008, Christian Couder wrote:\n> > Before this patch, \"git bisect\", when it was given some good revs that\n> > are not ancestor of the bad rev, didn't check if the merge bases were\n> > good. \"git bisect\" just supposed that the user knew what he was doing,\n> > and that, when he said the revs were good, he knew that it meant that\n> > all the revs in the history leading to the good revs were also\n> > considered good.\n>\n> Well, it is not completely relying on the user.\n\nYeah it's relying on statistics too.\n\n> The common scenario before a bisect is this: something used to work _all\n> the time_, and all of a sudden, it does not anymore.\n\nI agree that it is the most common scenario, perhaps 95% or perhaps even 99% \nor more. But mistakes and special cases happens too.\n\nFor example people can forget to apply in the trunk a fix that is in a side \nbranch. Or they can revert in a side branch a big buggy feature just before \nmaking a minor release; but in the trunk the big buggy feature is still \nthere and may very well have introduced regressions.\n\n> So it is expected that there is no fix in the history. Not in the\n> current branch, not in the \"good\" branch, not wherever.\n>\n> In that case, you are literally guaranteed that all ancestors of a good\n> commit are good, too, because if there was a bad one, there would be a\n> fix, too.\n\nYeah, in that case...\n\n> The whole idea of \"bisect\" relies on that idea, that any ancestor of a\n> good commit is good. Otherwise you'd have to check the commits one by\n> one, not in a bisecting manner.\n\nNo, you just need to check that the merge bases between the bad rev on one \nside and each good rev on the other side are good too. And if that is the \ncase, then you can be sure that bisection will point to a first bad commit.\n\nSo the choice is between a simple and fast but not fully reliable bisect, or \na more complex and slower but fully reliable bisect.\n\nRegards,\nChristian.\n"},{"id":"82841","messageId":"7vd4llpkxq.fsf@gitster.siamese.dyndns.org","threadId":"14388","inReplyTo":"200807102126.37567.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-10T20:02:09Z","receivedAt":"2008-07-10T20:02:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Yeah, in that case...\n>\n>> The whole idea of \"bisect\" relies on that idea, that any ancestor of a\n>> good commit is good. Otherwise you'd have to check the commits one by\n>> one, not in a bisecting manner.\n\nDidn't we already discuss this at length?\n\n> No, you just need to check that the merge bases between the bad rev on one \n> side and each good rev on the other side are good too. And if that is the \n> case, then you can be sure that bisection will point to a first bad commit.\n>\n> So the choice is between a simple and fast but not fully reliable bisect, or \n> a more complex and slower but fully reliable bisect.\n\nI have not looked at your implementation, but I do not think:\n\n - The current one is not \"fully reliable\"; the user needs to know what\n he is doing. You might call it \"prone to user errors\".\n\n - \"Test this merge-base before going forward, please\" will add typically\n only one round of check (if you have more merge bases between good and\n bad, you need to test all of them are good to be sure), so it is not\n \"slower nor more complex\".\n"},{"id":"82843","messageId":"7v4p6xpkfk.fsf@gitster.siamese.dyndns.org","threadId":"14388","inReplyTo":"7vd4llpkxq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-10T20:13:03Z","receivedAt":"2008-07-10T20:13:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I have not looked at your implementation, but I do not think:\n\nSheesh. \"I *do* think\"...\n\n> - The current one is not \"fully reliable\"; the user needs to know what\n> he is doing. You might call it \"prone to user errors\".\n>\n> - \"Test this merge-base before going forward, please\" will add typically\n> only one round of check (if you have more merge bases between good and\n> bad, you need to test all of them are good to be sure), so it is not\n> \"slower nor more complex\".\n\nand I think it is a reasonable thing to do.\n"},{"id":"82857","messageId":"200807110036.17504.chriscool@tuxfamily.org","threadId":"14388","inReplyTo":"7vd4llpkxq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-07-10T22:36:17Z","receivedAt":"2008-07-10T22:36:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le jeudi 10 juillet 2008, Junio C Hamano a écrit :\n> Christian Couder writes:\n> > Yeah, in that case...\n> >\n> >> The whole idea of \"bisect\" relies on that idea, that any ancestor of a\n> >> good commit is good. Otherwise you'd have to check the commits one by\n> >> one, not in a bisecting manner.\n>\n> Didn't we already discuss this at length?\n\nYes, the thread is there:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/86951\n\n> > No, you just need to check that the merge bases between the bad rev on\n> > one side and each good rev on the other side are good too. And if that\n> > is the case, then you can be sure that bisection will point to a first\n> > bad commit.\n> >\n> > So the choice is between a simple and fast but not fully reliable\n> > bisect, or a more complex and slower but fully reliable bisect.\n>\n> I have not looked at your implementation, but I do think:\n>\n> - The current one is not \"fully reliable\"; the user needs to know what\n> he is doing. You might call it \"prone to user errors\".\n\nI agree.\n\n> - \"Test this merge-base before going forward, please\" will add typically\n> only one round of check (if you have more merge bases between good and\n> bad, you need to test all of them are good to be sure), so it is not\n> \"slower nor more complex\".\n\nBy \"slower\" I meant that it would need more rounds of check on average.\nBy \"more complex\" I meant that more code is needed.\n\nAnd I think you are right, all the merge bases need to be tested so I will \nsend a patch on top of the patch discussed here.\n\nAnother idea to fix the problem, might be to bisect as usual and at the end \nbefore saying \"X is first bad commit\" to check if some of X parents are \nmerge bases between the bad rev and a good rev. If that is the case, then \nwe could ask the user to check that these parents are all good. On average \nthis would probably reduce the number of revs the user must check.\n\nRegards,\nChristian.\n\n \n"},{"id":"82859","messageId":"alpine.DEB.1.00.0807110035180.3279@eeepc-johanness","threadId":"14388","inReplyTo":"200807110036.17504.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-10T22:38:10Z","receivedAt":"2008-07-10T22:38:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 11 Jul 2008, Christian Couder wrote:\n\n> Le jeudi 10 juillet 2008, Junio C Hamano a écrit :\n>\n> > - \"Test this merge-base before going forward, please\" will add \n> > typically only one round of check (if you have more merge bases \n> > between good and bad, you need to test all of them are good to be \n> > sure), so it is not \"slower nor more complex\".\n> \n> By \"slower\" I meant that it would need more rounds of check on average. \n> By \"more complex\" I meant that more code is needed.\n> \n> And I think you are right, all the merge bases need to be tested so I \n> will send a patch on top of the patch discussed here.\n\nGood luck. This will open the scenario where people use a proper ancestor \nas \"good\" revision. In this case, you test that. If it is \"bad\" you \nreport that it is the _first_ one.\n\nYou are opening a can of worms here, and I doubt that this is a good idea.\n\ngit-bisect as-is has very precise, and _simple_ semantics, and users \nshould really know what they are doing (i.e. not marking something as \n\"good\" which is on a branch containing a fix).\n\nTrying to be too clever here might just make the whole tool rather \nuseless.\n\nCiao,\nDscho\n"},{"id":"82888","messageId":"7vej61nxo3.fsf@gitster.siamese.dyndns.org","threadId":"14388","inReplyTo":"200807110036.17504.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-10T23:10:04Z","receivedAt":"2008-07-10T23:10:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Another idea to fix the problem, might be to bisect as usual and at the end \n> before saying \"X is first bad commit\" to check if some of X parents are \n> merge bases between the bad rev and a good rev. If that is the case, then \n> we could ask the user to check that these parents are all good. On average \n> this would probably reduce the number of revs the user must check.\n\nI do not think that would fly well. After spending long bisection cycle,\nyou will be telling the user that it was a wild goose chase (iow, the user\ndid an invalid bisection and what we stopped at was not the first\nbreakage). If the bisection topology is invalid, we should tell the user\nbefore he wastes too much time.\n\nThe sad part is that the biesction log from such an initial round would\nnot be very useful for reusing even if the user then chooses to hunt for\nthe \"fix\" on the side branch to forward port, in which case the meaning of\ngood and bad needs to be swapped from the beginning.\n"},{"id":"82889","messageId":"200807110121.34656.chriscool@tuxfamily.org","threadId":"14388","inReplyTo":"alpine.DEB.1.00.0807110035180.3279@eeepc-johanness","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-07-10T23:21:34Z","receivedAt":"2008-07-10T23:21:34Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le vendredi 11 juillet 2008, Johannes Schindelin a écrit :\n> Hi,\n>\n> On Fri, 11 Jul 2008, Christian Couder wrote:\n> > Le jeudi 10 juillet 2008, Junio C Hamano a écrit :\n> > > - \"Test this merge-base before going forward, please\" will add\n> > > typically only one round of check (if you have more merge bases\n> > > between good and bad, you need to test all of them are good to be\n> > > sure), so it is not \"slower nor more complex\".\n> >\n> > By \"slower\" I meant that it would need more rounds of check on average.\n> > By \"more complex\" I meant that more code is needed.\n> >\n> > And I think you are right, all the merge bases need to be tested so I\n> > will send a patch on top of the patch discussed here.\n>\n> Good luck. This will open the scenario where people use a proper\n> ancestor as \"good\" revision. In this case, you test that.\n\nIf a proper ancestor is used as good rev, then the merge base between this \ngood rev and the bad rev is this good rev, and as it is known to be good it \ndoesn't need to be tested by the user. I think my patch handles this well.\n\n> If it is \n> \"bad\" you report that it is the _first_ one.\n>\n> You are opening a can of worms here, and I doubt that this is a good\n> idea.\n>\n> git-bisect as-is has very precise, and _simple_ semantics, and users\n> should really know what they are doing (i.e. not marking something as\n> \"good\" which is on a branch containing a fix).\n\nI think that users don't and can't always know what they are doing. If there \nis a revert in a branch for example, how can they know all the bugs that it \nfixes?\n\n> Trying to be too clever here might just make the whole tool rather\n> useless.\n\nI don't think so. I think that if user can trust \"git bisect\" more, they \nwill use it more, and in more automated ways and everyone will win in the \nend.\n\nRegards,\nChristian.\n"},{"id":"82892","messageId":"7v7ibtnx09.fsf@gitster.siamese.dyndns.org","threadId":"14388","inReplyTo":"alpine.DEB.1.00.0807110035180.3279@eeepc-johanness","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-10T23:24:22Z","receivedAt":"2008-07-10T23:24:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> You are opening a can of worms here, and I doubt that this is a good idea.\n>\n> git-bisect as-is has very precise, and _simple_ semantics, and users \n> should really know what they are doing (i.e. not marking something as \n> \"good\" which is on a branch containing a fix).\n>\n> Trying to be too clever here might just make the whole tool rather \n> useless.\n\nHave you read the original thread yet? I do not think this is trying to\nbe clever at all, but trying to be helpful.\n\nAs you explained, bisection *requires* that Good is ancestor of Bad.\n\n ---o---?---?---?---x\n Good Bad\n\nBut in real life, things are more like \"Today's master does not work, but\nI am sure it used to work at 1.5.6.2\". I happen to always merge all of\n'maint' to 'master' before pushing them out, so a good topology is\nguaranteed, but not everybody does this (it takes careful planning what to\napply to 'maint' and where to fork topics from, and maintainers are not\nperfect):\n\n Good\n ?---o (maint)\n /\n ---?---?---?---?---x (master)\n MB Bad\n\nPeople _will_ face such a topology. If the users Know Better, they will\ntest MB=$(merge-base master maint) first to see if it is broken, and then\nthe world will have two possibilities:\n\n (1) Good\n ?---o (maint)\n /\n ---o---?---?---?---x (master)\n Good Bad\n\n (2) Good\n ?---o (maint)\n /\n ---x---?---?---?---x (master)\n Bad Bad\n\n\nIf (1), you go ahead with the usual bisection. If (2), you cannot even\nbisect. Instead, you flip good and bad to find the \"fix\" in the side\nbranch (the answer has to be either the tip of maint or one previous in\nthe picture) to forward port to, either by merging 'maint' to 'master' or\ncherry-picking.\n\nThe idea to check merge-base first is about automating this process (I\nadmit I still haven't looked at Christian's patch text yet).\n"},{"id":"82895","messageId":"200807110145.33820.chriscool@tuxfamily.org","threadId":"14388","inReplyTo":"7v7ibtnx09.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-07-10T23:45:33Z","receivedAt":"2008-07-10T23:45:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le vendredi 11 juillet 2008, Junio C Hamano a écrit :\n>\n> But in real life, things are more like \"Today's master does not work, but\n> I am sure it used to work at 1.5.6.2\". I happen to always merge all of\n> 'maint' to 'master' before pushing them out, so a good topology is\n> guaranteed, but not everybody does this (it takes careful planning what\n> to apply to 'maint' and where to fork topics from, and maintainers are\n> not perfect):\n>\n> Good\n> ?---o (maint)\n> /\n> ---?---?---?---?---x (master)\n> MB Bad\n>\n> People _will_ face such a topology. If the users Know Better, they will\n> test MB=$(merge-base master maint) first to see if it is broken, and then\n> the world will have two possibilities:\n>\n> (1) Good\n> ?---o (maint)\n> /\n> ---o---?---?---?---x (master)\n> Good Bad\n>\n> (2) Good\n> ?---o (maint)\n> /\n> ---x---?---?---?---x (master)\n> Bad Bad\n>\n>\n> If (1), you go ahead with the usual bisection. If (2), you cannot even\n> bisect. Instead, you flip good and bad to find the \"fix\" in the side\n> branch (the answer has to be either the tip of maint or one previous in\n> the picture) to forward port to, either by merging 'maint' to 'master' or\n> cherry-picking.\n>\n> The idea to check merge-base first is about automating this process (I\n> admit I still haven't looked at Christian's patch text yet).\n\nWell in case (2) my patch does:\n\n-------\n\t\tcat >&2 < writes:\n\n> Le vendredi 11 juillet 2008, Junio C Hamano a écrit :\n>>\n>> (2) Good\n>> ?---o (maint)\n>> /\n>> ---x---?---?---?---x (master)\n>> Bad Bad\n>>\n>>\n>> If (1), you go ahead with the usual bisection. If (2), you cannot even\n>> bisect. Instead, you flip good and bad to find the \"fix\" in the side\n>> branch (the answer has to be either the tip of maint or one previous in\n>> the picture) to forward port to, either by merging 'maint' to 'master' or\n>> cherry-picking.\n>>\n>> The idea to check merge-base first is about automating this process (I\n>> admit I still haven't looked at Christian's patch text yet).\n>\n> Well in case (2) my patch does:\n>\n> -------\n> \t\tcat >&2 < The merge base $_badmb is bad.\n> This means the bug has been fixed between $_badmb and $_g.\n> EOF\n> \t\texit 3\n> -------\n>\n> but this can be improved upon in some latter patches.\n\nI think such an \"improvement\" is getting close to being too clever. I\nshould have worded my description on what you would do in (2) a bit more\ncarefully.\n\n If (2), you cannot even bisect.\n\n Instead, you may decide to merge 'maint' to 'master' to get that fix.\n In which case you do not have to worry about it; you do not do\n anything further.\n\n If you cannot afford to merge 'maint' to 'master' but somehow need to\n forward port the fix by cherry-picking, you need to flip good and bad\n to find the \"fix\" in the side branch (the answer has to be either the\n tip of maint or one previous in the picture).\n"},{"id":"82900","messageId":"alpine.DEB.1.00.0807110155040.3279@eeepc-johanness","threadId":"14388","inReplyTo":"7v7ibtnx09.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-10T23:59:58Z","receivedAt":"2008-07-10T23:59:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 10 Jul 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > You are opening a can of worms here, and I doubt that this is a good idea.\n> >\n> > git-bisect as-is has very precise, and _simple_ semantics, and users \n> > should really know what they are doing (i.e. not marking something as \n> > \"good\" which is on a branch containing a fix).\n> >\n> > Trying to be too clever here might just make the whole tool rather \n> > useless.\n> \n> Have you read the original thread yet? I do not think this is trying to\n> be clever at all, but trying to be helpful.\n> \n> As you explained, bisection *requires* that Good is ancestor of Bad.\n\nThanks, I got that already.\n\nOf course it can be that the user commits a pilot error and says \"but that \nunrelated version was good\", while the fork point(s) between good and bad \nwas bad (and this might be even the intention of the user, to find _one_ \ncommit that introduced the bug).\n\nSpeaking of plural, what if some of the merge bases are good, some are \nbad?\n\nWithout carefully thinking it through, you might even _break_ the tool.\n\nAll I was proposing is keeping the current semantics, keeping the \nmechanism simple, and therefore reliable.\n\nCiao,\nDscho\n"},{"id":"82923","messageId":"7vabgolxqa.fsf@gitster.siamese.dyndns.org","threadId":"14388","inReplyTo":"alpine.DEB.1.00.0807110155040.3279@eeepc-johanness","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-11T06:51:41Z","receivedAt":"2008-07-11T06:51:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Thu, 10 Jul 2008, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n> Of course it can be that the user commits a pilot error and says \"but that \n> unrelated version was good\", while the fork point(s) between good and bad \n> was bad (and this might be even the intention of the user, to find _one_ \n> commit that introduced the bug).\n>\n> Speaking of plural, what if some of the merge bases are good, some are \n> bad?\n>\n> Without carefully thinking it through, you might even _break_ the tool.\n\nAnd you think it is better to make all of your _users_ think it through\nevery time? Isn't it more error prone?\n\n> All I was proposing is keeping the current semantics, keeping the \n> mechanism simple, and therefore reliable.\n\nWhat I suggested to Christian (sorry, I've been busy and I still haven't\nchecked if that is what was implemented in the patch -- that is why I\nsuggested you to read the original thread) was:\n\n\t- check good and bad to see if they are forked\n\n - iff they are,\n\n - have the user check merge bases and make sure they are all\n good. otherwise, the initial good/bad pair is unsuitable for\n bisection, so explain the situation and quit [*1*];\n\n\t - otherwise, keep these good markers.\n\n\t- do the usual bisection --- from this point on it is \"simple and\n reliable as it has always been\".\n\nAnd I do not think adding the \"pre-check\" stage before going into the main\npart of the processing that we have always done is against \"keeping the\nmechanism simple and reliable\".\n\n[Footnote]\n\n*1* We _could_ make things more complex by offering to swap good and bad\nat this point and then continue bisecting to find a commit to cherry-pick\nto forward port the fix. Arguably, that step would be a new code and\ncould start out to be buggy --- it _could_ be called destabilizing what\nhas been reliable, but even then, it would be a separate codepath and a\nnew bug will be something that triggers only when the user accepts that\noffer. I do not see what the big deal is that you seem to be worried\nabout.\n"},{"id":"82945","messageId":"alpine.DEB.1.00.0807111316450.3640@eeepc-johanness","threadId":"14388","inReplyTo":"7vabgolxqa.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-11T11:21:16Z","receivedAt":"2008-07-11T11:21:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 10 Jul 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Thu, 10 Jul 2008, Junio C Hamano wrote:\n> >\n> >> Johannes Schindelin writes:\n> >> \n> > Of course it can be that the user commits a pilot error and says \"but \n> > that unrelated version was good\", while the fork point(s) between good \n> > and bad was bad (and this might be even the intention of the user, to \n> > find _one_ commit that introduced the bug).\n> >\n> > Speaking of plural, what if some of the merge bases are good, some are \n> > bad?\n> >\n> > Without carefully thinking it through, you might even _break_ the tool.\n> \n> And you think it is better to make all of your _users_ think it through\n> every time? Isn't it more error prone?\n\nMaybe I am alone here, but except for that occasion that triggered my \n\"fixed/unfixed\" patch, I did have to think, in order to use git-bisect. I \nsaid \"git bisect start && git bisect bad HEAD && git bisect good \nHEAD@{1.day.ago}\", and then follow the instructions.\n\n> > All I was proposing is keeping the current semantics, keeping the \n> > mechanism simple, and therefore reliable.\n> \n> What I suggested to Christian (sorry, I've been busy and I still haven't \n> checked if that is what was implemented in the patch -- that is why I \n> suggested you to read the original thread) was:\n> \n> \t- check good and bad to see if they are forked\n> \n> - iff they are,\n> \n> - have the user check merge bases and make sure they are all\n> good. otherwise, the initial good/bad pair is unsuitable for\n> bisection, so explain the situation and quit [*1*];\n> \n> \t - otherwise, keep these good markers.\n> \n> \t- do the usual bisection --- from this point on it is \"simple and\n> reliable as it has always been\".\n\nOkay, that seems like a trivial and good patch.\n\n> *1* We _could_ make things more complex by offering to swap good and bad\n> at this point and then continue bisecting to find a commit to cherry-pick\n> to forward port the fix. Arguably, that step would be a new code and\n> could start out to be buggy --- it _could_ be called destabilizing what\n> has been reliable, but even then, it would be a separate codepath and a\n> new bug will be something that triggers only when the user accepts that\n> offer. I do not see what the big deal is that you seem to be worried\n> about.\n\nThat is what I am actually scared off. That in the wake of a nice and \ntrivial patch, things get muddied and complicated like back when \"rebase \n-i -m\" was made unusable for the layman.\n\nCiao,\nDscho\n"},{"id":"83127","messageId":"200807130837.40961.chriscool@tuxfamily.org","threadId":"14388","inReplyTo":"200807110036.17504.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-07-13T06:37:40Z","receivedAt":"2008-07-13T06:37:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le vendredi 11 juillet 2008, Christian Couder a écrit :\n> Le jeudi 10 juillet 2008, Junio C Hamano a écrit :\n> > Christian Couder writes:\n> > > Yeah, in that case...\n> > >\n> > >> The whole idea of \"bisect\" relies on that idea, that any ancestor of\n> > >> a good commit is good. Otherwise you'd have to check the commits\n> > >> one by one, not in a bisecting manner.\n> >\n> > Didn't we already discuss this at length?\n>\n> Yes, the thread is there:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/86951\n>\n> > > No, you just need to check that the merge bases between the bad rev\n> > > on one side and each good rev on the other side are good too. And if\n> > > that is the case, then you can be sure that bisection will point to a\n> > > first bad commit.\n> > >\n> > > So the choice is between a simple and fast but not fully reliable\n> > > bisect, or a more complex and slower but fully reliable bisect.\n> >\n> > I have not looked at your implementation, but I do think:\n> >\n> > - The current one is not \"fully reliable\"; the user needs to know what\n> > he is doing. You might call it \"prone to user errors\".\n>\n> I agree.\n>\n> > - \"Test this merge-base before going forward, please\" will add\n> > typically only one round of check (if you have more merge bases between\n> > good and bad, you need to test all of them are good to be sure), so it\n> > is not \"slower nor more complex\".\n>\n> By \"slower\" I meant that it would need more rounds of check on average.\n> By \"more complex\" I meant that more code is needed.\n>\n> And I think you are right, all the merge bases need to be tested so I\n> will send a patch on top of the patch discussed here.\n\nOk, here is an untested patch that should check all merge bases. I don't \nhave time right now to add tests and a good commit message but I will do \nthat when I come back from vacancy in about one week. So please consider it \nas just a RFC.\n\nThanks,\nChristian.\n\n-----8<------------\n\n[PATCH] bisect: check all merge bases instead of only one\n\n\nSigned-off-by: Christian Couder \n---\n git-bisect.sh | 29 +++++++++++++++--------------\n 1 files changed, 15 insertions(+), 14 deletions(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 50f912f..ca16609 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -375,7 +375,6 @@ Warning: the merge base between $_bad and $_g must be \nskipped.\n So we cannot be sure the first bad commit is between $_mb and $_bad.\n We continue anyway.\n EOF\n-\tmark_merge_base_ok \"$_bad\" \"$_g\"\n }\n \n check_merge_bases() {\n@@ -384,19 +383,21 @@ check_merge_bases() {\n \t_skip=\"$3\"\n \tfor _g in $_good; do\n \t\tis_merge_base_ok \"$_bad\" \"$_g\" && continue\n-\t\t_mb=$(git merge-base $_g $_bad)\n-\t\tif test \"$_mb\" = \"$_g\" || is_among \"$_mb\" \"$_good\"; then\n-\t\t\tmark_merge_base_ok \"$_bad\" \"$_g\"\n-\t\telif test \"$_mb\" = \"$_bad\"; then\n-\t\t\thandle_bad_merge_base \"$_bad\" \"$_g\"\n-\t\telif is_among \"$_mb\" \"$_skip\"; then\n-\t\t\thandle_skipped_merge_base \"$_bad\" \"$_g\" \"_mb\"\n-\t\telse\n-\t\t\tmark_testing_merge_base \"$_mb\"\n-\t\t\tcheckout \"$_mb\" \"a merge base must be tested\"\n-\t\t\tcheckout_done=1\n-\t\t\tbreak\n-\t\tfi\n+\t\tfor _mb in $(git merge-base --all $_g $_bad); do\n+\t\t\tif test \"$_mb\" = \"$_g\" || is_among \"$_mb\" \"$_good\"; then\n+\t\t\t\tcontinue\n+\t\t\telif test \"$_mb\" = \"$_bad\"; then\n+\t\t\t\thandle_bad_merge_base \"$_bad\" \"$_g\"\n+\t\t\telif is_among \"$_mb\" \"$_skip\"; then\n+\t\t\t\thandle_skipped_merge_base \"$_bad\" \"$_g\" \"_mb\"\n+\t\t\telse\n+\t\t\t\tmark_testing_merge_base \"$_mb\"\n+\t\t\t\tcheckout \"$_mb\" \"a merge base must be tested\"\n+\t\t\t\tcheckout_done=1\n+\t\t\t\treturn\n+\t\t\tfi\n+\t\tdone\n+\t\tmark_merge_base_ok \"$_bad\" \"$_g\"\n \tdone\n }\n \n-- \n1.5.6.2.221.gf54e0.dirty\n"},{"id":"83148","messageId":"alpine.DEB.1.00.0807131504130.4816@eeepc-johanness","threadId":"14388","inReplyTo":"200807130837.40961.chriscool@tuxfamily.org","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-13T13:14:46Z","receivedAt":"2008-07-13T13:14:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 13 Jul 2008, Christian Couder wrote:\n\n> [PATCH] bisect: check all merge bases instead of only one\n\nIt would have been so much nicer to squash the two patches into one, as we \ngenerally frown upon \"let's submit one patch that we know is faulty, and \nthen another that fixes it\". That's so CVS/SVN.\n\n> @@ -384,19 +383,21 @@ check_merge_bases() {\n> \t_skip=\"$3\"\n> \tfor _g in $_good; do\n\nI really wonder if we can be blessed with less ugly variable names. Maybe \nsome that do not start with an underscore for no apparent reason, and \nmaybe some that are longer than one letter, so that you have a chance to \nunderstand later what it is supposed to contain. I.e. something like:\n\n\tfor good in $good_revisions\n\tdo\n\n(You see that I also broke up the \"for\" and \"do\" into two lines, as is \ncommon practice in the rest of Git's shell code.)\n\n> \t\tis_merge_base_ok \"$_bad\" \"$_g\" && continue\n> -\t\t_mb=$(git merge-base $_g $_bad)\n> -\t\tif test \"$_mb\" = \"$_g\" || is_among \"$_mb\" \"$_good\"; then\n> -\t\t\tmark_merge_base_ok \"$_bad\" \"$_g\"\n> -\t\telif test \"$_mb\" = \"$_bad\"; then\n> -\t\t\thandle_bad_merge_base \"$_bad\" \"$_g\"\n> -\t\telif is_among \"$_mb\" \"$_skip\"; then\n> -\t\t\thandle_skipped_merge_base \"$_bad\" \"$_g\" \"_mb\"\n> -\t\telse\n> -\t\t\tmark_testing_merge_base \"$_mb\"\n> -\t\t\tcheckout \"$_mb\" \"a merge base must be tested\"\n> -\t\t\tcheckout_done=1\n> -\t\t\tbreak\n> -\t\tfi\n> +\t\tfor _mb in $(git merge-base --all $_g $_bad); do\n> +\t\t\tif test \"$_mb\" = \"$_g\" || is_among \"$_mb\" \"$_good\"; then\n> +\t\t\t\tcontinue\n> +\t\t\telif test \"$_mb\" = \"$_bad\"; then\n> +\t\t\t\thandle_bad_merge_base \"$_bad\" \"$_g\"\n> +\t\t\telif is_among \"$_mb\" \"$_skip\"; then\n> +\t\t\t\thandle_skipped_merge_base \"$_bad\" \"$_g\" \"_mb\"\n> +\t\t\telse\n> +\t\t\t\tmark_testing_merge_base \"$_mb\"\n> +\t\t\t\tcheckout \"$_mb\" \"a merge base must be tested\"\n> +\t\t\t\tcheckout_done=1\n> +\t\t\t\treturn\n> +\t\t\tfi\n> +\t\tdone\n> +\t\tmark_merge_base_ok \"$_bad\" \"$_g\"\n> \tdone\n> }\n\nI really wonder if we cannot do better than that, in terms of code \ncomplexity.\n\nFor example, I wonder if we should special-case the start, and not just \ncheck everytime if there are unchecked merge bases instead. If there are, \ncheck the first.\n\nBut that can wait until you come back from your vacation...\n\nHave fun,\nDscho\n"},{"id":"84277","messageId":"200807220815.32989.chriscool@tuxfamily.org","threadId":"14388","inReplyTo":"alpine.DEB.1.00.0807131504130.4816@eeepc-johanness","subject":"Re: [PATCH] bisect: test merge base if good rev is not an ancestor of bad rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-07-22T06:15:32Z","receivedAt":"2008-07-22T06:15:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nLe dimanche 13 juillet 2008, Johannes Schindelin a écrit :\n> Hi,\n>\n> On Sun, 13 Jul 2008, Christian Couder wrote:\n> > [PATCH] bisect: check all merge bases instead of only one\n>\n> It would have been so much nicer to squash the two patches into one, as\n> we generally frown upon \"let's submit one patch that we know is faulty,\n> and then another that fixes it\". That's so CVS/SVN.\n\nI didn't think the first one was \"faulty\". It just didn't fix everything.\n\n> > @@ -384,19 +383,21 @@ check_merge_bases() {\n> > \t_skip=\"$3\"\n> > \tfor _g in $_good; do\n>\n> I really wonder if we can be blessed with less ugly variable names. \n> Maybe some that do not start with an underscore for no apparent reason,\n\nThere is a reason though. It's because in \"bisect_next\", variables with \nthose names (\"good\", \"bad\" and \"skip\") are already used, so reusing the \nsame name is not easily possible.\n\n> and maybe some that are longer than one letter, so that you have a chance\n> to understand later what it is supposed to contain. I.e. something like:\n>\n> \tfor good in $good_revisions\n> \tdo\n>\n> (You see that I also broke up the \"for\" and \"do\" into two lines, as is\n> common practice in the rest of Git's shell code.)\n\nThere are other places in git-bisect.sh where \"for\" and \"do\" are in the same \nline. Perhaps one day I will submit a patch to fix these.\n\n> > \t\tis_merge_base_ok \"$_bad\" \"$_g\" && continue\n> > -\t\t_mb=$(git merge-base $_g $_bad)\n> > -\t\tif test \"$_mb\" = \"$_g\" || is_among \"$_mb\" \"$_good\"; then\n> > -\t\t\tmark_merge_base_ok \"$_bad\" \"$_g\"\n> > -\t\telif test \"$_mb\" = \"$_bad\"; then\n> > -\t\t\thandle_bad_merge_base \"$_bad\" \"$_g\"\n> > -\t\telif is_among \"$_mb\" \"$_skip\"; then\n> > -\t\t\thandle_skipped_merge_base \"$_bad\" \"$_g\" \"_mb\"\n> > -\t\telse\n> > -\t\t\tmark_testing_merge_base \"$_mb\"\n> > -\t\t\tcheckout \"$_mb\" \"a merge base must be tested\"\n> > -\t\t\tcheckout_done=1\n> > -\t\t\tbreak\n> > -\t\tfi\n> > +\t\tfor _mb in $(git merge-base --all $_g $_bad); do\n> > +\t\t\tif test \"$_mb\" = \"$_g\" || is_among \"$_mb\" \"$_good\"; then\n> > +\t\t\t\tcontinue\n> > +\t\t\telif test \"$_mb\" = \"$_bad\"; then\n> > +\t\t\t\thandle_bad_merge_base \"$_bad\" \"$_g\"\n> > +\t\t\telif is_among \"$_mb\" \"$_skip\"; then\n> > +\t\t\t\thandle_skipped_merge_base \"$_bad\" \"$_g\" \"_mb\"\n> > +\t\t\telse\n> > +\t\t\t\tmark_testing_merge_base \"$_mb\"\n> > +\t\t\t\tcheckout \"$_mb\" \"a merge base must be tested\"\n> > +\t\t\t\tcheckout_done=1\n> > +\t\t\t\treturn\n> > +\t\t\tfi\n> > +\t\tdone\n> > +\t\tmark_merge_base_ok \"$_bad\" \"$_g\"\n> > \tdone\n> > }\n>\n> I really wonder if we cannot do better than that, in terms of code\n> complexity.\n>\n> For example, I wonder if we should special-case the start, and not just\n> check everytime if there are unchecked merge bases instead. If there\n> are, check the first.\n\nIn fact, there was such a thing in my patch, search \nfor \"$GIT_DIR/BISECT_ANCESTORS_OK\". But it was a little bit broken if \npeople didn't test the commit that \"git bisect\" suggested.\n\nIn the next version I will post just after this mail, this is in the 2/2 \npatch (and hopefully fixed).\n\n> But that can wait until you come back from your vacation...\n>\n> Have fun,\n> Dscho\n\nThanks,\nChristian.\n"}]}