{"thread":{"id":"38251","subject":"[PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()","startedAt":"2014-12-25T18:25:32Z","lastAt":"2014-12-29T22:01:50Z","messageCount":3,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"254098","messageId":"20141225182534.32540.83491.chriscool@tuxfamily.org","threadId":"38251","inReplyTo":null,"subject":"[PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-12-25T18:25:32Z","receivedAt":"2014-12-25T18:25:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"When running for example \"git bisect bad HEAD\" or\n\"git bisect good master\", the parameter passed to\n\"git bisect (bad|good)\" has to be parsed into a\ncommit hash before checking if it is the expected\ncommit or not.\n\nWe could do that in is_expected_rev() or in\ncheck_expected_revs(), but it is already done in\nbisect_state(). Let's just store the hash values\nthat result from this parsing, and then reuse\nthem after all the parsing is done.\n\nThis way we can also use a for loop over these\nvalues to call bisect_write() on them, instead of\nusing eval.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\nI think that it is a better patch than the one I sent\npreviously to the list as with this one we parse revs\nonly once. \n\nMerry Christmas!\n\n git-bisect.sh | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 6cda2b5..2fc07ac 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -237,15 +237,18 @@ bisect_state() {\n \t\tcheck_expected_revs \"$rev\" ;;\n \t2,bad|*,good|*,skip)\n \t\tshift\n-\t\teval=''\n+\t\thash_list=''\n \t\tfor rev in \"$@\"\n \t\tdo\n \t\t\tsha=$(git rev-parse --verify \"$rev^{commit}\") ||\n \t\t\t\tdie \"$(eval_gettext \"Bad rev input: \\$rev\")\"\n-\t\t\teval=\"$eval bisect_write '$state' '$sha'; \"\n+\t\t\thash_list=\"$hash_list $sha\"\n \t\tdone\n-\t\teval \"$eval\"\n-\t\tcheck_expected_revs \"$@\" ;;\n+\t\tfor rev in $hash_list\n+\t\tdo\n+\t\t\tbisect_write \"$state\" \"$rev\"\n+\t\tdone\n+\t\tcheck_expected_revs $hash_list ;;\n \t*,bad)\n \t\tdie \"$(gettext \"'git bisect bad' can take only one argument.\")\" ;;\n \t*)\n-- \n2.1.2.555.gfbecd99\n"},{"id":"254149","messageId":"xmqqegriz1wi.fsf@gitster.dls.corp.google.com","threadId":"38251","inReplyTo":"20141225182534.32540.83491.chriscool@tuxfamily.org","subject":"Re: [PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-29T19:33:49Z","receivedAt":"2014-12-29T19:33:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> When running for example \"git bisect bad HEAD\" or\n> \"git bisect good master\", the parameter passed to\n> \"git bisect (bad|good)\" has to be parsed into a\n> commit hash before checking if it is the expected\n> commit or not.\n\nHmm, is that because you wrote commit object name in 40-hex in the\nEXPECTED_REV and you need to compare with what the user gave you\nwhich could be symbolic?\n\nThe conversion makes sense, but why is it a bad thing to say\n\n\tgit bisect bad maint\n\nwhen 'maint' is not what you checked out in the current bisect run\nin the first place (perhaps you checked if it is good or bad manually\nbefore you started bisecting)?\n\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 6cda2b5..2fc07ac 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -237,15 +237,18 @@ bisect_state() {\n>  \t\tcheck_expected_revs \"$rev\" ;;\n>  \t2,bad|*,good|*,skip)\n\nThis part accepts arbitrary number of revs when running good and\nskip, e.g.\n\n\tgit bisect good maint master next\n\nand it loops\n\n>  \t\tshift\n> -\t\teval=''\n> +\t\thash_list=''\n>  \t\tfor rev in \"$@\"\n> ...\n> +\t\tfor rev in $hash_list\n> +\t\tdo\n> +\t\t\tbisect_write \"$state\" \"$rev\"\n> +\t\tdone\n> +\t\tcheck_expected_revs $hash_list ;;\n\nBut check_expected_revs loops and leaves the loop early when it\nfinds anything that is not expected.\n\n... goes and looks ...\n\nHmph, I think the logic in check_expected_revs is not wrong, but\nthis helper function is grossly misnamed.  It is not checking and\nrejecting the user input---it is checking to see if it can bypass\ncheck_good_are_ancestors_of_bad() which is expensive, so when it\nsees any one of the input is not what it checked out, it just\ndisables the \"optimization\".\n\nOK, will queue.\n\nThanks.\n"},{"id":"254167","messageId":"20141229.230150.1211587976341702383.chriscool@tuxfamily.org","threadId":"38251","inReplyTo":"xmqqegriz1wi.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] bisect: parse revs before passing them to check_expected_revs()","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-12-29T22:01:50Z","receivedAt":"2014-12-29T22:01:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Junio C Hamano <gitster@pobox.com>\n\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> \n>> When running for example \"git bisect bad HEAD\" or\n>> \"git bisect good master\", the parameter passed to\n>> \"git bisect (bad|good)\" has to be parsed into a\n>> commit hash before checking if it is the expected\n>> commit or not.\n> \n> Hmm, is that because you wrote commit object name in 40-hex in the\n> EXPECTED_REV and you need to compare with what the user gave you\n> which could be symbolic?\n\nYes, that's the reason.\n \n> The conversion makes sense, but why is it a bad thing to say\n> \n> \tgit bisect bad maint\n> \n> when 'maint' is not what you checked out in the current bisect run\n> in the first place (perhaps you checked if it is good or bad manually\n> before you started bisecting)?\n\nIt is not a \"bad thing\" to test another commit than the one that has\nbeen checked out and then to say if it is \"good\" or \"bad\". But if you\ndo that then it is safer to check if a merge base should be tested.\n\nI can discuss this point further and there are indeed some\noptimisations we could implement in this area, but I think it is\nbetter to try to just fix the bug first.\n\n>> diff --git a/git-bisect.sh b/git-bisect.sh\n>> index 6cda2b5..2fc07ac 100755\n>> --- a/git-bisect.sh\n>> +++ b/git-bisect.sh\n>> @@ -237,15 +237,18 @@ bisect_state() {\n>>  \t\tcheck_expected_revs \"$rev\" ;;\n>>  \t2,bad|*,good|*,skip)\n> \n> This part accepts arbitrary number of revs when running good and\n> skip, e.g.\n> \n> \tgit bisect good maint master next\n> \n> and it loops\n\nYeah.\n\n>>  \t\tshift\n>> -\t\teval=''\n>> +\t\thash_list=''\n>>  \t\tfor rev in \"$@\"\n>> ...\n>> +\t\tfor rev in $hash_list\n>> +\t\tdo\n>> +\t\t\tbisect_write \"$state\" \"$rev\"\n>> +\t\tdone\n>> +\t\tcheck_expected_revs $hash_list ;;\n> \n> But check_expected_revs loops and leaves the loop early when it\n> finds anything that is not expected.\n> \n> ... goes and looks ...\n> \n> Hmph, I think the logic in check_expected_revs is not wrong, but\n> this helper function is grossly misnamed.  It is not checking and\n> rejecting the user input---it is checking to see if it can bypass\n> check_good_are_ancestors_of_bad() which is expensive, so when it\n> sees any one of the input is not what it checked out, it just\n> disables the \"optimization\".\n\nYeah, that's the idea. If you have a better name for\ncheck_expected_revs(), I can change it in another patch.\n\nAnd yeah, check_good_are_ancestors_of_bad() is expensive to compute\nand also expensive because it might mean that more tests have to be\nperformed by the user to be safe.\n\n> OK, will queue.\n\nThanks,\nChristian.\n"}]}