{"thread":{"id":"13103","subject":"[PATCH 2/2] bisect: warn if given dubious tag or branch as rev","startedAt":"2008-04-14T03:41:51Z","lastAt":"2008-04-15T07:05:14Z","messageCount":3,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"74302","messageId":"20080414054151.52e55e7f.chriscool@tuxfamily.org","threadId":"13103","inReplyTo":null,"subject":"[PATCH 2/2] bisect: warn if given dubious tag or branch as rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-04-14T03:41:51Z","receivedAt":"2008-04-14T03:41:51Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch refactors rev parsing code in a new \"bisect_parse_rev\"\nfunction, and adds warnings in case a rev with a name that is the\nsame as one of the bisect subcommands is given where a revision\nis expected.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n git-bisect.sh               |   47 ++++++++++++++++++++++++++++++------------\n t/t6030-bisect-porcelain.sh |   21 +++++++++++++++++++\n 2 files changed, 54 insertions(+), 14 deletions(-)\n\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 6b43461..a090b97 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -61,6 +61,31 @@ bisect_autostart() {\n \t}\n }\n \n+bisect_parse_rev() {\n+\trev=\"$1\"\n+\tdont_die_on_wrong_rev=\"$2\"\n+\tname=\"$rev^{commit}\"\n+\tdubious_rev_name=''\n+\n+\tcase \"$rev\" in\n+\t\tHEAD)\n+\t\t\tname='HEAD' ;;\n+\t\thelp|start|bad|good|skip|next|reset|visualize|replay|log|run)\n+\t\t\tdubious_rev_name='yes' ;;\n+\tesac\n+\n+\tsha=$(git rev-parse --verify \"$name\" 2>/dev/null)\n+\tstatus=$?\n+\n+\ttest $status -eq 0 && test -n \"$dubious_rev_name\" &&\n+\t\techo >&2 \"A tag or branch named '$rev' exists and will be used.\" \\\n+\t\t\t\"This may be a syntax error, please check.\"\n+\n+\ttest -n \"$dont_die_on_wrong_rev\" && return $status\n+\n+\ttest $status -eq 0 || die \"Bad rev input: $rev\"\n+}\n+\n bisect_start() {\n \t#\n \t# Verify HEAD. If we were bisecting before this, reset to the\n@@ -68,7 +93,7 @@ bisect_start() {\n \t#\n \thead=$(GIT_DIR=\"$GIT_DIR\" git symbolic-ref HEAD) ||\n \thead=$(GIT_DIR=\"$GIT_DIR\" git rev-parse --verify HEAD) ||\n-\tdie \"Bad HEAD - I need a HEAD\"\n+\t\tdie \"Bad HEAD - I need a HEAD\"\n \tstart_head=''\n \tcase \"$head\" in\n \trefs/heads/bisect)\n@@ -98,9 +123,9 @@ bisect_start() {\n \t#\n \t# Check for one bad and then some good revisions.\n \t#\n-\thas_double_dash=0\n+\tdont_die_on_wrong_rev='yes'\n \tfor arg; do\n-\t    case \"$arg\" in --) has_double_dash=1; break ;; esac\n+\t    case \"$arg\" in --) dont_die_on_wrong_rev=''; break ;; esac\n \tdone\n \torig_args=$(sq \"$@\")\n \tbad_seen=0\n@@ -113,16 +138,12 @@ bisect_start() {\n \t\tbreak\n \t\t;;\n \t    *)\n-\t\trev=$(git rev-parse --verify \"$arg^{commit}\" 2>/dev/null) || {\n-\t\t    test $has_double_dash -eq 1 &&\n-\t\t        die \"'$arg' does not appear to be a valid revision\"\n-\t\t    break\n-\t\t}\n+\t\tbisect_parse_rev \"$arg\" \"$dont_die_on_wrong_rev\" || break\n \t\tcase $bad_seen in\n \t\t0) state='bad' ; bad_seen=1 ;;\n \t\t*) state='good' ;;\n \t\tesac\n-\t\teval=\"$eval bisect_write '$state' '$rev' 'nolog'; \"\n+\t\teval=\"$eval bisect_write '$state' '$sha' 'nolog'; \"\n \t\tshift\n \t\t;;\n \t    esac\n@@ -156,16 +177,14 @@ bisect_state() {\n \t0,*)\n \t\tdie \"Please call 'bisect_state' with at least one argument.\" ;;\n \t1,bad|1,good|1,skip)\n-\t\trev=$(git rev-parse --verify HEAD) ||\n-\t\t\tdie \"Bad rev input: HEAD\"\n-\t\tbisect_write \"$state\" \"$rev\" ;;\n+\t\tbisect_parse_rev HEAD\n+\t\tbisect_write \"$state\" \"$sha\" ;;\n \t2,bad|*,good|*,skip)\n \t\tshift\n \t\teval=''\n \t\tfor rev in \"$@\"\n \t\tdo\n-\t\t\tsha=$(git rev-parse --verify \"$rev^{commit}\") ||\n-\t\t\t\tdie \"Bad rev input: $rev\"\n+\t\t\tbisect_parse_rev \"$rev\"\n \t\t\teval=\"$eval bisect_write '$state' '$sha'; \"\n \t\tdone\n \t\teval \"$eval\" ;;\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 5e3e544..dc95117 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -89,6 +89,27 @@ test_expect_success 'bisect fails if given any junk instead of revs' '\n \tgit bisect bad $HASH4\n '\n \n+test_expect_success 'bisect warns if given dubious tags or branches' '\n+\tgit branch bad $HASH4 &&\n+\tgit bisect start bad -- 2>start.output &&\n+\tgrep \"bad\" start.output &&\n+\tgrep \"may be a syntax error\" start.output &&\n+\tgit tag good $HASH1 &&\n+\tgit bisect start $HASH4 $HASH2 good -- 2>start.output &&\n+\tgrep \"good\" start.output &&\n+\tgrep \"may be a syntax error\" start.output &&\n+\tgit bisect start &&\n+\tgit bisect good $HASH2 bad 2>good.output &&\n+\tgrep \"bad\" good.output &&\n+\tgrep \"may be a syntax error\" good.output &&\n+\tgit bisect start &&\n+\tgit bisect bad bad 2>bad.output &&\n+\tgrep \"bad\" bad.output &&\n+\tgrep \"may be a syntax error\" bad.output &&\n+\tgit branch -D bad &&\n+\tgit tag -d good\n+'\n+\n test_expect_success 'bisect reset: back in the master branch' '\n \tgit bisect reset &&\n \techo \"* master\" > branch.expect &&\n-- \n1.5.5.50.ge6e82.dirty\n"},{"id":"74388","messageId":"7vabjw810v.fsf@gitster.siamese.dyndns.org","threadId":"13103","inReplyTo":"20080414054151.52e55e7f.chriscool@tuxfamily.org","subject":"Re: [PATCH 2/2] bisect: warn if given dubious tag or branch as rev","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-04-14T23:33:36Z","receivedAt":"2008-04-14T23:33:36Z","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> This patch refactors rev parsing code in a new \"bisect_parse_rev\"\n> function, and adds warnings in case a rev with a name that is the\n> same as one of the bisect subcommands is given where a revision\n> is expected.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  git-bisect.sh               |   47 ++++++++++++++++++++++++++++++------------\n>  t/t6030-bisect-porcelain.sh |   21 +++++++++++++++++++\n>  2 files changed, 54 insertions(+), 14 deletions(-)\n>\n> diff --git a/git-bisect.sh b/git-bisect.sh\n> index 6b43461..a090b97 100755\n> --- a/git-bisect.sh\n> +++ b/git-bisect.sh\n> @@ -61,6 +61,31 @@ bisect_autostart() {\n>  \t}\n>  }\n>  \n> +bisect_parse_rev() {\n> +\trev=\"$1\"\n> +\tdont_die_on_wrong_rev=\"$2\"\n> +\tname=\"$rev^{commit}\"\n> +\tdubious_rev_name=''\n> +\n> +\tcase \"$rev\" in\n> +\tHEAD)\n> +\t\tname='HEAD' ;;\n> +\thelp|start|bad|good|skip|next|reset|visualize|replay|log|run)\n> +\t\tdubious_rev_name='yes' ;;\n> +\tesac\n\nI see you listed all the subcommand names here but I somehow doubt this is\nsensible.\n\nThe \"bisect good $foo bad $bar\" example from Ingo's transcript may suggest\nthat good/bad may incorrectly appear by copying and pasting from an\nincorrect how-to guide, but on the other hand \"bisect bad log\" to mark the\ntip of the \"log\" branch (or \"replay or any other common words) to see\nwhere in the development trail on that branch things got broken is a\nperfectly valid and plausible thing to do.  Also when you happen to have\nfound something broken while you are working on a more important issue,\nyou may tentatively do \"git tag bad $it\", and keep working on that\nimportant issue, only to come back later and use that lightweight tag to\nfeed \"git bisect\" with.  In either use case, \"you named your ref the same\nas bisect subcommand name, bad boy\" is an unnecessary noise.\n"},{"id":"74419","messageId":"200804150905.15053.chriscool@tuxfamily.org","threadId":"13103","inReplyTo":"7vabjw810v.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/2] bisect: warn if given dubious tag or branch as rev","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-04-15T07:05:14Z","receivedAt":"2008-04-15T07:05:14Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le mardi 15 avril 2008, Junio C Hamano a écrit :\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> > This patch refactors rev parsing code in a new \"bisect_parse_rev\"\n> > function, and adds warnings in case a rev with a name that is the\n> > same as one of the bisect subcommands is given where a revision\n> > is expected.\n> >\n> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> > ---\n> >  git-bisect.sh               |   47\n> > ++++++++++++++++++++++++++++++------------ t/t6030-bisect-porcelain.sh\n> > |   21 +++++++++++++++++++\n> >  2 files changed, 54 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/git-bisect.sh b/git-bisect.sh\n> > index 6b43461..a090b97 100755\n> > --- a/git-bisect.sh\n> > +++ b/git-bisect.sh\n> > @@ -61,6 +61,31 @@ bisect_autostart() {\n> >  \t}\n> >  }\n> >\n> > +bisect_parse_rev() {\n> > +\trev=\"$1\"\n> > +\tdont_die_on_wrong_rev=\"$2\"\n> > +\tname=\"$rev^{commit}\"\n> > +\tdubious_rev_name=''\n> > +\n> > +\tcase \"$rev\" in\n> > +\tHEAD)\n> > +\t\tname='HEAD' ;;\n> > +\thelp|start|bad|good|skip|next|reset|visualize|replay|log|run)\n> > +\t\tdubious_rev_name='yes' ;;\n> > +\tesac\n>\n> I see you listed all the subcommand names here but I somehow doubt this\n> is sensible.\n>\n> The \"bisect good $foo bad $bar\" example from Ingo's transcript may\n> suggest that good/bad may incorrectly appear by copying and pasting from\n> an incorrect how-to guide,\n\nYes, and there are also some \"git bisect start good $foo bad $bar\" in his \ntranscript. So perhaps we should warn only on \"bad\" good\" and maybe \"skip\".\n\n> but on the other hand \"bisect bad log\" to mark \n> the tip of the \"log\" branch (or \"replay or any other common words) to see\n> where in the development trail on that branch things got broken is a\n> perfectly valid and plausible thing to do.  Also when you happen to have\n> found something broken while you are working on a more important issue,\n> you may tentatively do \"git tag bad $it\", and keep working on that\n> important issue, only to come back later and use that lightweight tag to\n> feed \"git bisect\" with.  In either use case, \"you named your ref the same\n> as bisect subcommand name, bad boy\" is an unnecessary noise.\n\nI think Ingo uses git and git bisect very often, and still he made mistakes \nthat could have resulted in unwanted behavior with nothing to help \nunderstand what happened, if he had tags or branches named \"good\" or \"bad\".\n\nSo it's not for \"bad boys\" or forcing good naming, but really preventing \nmistakes.\n\nIn bisect_autostart we also have:\n\n\t\techo >&2 'You need to start by \"git bisect start\"'\n\t\tif test -t 0\n\t\tthen\n\t\t\techo >&2 -n 'Do you want me to do it for you [Y/n]? '\n\t\t\tread yesno\n\t\t\tcase \"$yesno\" in\n\t\t\t[Nn]*)\n\t\t\t\texit ;;\n\t\t\tesac\n\t\t\tbisect_start\n\t\telse\n\t\t\texit 1\n\t\tfi\n\nto make sure that the users know what happens, and help them do what they \nwant. If you prefer I can add something like the above to make sure that \nthe user knows what he is doing.\n\nThanks,\nChristian.\n"}]}