{"thread":{"id":"39315","subject":"[PATCH v2] bisect: improve output when bad commit is found","startedAt":"2015-05-12T23:19:11Z","lastAt":"2015-05-13T21:42:29Z","messageCount":10,"participants":["Trevor Saunders","Junio C Hamano","Jeff King","Christian Couder"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"261099","messageId":"1431472751-20974-1-git-send-email-tbsaunde@tbsaunde.org","threadId":"39315","inReplyTo":null,"subject":"[PATCH v2] bisect: improve output when bad commit is found","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-12T23:19:11Z","receivedAt":"2015-05-12T23:19:11Z","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\t<40 char sha1> is the first bad commit\n\tCommit <40 char sha1>\n\t...\n\n\t:100644 100644 10f5e57... a46cfeb... M bisect.c\n\t:100755 100755 ae3fec2... 65a19fa... M git-bisect.sh\n\nThe raw diff output is not really useful, and its kind of silly to print\nthe sha1 twice. Instead lets print something like this:\n\n\tThe first bad commit is\n\tCommit \n\t...\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\nSigned-off-by: Trevor Saunders \n---\n bisect.c | 9 +++------\n git-bisect.sh | 2 +-\n t/t6030-bisect-porcelain.sh | 36 +++++++++++++++++++++++++-----------\n 3 files changed, 29 insertions(+), 18 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 10f5e57..a46cfeb 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..65a19fa 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..43f420d 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -26,6 +26,20 @@ add_line_into_file()\n git commit --quiet -m \"$MSG\" $_file\n }\n \n+check_bisect_msg () {\n+\tfile=$1\n+\thash=$2\n+\techo \"The first bad commit is\" > expect &&\n+\tgit show -s --no-abbrev $hash >> expect &&\n+\tif test -n \"$3\"\n+ \tthen\n+\t\techo $3 >> expect || return $?\n+\tfi\n+\tcnt=$(wc -l actual &&\n+\ttest_cmp expect actual\n+}\n+\n HASH1=\n HASH2=\n HASH3=\n@@ -189,7 +203,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 +268,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 \"bisect run success\" &&\n git bisect reset'\n \n # We want to automatically find the commit that\n@@ -267,7 +281,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 \"bisect run success\" &&\n git bisect reset'\n \n # $HASH1 is good, $HASH5 is bad, we skip $HASH3\n@@ -280,14 +294,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 +342,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 \"bisect run success\"\n '\n \n test_expect_success 'bisect skip only one range' '\n@@ -378,7 +392,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 +541,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 +559,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 +590,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 \"bisect run success\"\n '\n \n \n@@ -591,7 +605,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 \"bisect run success\"\n '\n \n #\n-- \n2.4.0.78.g7c6ecbf\n"},{"id":"261100","messageId":"xmqqwq0dcrcf.fsf@gitster.dls.corp.google.com","threadId":"39315","inReplyTo":"1431472751-20974-1-git-send-email-tbsaunde@tbsaunde.org","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-12T23:24:00Z","receivedAt":"2015-05-12T23:24:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Trevor Saunders writes:\n\n> When the first bad commit has been found git bisect prints something\n> like this:\n>\n> \t<40 char sha1> is the first bad commit\n> \tCommit <40 char sha1>\n> \t...\n>\n> \t:100644 100644 10f5e57... a46cfeb... M bisect.c\n> \t:100755 100755 ae3fec2... 65a19fa... M git-bisect.sh\n>\n> The raw diff output is not really useful, and its kind of silly to print\n> the sha1 twice. Instead lets print something like this:\n>\n> \tThe first bad commit is\n> \tCommit \n> \t...\n\nAccording to +CCouder, this change will break existing people's use\ncases.\n\nSee $gmane/268881\n"},{"id":"261104","messageId":"20150513005432.GF31257@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"39315","inReplyTo":"xmqqwq0dcrcf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-13T00:54:32Z","receivedAt":"2015-05-13T00:54:32Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:\n> Trevor Saunders writes:\n> \n> > When the first bad commit has been found git bisect prints something\n> > like this:\n> >\n> > \t<40 char sha1> is the first bad commit\n> > \tCommit <40 char sha1>\n> > \t...\n> >\n> > \t:100644 100644 10f5e57... a46cfeb... M bisect.c\n> > \t:100755 100755 ae3fec2... 65a19fa... M git-bisect.sh\n> >\n> > The raw diff output is not really useful, and its kind of silly to print\n> > the sha1 twice. Instead lets print something like this:\n> >\n> > \tThe first bad commit is\n> > \tCommit \n> > \t...\n> \n> According to +CCouder, this change will break existing people's use\n> cases.\n> \n> See $gmane/268881\n\nWell, technically he just said it might be that people are parsing the\noutput and could be broken, but if you'd rather not take that risk then\nI guess we just have to leave things the way they are.\n\nTrev\n"},{"id":"261105","messageId":"xmqqoalpcm9f.fsf@gitster.dls.corp.google.com","threadId":"39315","inReplyTo":"20150513005432.GF31257@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-13T01:13:48Z","receivedAt":"2015-05-13T01:13:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Trevor Saunders writes:\n\n> On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:\n>> Trevor Saunders writes:\n>> \n>> > When the first bad commit has been found git bisect prints something\n>> > like this:\n>> >\n>> > \t<40 char sha1> is the first bad commit\n>> > \tCommit <40 char sha1>\n>> > \t...\n>> >\n>> > \t:100644 100644 10f5e57... a46cfeb... M bisect.c\n>> > \t:100755 100755 ae3fec2... 65a19fa... M git-bisect.sh\n>> >\n>> > The raw diff output is not really useful, and its kind of silly to print\n>> > the sha1 twice. Instead lets print something like this:\n>> >\n>> > \tThe first bad commit is\n>> > \tCommit \n>> > \t...\n>> \n>> According to +CCouder, this change will break existing people's use\n>> cases.\n>> \n>> See $gmane/268881\n>\n> Well, technically he just said it might be that people are parsing the\n> output and could be broken, but if you'd rather not take that risk then\n> I guess we just have to leave things the way they are.\n\nFWIW.\n\n - I personally do not agree that those who scripted around \"git\n bisect\" (as opposed to those who wrote scripts to be driven by\n the \"bisect run\" interface) are worth worrying about. But I am\n not the whole of the Git world ;-)\n\n - I personally do not find two same 40-hex on two lines is silly at\n all.\n\n - I _do_ think diff-tree --raw output without recursive is silly.\n It is not useful for humans (it doesn't even give paths fully),\n and it is insufficient for scripts, which can grok more through\n information out of the 40-hex.\n\nSo perhaps if we keep\n\n\t<40 char sha1> is the first bad commit\n\nand then replace the diff-tree output with \"show -s\", then the\nresult would be good enough, I would say.\n"},{"id":"261107","messageId":"20150513013637.GB3066@peff.net","threadId":"39315","inReplyTo":"xmqqoalpcm9f.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-13T01:36:37Z","receivedAt":"2015-05-13T01:36:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 12, 2015 at 06:13:48PM -0700, Junio C Hamano wrote:\n\n> >> See $gmane/268881\n> >\n> > Well, technically he just said it might be that people are parsing the\n> > output and could be broken, but if you'd rather not take that risk then\n> > I guess we just have to leave things the way they are.\n> \n> FWIW.\n> \n> - I personally do not agree that those who scripted around \"git\n> bisect\" (as opposed to those who wrote scripts to be driven by\n> the \"bisect run\" interface) are worth worrying about. But I am\n> not the whole of the Git world ;-)\n\nIt is not clear to me that people are actually scripting around the\noutput. Between the exit code and the stable output in BISECT_LOG, that\nseems like a much more preferable way for scripted uses to find out what\nhappened.\n\nOf course, that is not a guarantee that nobody scraped stderr, but at\nleast it makes me feel better that they're Doing It Wrong. :)\n\nIt would be nice if we had some actual data points. I followed the link\nChristian gave to Ingo's old post, but I didn't see the actual script\nthere. There is:\n\n https://github.com/grosser/git-autobisect/blob/master/lib/git/autobisect.rb\n\nwhich does seem to scrape stderr. Bleh.\n\n> - I _do_ think diff-tree --raw output without recursive is silly.\n> It is not useful for humans (it doesn't even give paths fully),\n> and it is insufficient for scripts, which can grok more through\n> information out of the 40-hex.\n> \n> So perhaps if we keep\n> \n> \t<40 char sha1> is the first bad commit\n> \n> and then replace the diff-tree output with \"show -s\", then the\n> result would be good enough, I would say.\n\nThat seems like a reasonable first step, at the very least. I wonder if\nwe should also better document the exit code and BISECT_LOG semantics,\nand explicitly tell people not to scrape stderr. That at least frees us\nup later to change the output as we see fit (I notice that most of the\nmessages in the script are already i18n'd; the ones from bisect--helper\nare the odd ones out).\n\n-Peff\n"},{"id":"261133","messageId":"xmqqvbfxauyz.fsf@gitster.dls.corp.google.com","threadId":"39315","inReplyTo":"20150513013637.GB3066@peff.net","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-13T05:48:36Z","receivedAt":"2015-05-13T05:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Of course, that is not a guarantee that nobody scraped stderr, but at\n> least it makes me feel better that they're Doing It Wrong. :)\n>\n> It would be nice if we had some actual data points. I followed the link\n> Christian gave to Ingo's old post, but I didn't see the actual script\n> there. There is:\n>\n> https://github.com/grosser/git-autobisect/blob/master/lib/git/autobisect.rb\n>\n> which does seem to scrape stderr. Bleh.\n\nHeh, anything ending with .rb is a bleh to me ;-)\n\n>> So perhaps if we keep\n>> \n>> \t<40 char sha1> is the first bad commit\n>> \n>> and then replace the diff-tree output with \"show -s\", then the\n>> result would be good enough, I would say.\n>\n> That seems like a reasonable first step, at the very least. I wonder if\n> we should also better document the exit code and BISECT_LOG semantics,\n> and explicitly tell people not to scrape stderr.\n\nYeah, that would also be a good first step, whichever comes first.\n"},{"id":"261155","messageId":"CAP8UFD0VsJG6G=sP9mtkwwG30mZeiGwQLs_9d48Ovx=ws15YuA@mail.gmail.com","threadId":"39315","inReplyTo":"xmqqoalpcm9f.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-13T09:10:31Z","receivedAt":"2015-05-13T09:10:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, May 13, 2015 at 3:13 AM, Junio C Hamano wrote:\n> Trevor Saunders writes:\n>\n>> On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:\n>>> Trevor Saunders writes:\n>>>\n>>> > When the first bad commit has been found git bisect prints something\n>>> > like this:\n>>> >\n>>> > <40 char sha1> is the first bad commit\n>>> > Commit <40 char sha1>\n>>> > ...\n>>> >\n>>> > :100644 100644 10f5e57... a46cfeb... M bisect.c\n>>> > :100755 100755 ae3fec2... 65a19fa... M git-bisect.sh\n>>> >\n>>> > The raw diff output is not really useful, and its kind of silly to print\n>>> > the sha1 twice. Instead lets print something like this:\n>>> >\n>>> > The first bad commit is\n>>> > Commit \n>>> > ...\n>>>\n>>> According to +CCouder, this change will break existing people's use\n>>> cases.\n>>>\n>>> See $gmane/268881\n>>\n>> Well, technically he just said it might be that people are parsing the\n>> output and could be broken, but if you'd rather not take that risk then\n>> I guess we just have to leave things the way they are.\n>\n> FWIW.\n>\n> - I personally do not agree that those who scripted around \"git\n> bisect\" (as opposed to those who wrote scripts to be driven by\n> the \"bisect run\" interface) are worth worrying about. But I am\n> not the whole of the Git world ;-)\n\nYou know in git-bisect.sh:bisect_run() we do:\n\n if sane_grep \"is the first bad commit\"\n\"$GIT_DIR/BISECT_RUN\" >/dev/null\n then\n gettextln \"bisect run success\"\n exit 0;\n fi\n\nso we are doing it too!\n\n> - I personally do not find two same 40-hex on two lines is silly at\n> all.\n\nI agree.\n\n> - I _do_ think diff-tree --raw output without recursive is silly.\n> It is not useful for humans (it doesn't even give paths fully),\n> and it is insufficient for scripts, which can grok more through\n> information out of the 40-hex.\n>\n> So perhaps if we keep\n>\n> <40 char sha1> is the first bad commit\n>\n> and then replace the diff-tree output with \"show -s\", then the\n> result would be good enough, I would say.\n\nYeah I agree.\n\nAnd for people who want something else we can implement config options.\n\nFor example a bisect.outputformat that could be used like in the\nfollowing pseudo shell code:\n\nformat=$(git config bisect.outputformat)\n\ntest -z \"$format\" && format=\"medium\"\n\ngit show -s --format=\"$format\" \"$firstbadcommit\"\n"},{"id":"261162","messageId":"CAP8UFD14LuyaVcoNHxR00ZeGgHWXbuW_QdVf4YqZWzgMdufUjQ@mail.gmail.com","threadId":"39315","inReplyTo":"20150513013637.GB3066@peff.net","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-13T09:39:41Z","receivedAt":"2015-05-13T09:39:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, May 13, 2015 at 3:36 AM, Jeff King wrote:\n> On Tue, May 12, 2015 at 06:13:48PM -0700, Junio C Hamano wrote:\n>\n>> >> See $gmane/268881\n>> >\n>> > Well, technically he just said it might be that people are parsing the\n>> > output and could be broken, but if you'd rather not take that risk then\n>> > I guess we just have to leave things the way they are.\n>>\n>> FWIW.\n>>\n>> - I personally do not agree that those who scripted around \"git\n>> bisect\" (as opposed to those who wrote scripts to be driven by\n>> the \"bisect run\" interface) are worth worrying about. But I am\n>> not the whole of the Git world ;-)\n>\n> It is not clear to me that people are actually scripting around the\n> output. Between the exit code and the stable output in BISECT_LOG, that\n> seems like a much more preferable way for scripted uses to find out what\n> happened.\n>\n> Of course, that is not a guarantee that nobody scraped stderr, but at\n> least it makes me feel better that they're Doing It Wrong. :)\n\nAren't we sending the \"XXXX is the first bad commit\" and the diff-tree\nto stdout?\n"},{"id":"261189","messageId":"20150513172512.GA27231@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"39315","inReplyTo":"CAP8UFD0VsJG6G=sP9mtkwwG30mZeiGwQLs_9d48Ovx=ws15YuA@mail.gmail.com","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-13T17:25:12Z","receivedAt":"2015-05-13T17:25:12Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Wed, May 13, 2015 at 11:10:31AM +0200, Christian Couder wrote:\n> On Wed, May 13, 2015 at 3:13 AM, Junio C Hamano wrote:\n> > Trevor Saunders writes:\n> >\n> >> On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:\n> >>> Trevor Saunders writes:\n> >>>\n> >>> > When the first bad commit has been found git bisect prints something\n> >>> > like this:\n> >>> >\n> >>> > <40 char sha1> is the first bad commit\n> >>> > Commit <40 char sha1>\n> >>> > ...\n> >>> >\n> >>> > :100644 100644 10f5e57... a46cfeb... M bisect.c\n> >>> > :100755 100755 ae3fec2... 65a19fa... M git-bisect.sh\n> >>> >\n> >>> > The raw diff output is not really useful, and its kind of silly to print\n> >>> > the sha1 twice. Instead lets print something like this:\n> >>> >\n> >>> > The first bad commit is\n> >>> > Commit \n> >>> > ...\n> >>>\n> >>> According to +CCouder, this change will break existing people's use\n> >>> cases.\n> >>>\n> >>> See $gmane/268881\n> >>\n> >> Well, technically he just said it might be that people are parsing the\n> >> output and could be broken, but if you'd rather not take that risk then\n> >> I guess we just have to leave things the way they are.\n> >\n> > FWIW.\n> >\n> > - I personally do not agree that those who scripted around \"git\n> > bisect\" (as opposed to those who wrote scripts to be driven by\n> > the \"bisect run\" interface) are worth worrying about. But I am\n> > not the whole of the Git world ;-)\n> \n> You know in git-bisect.sh:bisect_run() we do:\n> \n> if sane_grep \"is the first bad commit\"\n> \"$GIT_DIR/BISECT_RUN\" >/dev/null\n> then\n> gettextln \"bisect run success\"\n> exit 0;\n> fi\n> \n> so we are doing it too!\n\nI saw that, and thought it was pretty gross.\n\n> > - I personally do not find two same 40-hex on two lines is silly at\n> > all.\n> \n> I agree.\n\n*shrug* I'd probably agree more if it came last after the more useful\ninformation the first bad commit has been found.\n\n> > - I _do_ think diff-tree --raw output without recursive is silly.\n> > It is not useful for humans (it doesn't even give paths fully),\n> > and it is insufficient for scripts, which can grok more through\n> > information out of the 40-hex.\n> >\n> > So perhaps if we keep\n> >\n> > <40 char sha1> is the first bad commit\n> >\n> > and then replace the diff-tree output with \"show -s\", then the\n> > result would be good enough, I would say.\n> \n> Yeah I agree.\n\nsounds like we all agree about that part at least :)\n\n> And for people who want something else we can implement config options.\n> \n> For example a bisect.outputformat that could be used like in the\n\nI'd been thinking of a config option that was a little more user\nfriendly than writing \"The first bad commit is%nCommit %h%n...\" but\ndoing it that way is both simple and might allow us to replace the\nprinting code in bisect.c with a default format string.\n\nTrev\n"},{"id":"261205","messageId":"20150513214228.GA17350@peff.net","threadId":"39315","inReplyTo":"CAP8UFD14LuyaVcoNHxR00ZeGgHWXbuW_QdVf4YqZWzgMdufUjQ@mail.gmail.com","subject":"Re: [PATCH v2] bisect: improve output when bad commit is found","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-13T21:42:29Z","receivedAt":"2015-05-13T21:42:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 13, 2015 at 11:39:41AM +0200, Christian Couder wrote:\n\n> > It is not clear to me that people are actually scripting around the\n> > output. Between the exit code and the stable output in BISECT_LOG, that\n> > seems like a much more preferable way for scripted uses to find out what\n> > happened.\n> >\n> > Of course, that is not a guarantee that nobody scraped stderr, but at\n> > least it makes me feel better that they're Doing It Wrong. :)\n> \n> Aren't we sending the \"XXXX is the first bad commit\" and the diff-tree\n> to stdout?\n\nGood point. I'm much more sympathetic to people scraping stdout than\nstderr. I do still think we would do better to direct them to more\nrobust formats, though.\n\n-Peff\n"}]}