{"thread":{"id":"39302","subject":"[PATCH] bisect: improve output when bad commit is found","startedAt":"2015-05-11T20:58:59Z","lastAt":"2015-05-12T02:10:24Z","messageCount":5,"participants":["Trevor Saunders","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"261018","messageId":"1431377939-13887-1-git-send-email-tbsaunde@tbsaunde.org","threadId":"39302","inReplyTo":null,"subject":"[PATCH] bisect: improve output when bad commit is found","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-11T20:58:59Z","receivedAt":"2015-05-11T20:58:59Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"When the first bad commit has been found git bisect prints something\nlike this\n\n<40 char sha1> is the first bad commit\nCommit <40 char sha1>\n...\n\n<raw diff output>\n\nThe raw diff output is not really useful, and its kind of silly to print\nthe sha1 twice.  Instead lets print something like\n\nthe first bad commit is\nCommit <sha1>\n...\n\nThis also fixes an odd inconsistancy where if the first bad commit is a\ntrivial merge git bisect will only print the first line.\n---\n bisect.c                    |  9 +++------\n git-bisect.sh               |  2 +-\n t/t6030-bisect-porcelain.sh | 30 +++++++++++++++++++-----------\n 3 files changed, 23 insertions(+), 18 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 10f5e57..a0ebb7f 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -875,17 +875,14 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n \tinit_revisions(&opt, prefix);\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \topt.abbrev = 0;\n-\topt.diff = 1;\n+\topt.diff = 0;\n+\topt.always_show_header = 1;\n \n \t/* This is what \"--pretty\" does */\n \topt.verbose_header = 1;\n \topt.use_terminator = 0;\n \topt.commit_format = CMIT_FMT_DEFAULT;\n \n-\t/* diff-tree init */\n-\tif (!opt.diffopt.output_format)\n-\t\topt.diffopt.output_format = DIFF_FORMAT_RAW;\n-\n \tlog_tree_commit(&opt, commit);\n }\n \n@@ -942,7 +939,7 @@ int bisect_next_all(const char *prefix, int no_checkout)\n \n \tif (!hashcmp(bisect_rev, current_bad_oid->hash)) {\n \t\texit_if_skipped_commits(tried, current_bad_oid);\n-\t\tprintf(\"%s is the first bad commit\\n\", bisect_rev_hex);\n+\t\tputs(\"the first bad commit is\");\n \t\tshow_diff_tree(prefix, revs.commits->item);\n \t\t/* This means the bisection process succeeded. */\n \t\texit(10);\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex ae3fec2..cb4bd2f 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -480,7 +480,7 @@ exit code \\$res from '\\$command' is < 0 or >= 128\" >&2\n \t\t\texit $res\n \t\tfi\n \n-\t\tif sane_grep \"is the first bad commit\" \"$GIT_DIR/BISECT_RUN\" >/dev/null\n+\t\tif sane_grep \"the first bad commit is\" \"$GIT_DIR/BISECT_RUN\" >/dev/null\n \t\tthen\n \t\t\tgettextln \"bisect run success\"\n \t\t\texit 0;\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 06b4868..bf50d20 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -26,6 +26,14 @@ add_line_into_file()\n     git commit --quiet -m \"$MSG\" $_file\n }\n \n+check_bisect_msg()\n+{\n+\tfile=$1\n+\thash=$2\n+\tgrep \"the first bad commit is\" $file || return $?\n+\tgrep $hash $file || return $?\n+}\n+\n HASH1=\n HASH2=\n HASH3=\n@@ -189,7 +197,7 @@ test_expect_success 'bisect skip: successful result' '\n \tgit bisect start $HASH4 $HASH1 &&\n \tgit bisect skip &&\n \tgit bisect bad > my_bisect_log.txt &&\n-\tgrep \"$HASH2 is the first bad commit\" my_bisect_log.txt\n+\tcheck_bisect_msg my_bisect_log.txt $HASH2\n '\n \n # $HASH1 is good, $HASH4 is bad, we skip $HASH3 and $HASH2\n@@ -254,7 +262,7 @@ test_expect_success \\\n      git bisect good $HASH1 &&\n      git bisect bad $HASH4 &&\n      git bisect run ./test_script.sh > my_bisect_log.txt &&\n-     grep \"$HASH3 is the first bad commit\" my_bisect_log.txt &&\n+     check_bisect_msg my_bisect_log.txt $HASH3 &&\n      git bisect reset'\n \n # We want to automatically find the commit that\n@@ -267,7 +275,7 @@ test_expect_success \\\n      chmod +x test_script.sh &&\n      git bisect start $HASH4 $HASH1 &&\n      git bisect run ./test_script.sh > my_bisect_log.txt &&\n-     grep \"$HASH4 is the first bad commit\" my_bisect_log.txt &&\n+     check_bisect_msg my_bisect_log.txt $HASH4 &&\n      git bisect reset'\n \n # $HASH1 is good, $HASH5 is bad, we skip $HASH3\n@@ -280,14 +288,14 @@ test_expect_success 'bisect skip: add line and then a new test' '\n \tgit bisect start $HASH5 $HASH1 &&\n \tgit bisect skip &&\n \tgit bisect good > my_bisect_log.txt &&\n-\tgrep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n+\tcheck_bisect_msg my_bisect_log.txt $HASH5 &&\n \tgit bisect log > log_to_replay.txt &&\n \tgit bisect reset\n '\n \n test_expect_success 'bisect skip and bisect replay' '\n \tgit bisect replay log_to_replay.txt > my_bisect_log.txt &&\n-\tgrep \"$HASH5 is the first bad commit\" my_bisect_log.txt &&\n+\tcheck_bisect_msg my_bisect_log.txt $HASH5 &&\n \tgit bisect reset\n '\n \n@@ -328,7 +336,7 @@ test_expect_success 'bisect run & skip: find first bad' '\n \tchmod +x test_script.sh &&\n \tgit bisect start $HASH7 $HASH1 &&\n \tgit bisect run ./test_script.sh > my_bisect_log.txt &&\n-\tgrep \"$HASH6 is the first bad commit\" my_bisect_log.txt\n+\tcheck_bisect_msg my_bisect_log.txt $HASH6\n '\n \n test_expect_success 'bisect skip only one range' '\n@@ -378,7 +386,7 @@ test_expect_success 'bisect does not create a \"bisect\" branch' '\n \trev_hash6=$(git rev-parse --verify HEAD) &&\n \ttest \"$rev_hash6\" = \"$HASH6\" &&\n \tgit bisect good > my_bisect_log.txt &&\n-\tgrep \"$HASH7 is the first bad commit\" my_bisect_log.txt &&\n+\tcheck_bisect_msg my_bisect_log.txt $HASH7 &&\n \tgit bisect reset &&\n \trev_hash6=$(git rev-parse --verify bisect) &&\n \ttest \"$rev_hash6\" = \"$HASH6\" &&\n@@ -527,7 +535,7 @@ test_expect_success 'restricting bisection on one dir' '\n \tpara1=$(git rev-parse --verify HEAD) &&\n \ttest \"$para1\" = \"$PARA_HASH1\" &&\n \tgit bisect bad > my_bisect_log.txt &&\n-\tgrep \"$PARA_HASH1 is the first bad commit\" my_bisect_log.txt\n+\tcheck_bisect_msg my_bisect_log.txt $PARA_HASH1\n '\n \n test_expect_success 'restricting bisection on one dir and a file' '\n@@ -545,7 +553,7 @@ test_expect_success 'restricting bisection on one dir and a file' '\n \tpara1=$(git rev-parse --verify HEAD) &&\n \ttest \"$para1\" = \"$PARA_HASH1\" &&\n \tgit bisect good > my_bisect_log.txt &&\n-\tgrep \"$PARA_HASH4 is the first bad commit\" my_bisect_log.txt\n+\tcheck_bisect_msg my_bisect_log.txt $PARA_HASH4\n '\n \n test_expect_success 'skipping away from skipped commit' '\n@@ -576,7 +584,7 @@ test_expect_success 'test bisection on bare repo - --no-checkout specified' '\n \t\t\t\"test \\$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0\" \\\n \t\t\t>../nocheckout.log\n \t) &&\n-\tgrep \"$HASH3 is the first bad commit\" nocheckout.log\n+\tcheck_bisect_msg  nocheckout.log $HASH3\n '\n \n \n@@ -591,7 +599,7 @@ test_expect_success 'test bisection on bare repo - --no-checkout defaulted' '\n \t\t\t\"test \\$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0\" \\\n \t\t\t>../defaulted.log\n \t) &&\n-\tgrep \"$HASH3 is the first bad commit\" defaulted.log\n+\tcheck_bisect_msg defaulted.log $HASH3\n '\n \n #\n-- \n2.4.0.78.g7c6ecbf.dirty\n"},{"id":"261020","messageId":"xmqq4mni3jjg.fsf@gitster.dls.corp.google.com","threadId":"39302","inReplyTo":"1431377939-13887-1-git-send-email-tbsaunde@tbsaunde.org","subject":"Re: [PATCH] bisect: improve output when bad commit is found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-11T21:12:51Z","receivedAt":"2015-05-11T21:12:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Trevor Saunders <tbsaunde@tbsaunde.org> writes:\n\n> When the first bad commit has been found git bisect prints something\n> like this\n\n>\n> <40 char sha1> is the first bad commit\n> Commit <40 char sha1>\n> ...\n>\n> <raw diff output>\n>\n> The raw diff output is not really useful, and its kind of silly to print\n\nEnd \"something like this\" with a colon, and indent the example\ndisplay, i.e.\n\n        ... prints something like this:\n\n            <40-hex object name> is the first bad commit\n            Commit <40-hex object name>\n\n        The raw diff output is ...\n\n> the sha1 twice.  Instead lets print something like\n>\n> the first bad commit is\n> Commit <sha1>\n> ...\n\nLikewise.\n\n> This also fixes an odd inconsistancy where if the first bad commit is a\n> trivial merge git bisect will only print the first line.\n> ---\n\nSign-off?\n\n> -\t\tprintf(\"%s is the first bad commit\\n\", bisect_rev_hex);\n> +\t\tputs(\"the first bad commit is\");\n\ns/the/The/, I would think.\n\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index 06b4868..bf50d20 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -26,6 +26,14 @@ add_line_into_file()\n>      git commit --quiet -m \"$MSG\" $_file\n>  }\n>  \n> +check_bisect_msg()\n> +{\n\nFind this paragraph in Documentation/CodingGuidelines:\n\n - We prefer a space between the function name and the parentheses,\n   and no space inside the parentheses. The opening \"{\" should also\n   be on the same line.\n\n> +\tfile=$1\n> +\thash=$2\n> +\tgrep \"the first bad commit is\" $file || return $?\n> +\tgrep $hash $file || return $?\n\nIs it OK to have these strings anywhere in the $file?\n\nThanks.\n"},{"id":"261029","messageId":"20150511231125.GC18112@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"39302","inReplyTo":"xmqq4mni3jjg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: improve output when bad commit is found","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-11T23:11:25Z","receivedAt":"2015-05-11T23:11:25Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Mon, May 11, 2015 at 02:12:51PM -0700, Junio C Hamano wrote:\n> Trevor Saunders <tbsaunde@tbsaunde.org> writes:\n> > This also fixes an odd inconsistancy where if the first bad commit is a\n> > trivial merge git bisect will only print the first line.\n> > ---\n> \n> Sign-off?\n\noops, forgot\n\n> > -\t\tprintf(\"%s is the first bad commit\\n\", bisect_rev_hex);\n> > +\t\tputs(\"the first bad commit is\");\n> \n> s/the/The/, I would think.\n\nyup\n\n> > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> > index 06b4868..bf50d20 100755\n> > --- a/t/t6030-bisect-porcelain.sh\n> > +++ b/t/t6030-bisect-porcelain.sh\n> > @@ -26,6 +26,14 @@ add_line_into_file()\n> >      git commit --quiet -m \"$MSG\" $_file\n> >  }\n> >  \n> > +check_bisect_msg()\n> > +{\n> \n> Find this paragraph in Documentation/CodingGuidelines:\n> \n>  - We prefer a space between the function name and the parentheses,\n>    and no space inside the parentheses. The opening \"{\" should also\n>    be on the same line.\n\nyeah, I did it that way to be consistant with the near by\nfunction add_lineinto_file, but I can change if that's prefered.\n\n> > +\tfile=$1\n> > +\thash=$2\n> > +\tgrep \"the first bad commit is\" $file || return $?\n> > +\tgrep $hash $file || return $?\n> \n> Is it OK to have these strings anywhere in the $file?\n\nIts not great, but the test seems to log multiple invokations of git\nbisect into the same file, so there may be text about previous runs\nbefore we are told which commit is bad.\n"},{"id":"261032","messageId":"xmqqbnhq1r9t.fsf@gitster.dls.corp.google.com","threadId":"39302","inReplyTo":"20150511231125.GC18112@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH] bisect: improve output when bad commit is found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-12T02:08:46Z","receivedAt":"2015-05-12T02:08:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Trevor Saunders <tbsaunde@tbsaunde.org> writes:\n\n>> > +\tfile=$1\n>> > +\thash=$2\n>> > +\tgrep \"the first bad commit is\" $file || return $?\n>> > +\tgrep $hash $file || return $?\n>> \n>> Is it OK to have these strings anywhere in the $file?\n>\n> Its not great, but the test seems to log multiple invokations of git\n> bisect into the same file, so there may be text about previous runs\n> before we are told which commit is bad.\n\nSo if we had a previous entry that happens to match $hash, even if\nthe current test stopped and pointed at a different thing, this test\ndeclares a success?\n\nThis function knows how the $file should end, so it might be more\nsensible to craft the expected output and compare the tail end of\nthe $file with it, something like:\n\n\t(\n\t\techo \"The first bad commit is\"\n                git show -s \"$hash\"\n\t) >expect &&\n        cnt=$(wc -l <expect) &&\n        tail -n $cnt \"$file\" >actual &&\n        test_cmp expect actual\n\nperhaps?\n"},{"id":"261033","messageId":"20150512021024.GE18112@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"39302","inReplyTo":"xmqqbnhq1r9t.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: improve output when bad commit is found","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-12T02:10:24Z","receivedAt":"2015-05-12T02:10:24Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Mon, May 11, 2015 at 07:08:46PM -0700, Junio C Hamano wrote:\n> Trevor Saunders <tbsaunde@tbsaunde.org> writes:\n> \n> >> > +\tfile=$1\n> >> > +\thash=$2\n> >> > +\tgrep \"the first bad commit is\" $file || return $?\n> >> > +\tgrep $hash $file || return $?\n> >> \n> >> Is it OK to have these strings anywhere in the $file?\n> >\n> > Its not great, but the test seems to log multiple invokations of git\n> > bisect into the same file, so there may be text about previous runs\n> > before we are told which commit is bad.\n> \n> So if we had a previous entry that happens to match $hash, even if\n> the current test stopped and pointed at a different thing, this test\n> declares a success?\n\nerr yeah, didn't think of that :(\n\n> This function knows how the $file should end, so it might be more\n> sensible to craft the expected output and compare the tail end of\n> the $file with it, something like:\n> \n> \t(\n> \t\techo \"The first bad commit is\"\n>                 git show -s \"$hash\"\n> \t) >expect &&\n>         cnt=$(wc -l <expect) &&\n>         tail -n $cnt \"$file\" >actual &&\n>         test_cmp expect actual\n> \n> perhaps?\n\nseems about right.\n\nThanks!\n\nTrev\n"}]}