{"thread":{"id":"39281","subject":"[PATCH] bisect: print abbrev sha1 for first bad commit","startedAt":"2015-05-08T23:46:03Z","lastAt":"2015-05-13T13:24:40Z","messageCount":17,"participants":["Trevor Saunders","Stefan Beller","Jeff King","Junio C Hamano","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"260882","messageId":"1431128763-28453-1-git-send-email-tbsaunde@tbsaunde.org","threadId":"39281","inReplyTo":null,"subject":"[PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-08T23:46:03Z","receivedAt":"2015-05-08T23:46:03Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"When bisect finds the first bad commit it prints the full commit hash\nfollowed by \" is the first bad commit\".  That's not terribly readable,\nand its rather silly especially considering the next line contains the\nfull hash again.  So change bisect to print the unique abbrev hash and\nthen \"is the first bad commit\".\n\n\n---\n bisect.c                    |  3 ++-\n t/t6030-bisect-porcelain.sh | 28 +++++++++++++++++-----------\n 2 files changed, 19 insertions(+), 12 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 10f5e57..7cdb805 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -942,7 +942,8 @@ 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\tprintf(\"%s is the first bad commit\\n\",\n+\t\t\tfind_unique_abbrev(bisect_rev, DEFAULT_ABBREV));\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/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 06b4868..14232ed 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -26,6 +26,12 @@ add_line_into_file()\n     git commit --quiet -m \"$MSG\" $_file\n }\n \n+short()\n+{\n+\treturn git rev-parse --short $1\n+}\n+\n+\n HASH1=\n HASH2=\n HASH3=\n@@ -189,7 +195,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+\tgrep \"$(short $HASH2) is the first bad commit\" my_bisect_log.txt\n '\n \n # $HASH1 is good, $HASH4 is bad, we skip $HASH3 and $HASH2\n@@ -254,7 +260,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+     grep \"$(short $HASH3) is the first bad commit\" my_bisect_log.txt &&\n      git bisect reset'\n \n # We want to automatically find the commit that\n@@ -267,7 +273,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+     grep \"$(short $HASH4) is the first bad commit\" my_bisect_log.txt &&\n      git bisect reset'\n \n # $HASH1 is good, $HASH5 is bad, we skip $HASH3\n@@ -280,14 +286,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+\tgrep \"$(short $HASH5) is the first bad commit\" my_bisect_log.txt &&\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+\t\tgrep \"$(short $HASH5) is the first bad commit\" my_bisect_log.txt &&\n \tgit bisect reset\n '\n \n@@ -328,7 +334,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+\tgrep \"$(short $HASH6) is the first bad commit\" my_bisect_log.txt\n '\n \n test_expect_success 'bisect skip only one range' '\n@@ -378,7 +384,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+\tgrep \"$(short $HASH7) is the first bad commit\" my_bisect_log.txt &&\n \tgit bisect reset &&\n \trev_hash6=$(git rev-parse --verify bisect) &&\n \ttest \"$rev_hash6\" = \"$HASH6\" &&\n@@ -527,7 +533,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+\tgrep \"$(short $PARA_HASH1) is the first bad commit\" my_bisect_log.txt\n '\n \n test_expect_success 'restricting bisection on one dir and a file' '\n@@ -545,7 +551,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+\tgrep \"$(short $PARA_HASH4) is the first bad commit\" my_bisect_log.txt\n '\n \n test_expect_success 'skipping away from skipped commit' '\n@@ -576,7 +582,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+\t\tgrep \"$(short $HASH3) is the first bad commit\" nocheckout.log\n '\n \n \n@@ -591,7 +597,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+\t\tgrep \"$(short $HASH3) is the first bad commit\" defaulted.log\n '\n \n #\n-- \n2.4.0\n"},{"id":"260884","messageId":"CAGZ79kYjES6DXmvQdmXLAXrKMGrnvQ-vqJuHQU2QxVC4+6M0aA@mail.gmail.com","threadId":"39281","inReplyTo":"1431128763-28453-1-git-send-email-tbsaunde@tbsaunde.org","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-09T00:29:42Z","receivedAt":"2015-05-09T00:29:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 8, 2015 at 4:46 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:\n> its rather silly especially considering the next line contains the\n> full hash again.\n\nMaybe we can omit it altogether then?\n"},{"id":"260886","messageId":"20150509014152.GA31119@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"39281","inReplyTo":"CAGZ79kYjES6DXmvQdmXLAXrKMGrnvQ-vqJuHQU2QxVC4+6M0aA@mail.gmail.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-09T02:03:41Z","receivedAt":"2015-05-09T02:03:41Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Fri, May 08, 2015 at 05:29:42PM -0700, Stefan Beller wrote:\n> On Fri, May 8, 2015 at 4:46 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:\n> > its rather silly especially considering the next line contains the\n> > full hash again.\n> \n> Maybe we can omit it altogether then?\n\nSO we'd print something like\n\nthe first bad commit is\nCommit abcdefabcdefabcdefabcdefabcdefabcdefabcd\nAuthor foo@ba.com\n\nblah blah blah\n\n? That seems reasonable to me.  If we're going that far does it also\nmake sense to drop printingthe lines about which trees have changed and\njust print the commit message / author / hash?\n\nTrev\n"},{"id":"260892","messageId":"20150509040704.GA31428@peff.net","threadId":"39281","inReplyTo":"20150509014152.GA31119@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-09T04:07:04Z","receivedAt":"2015-05-09T04:07:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 08, 2015 at 10:03:41PM -0400, Trevor Saunders wrote:\n\n> On Fri, May 08, 2015 at 05:29:42PM -0700, Stefan Beller wrote:\n> > On Fri, May 8, 2015 at 4:46 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:\n> > > its rather silly especially considering the next line contains the\n> > > full hash again.\n> > \n> > Maybe we can omit it altogether then?\n> \n> SO we'd print something like\n> \n> the first bad commit is\n> Commit abcdefabcdefabcdefabcdefabcdefabcdefabcd\n> Author foo@ba.com\n> \n> blah blah blah\n> \n> ? That seems reasonable to me.  If we're going that far does it also\n> make sense to drop printingthe lines about which trees have changed and\n> just print the commit message / author / hash?\n\nYeah, I have always found bisect's output somewhat silly. It prints the\n\"--raw\" diff output, which is not incredibly useful. And then to top it\noff, it does not feed the \"--recursive\" switch to the diff, so you don't\neven get to see the real list of changed files.\n\nI suspect the most minimal we could go is:\n\n  git log --format='The first bad commit is %h %s' $bad\n\nand then let the user inspect further from there using the hash. But I\nthink it would also be reasonable to just do a straight \"git log -1\n$bad\" with no with no diff.\n\n(Actually, it looks like all this is generated in bisect.c:show_diff_tree,\nso it would have to be written in C; but it should be pretty easy to\ntweak the display options).\n\n-Peff\n"},{"id":"260947","messageId":"20150510231110.GA25157@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"39281","inReplyTo":"20150509040704.GA31428@peff.net","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-10T23:12:45Z","receivedAt":"2015-05-10T23:12:45Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Sat, May 09, 2015 at 12:07:04AM -0400, Jeff King wrote:\n> On Fri, May 08, 2015 at 10:03:41PM -0400, Trevor Saunders wrote:\n> \n> > On Fri, May 08, 2015 at 05:29:42PM -0700, Stefan Beller wrote:\n> > > On Fri, May 8, 2015 at 4:46 PM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:\n> > > > its rather silly especially considering the next line contains the\n> > > > full hash again.\n> > > \n> > > Maybe we can omit it altogether then?\n> > \n> > SO we'd print something like\n> > \n> > the first bad commit is\n> > Commit abcdefabcdefabcdefabcdefabcdefabcdefabcd\n> > Author foo@ba.com\n> > \n> > blah blah blah\n> > \n> > ? That seems reasonable to me.  If we're going that far does it also\n> > make sense to drop printingthe lines about which trees have changed and\n> > just print the commit message / author / hash?\n> \n> Yeah, I have always found bisect's output somewhat silly. It prints the\n> \"--raw\" diff output, which is not incredibly useful. And then to top it\n> off, it does not feed the \"--recursive\" switch to the diff, so you don't\n> even get to see the real list of changed files.\n\n So, fun fact it doesn't actually always print the raw diffoutput if\n there is no diff, for example a merge where both sides only touched\n different files as in test 40 in t6030.\n\n> (Actually, it looks like all this is generated in bisect.c:show_diff_tree,\n> so it would have to be written in C; but it should be pretty easy to\n> tweak the display options).\n\nyeah, that seems pretty straight forward, but I'm not really sure what\nto do about this case where no diff is printed, I guess I should figure\nout what bits need to be set for the commit to be shown anyway.\n\nTrev\n"},{"id":"260948","messageId":"20150511011009.GA21830@peff.net","threadId":"39281","inReplyTo":"20150510231110.GA25157@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-11T01:10:09Z","receivedAt":"2015-05-11T01:10:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 10, 2015 at 07:12:45PM -0400, Trevor Saunders wrote:\n\n> > Yeah, I have always found bisect's output somewhat silly. It prints the\n> > \"--raw\" diff output, which is not incredibly useful. And then to top it\n> > off, it does not feed the \"--recursive\" switch to the diff, so you don't\n> > even get to see the real list of changed files.\n> \n>  So, fun fact it doesn't actually always print the raw diffoutput if\n>  there is no diff, for example a merge where both sides only touched\n>  different files as in test 40 in t6030.\n\nAh, that makes sense. It's basically just feeding the commit to\n\"diff-tree\" (except doing it internally rather than running it as a\nseparate program). And the defaults there do not show anything for merge\ncommits. It could do the equivalent of \"--cc\" (i.e., set the\ndense_combined_merges flag in the \"struct rev_info\").\n\n> > (Actually, it looks like all this is generated in bisect.c:show_diff_tree,\n> > so it would have to be written in C; but it should be pretty easy to\n> > tweak the display options).\n> \n> yeah, that seems pretty straight forward, but I'm not really sure what\n> to do about this case where no diff is printed, I guess I should figure\n> out what bits need to be set for the commit to be shown anyway.\n\nI'd argue for simply never showing the diff (dropping the \"opt.diff = 1\"\nline from bisect.c:show_diff_tree), but that is mostly my personal\nopinion. If we are going to show a diff, perhaps \"--recursive\n--name-status\" would be the most friendly, with \"--cc\" for the merge\ncommits.\n\nTranslated into C, something like (this is completely untested):\n\ndiff --git a/bisect.c b/bisect.c\nindex 10f5e57..62786cf 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -876,6 +876,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \topt.abbrev = 0;\n \topt.diff = 1;\n+\topt.combine_merges = 1;\n+\topt.dense_combined_merges = 1;\n \n \t/* This is what \"--pretty\" does */\n \topt.verbose_header = 1;\n@@ -884,7 +886,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n \n \t/* diff-tree init */\n \tif (!opt.diffopt.output_format)\n-\t\topt.diffopt.output_format = DIFF_FORMAT_RAW;\n+\t\topt.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;\n+\tDIFF_OPT_SET(&opt.diffopt, RECURSIVE);\n \n \tlog_tree_commit(&opt, commit);\n }\n"},{"id":"260950","messageId":"xmqqmw1bg2dd.fsf@gitster.dls.corp.google.com","threadId":"39281","inReplyTo":"20150511011009.GA21830@peff.net","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-11T04:33:02Z","receivedAt":"2015-05-11T04:33:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'd argue for simply never showing the diff (dropping the \"opt.diff = 1\"\n> line from bisect.c:show_diff_tree), but that is mostly my personal\n> opinion.\n\nYeah, I think that is sensible. It may even be OK to just give a\n\"log --oneline\".  \n"},{"id":"260959","messageId":"CAP8UFD1Aq54dWvxo5JTP4Fqy5u-qhA0LAm3vRrw9=jYg3o_F+g@mail.gmail.com","threadId":"39281","inReplyTo":"xmqqmw1bg2dd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-11T07:38:12Z","receivedAt":"2015-05-11T07:38:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, May 11, 2015 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> I'd argue for simply never showing the diff (dropping the \"opt.diff = 1\"\n>> line from bisect.c:show_diff_tree), but that is mostly my personal\n>> opinion.\n>\n> Yeah, I think that is sensible. It may even be OK to just give a\n> \"log --oneline\".\n\nOr maybe we could let the user configure the diff options or even the\ncommand used when the first bad commit is found?\n"},{"id":"260992","messageId":"xmqqfv73f420.fsf@gitster.dls.corp.google.com","threadId":"39281","inReplyTo":"CAP8UFD1Aq54dWvxo5JTP4Fqy5u-qhA0LAm3vRrw9=jYg3o_F+g@mail.gmail.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-11T16:54:15Z","receivedAt":"2015-05-11T16:54:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Mon, May 11, 2015 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Jeff King <peff@peff.net> writes:\n>>\n>>> I'd argue for simply never showing the diff (dropping the \"opt.diff = 1\"\n>>> line from bisect.c:show_diff_tree), but that is mostly my personal\n>>> opinion.\n>>\n>> Yeah, I think that is sensible. It may even be OK to just give a\n>> \"log --oneline\".\n>\n> Or maybe we could let the user configure the diff options or even the\n> command used when the first bad commit is found?\n\nThat is a separate discussion.  I do not mind but I doubt many\npeople would use it (I was tempted to say \"doubt anybody would\", but\nthen was reminded how many people use Git, and toned it down), as\nlong as we have a good default.  And I thought that this discussion\nwas about coming up with a good-enough default.\n\nTo be bluntly honest, I think the current one is sufficient as a\ngood-enough default.  The first thing I would do after seeing that\nmessage is to either \"git checkout <commit-object-name>\" or \"git\nshow <commit-object-name>\", and the current full 40-hex output gives\nme an easier mouse-double-click target than the proposed abbreviated\none, so in that sense the original proposal may even be a usability\nregression.\n\nIt is tempting to say that the output can be eliminated by always\nchecking out the first-bad-commit (i.e. only when the last answer\nthat led to the first-bad decision was \"good\", do a \"git checkout\"\nof that bad commit), but in a project where a branch switching is\nnot instantaneous, that might be problematic (unless the first step\nthe user would have done is to check it out anyway, of course).\n"},{"id":"261008","messageId":"20150511181719.GB18112@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"39281","inReplyTo":"xmqqfv73f420.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-11T18:17:19Z","receivedAt":"2015-05-11T18:17:19Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Mon, May 11, 2015 at 09:54:15AM -0700, Junio C Hamano wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n> \n> > On Mon, May 11, 2015 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> >> Jeff King <peff@peff.net> writes:\n> >>\n> >>> I'd argue for simply never showing the diff (dropping the \"opt.diff = 1\"\n> >>> line from bisect.c:show_diff_tree), but that is mostly my personal\n> >>> opinion.\n> >>\n> >> Yeah, I think that is sensible. It may even be OK to just give a\n> >> \"log --oneline\".\n> >\n> > Or maybe we could let the user configure the diff options or even the\n> > command used when the first bad commit is found?\n> \n> That is a separate discussion.  I do not mind but I doubt many\n> people would use it (I was tempted to say \"doubt anybody would\", but\n> then was reminded how many people use Git, and toned it down), as\n> long as we have a good default.  And I thought that this discussion\n> was about coming up with a good-enough default.\n\nagreed\n\n> To be bluntly honest, I think the current one is sufficient as a\n> good-enough default.  The first thing I would do after seeing that\n> message is to either \"git checkout <commit-object-name>\" or \"git\n> show <commit-object-name>\", and the current full 40-hex output gives\n> me an easier mouse-double-click target than the proposed abbreviated\n> one, so in that sense the original proposal may even be a usability\n> regression.\n\nI think printing the full 40 chars once is reasonable, but twice in 2\nlines seems a bit excessive.  I was thinking of changing the format to\nbe\n\nthe first bad commit is\n$(git log -1 <bad sha1>)\n\n> It is tempting to say that the output can be eliminated by always\n> checking out the first-bad-commit (i.e. only when the last answer\n> that led to the first-bad decision was \"good\", do a \"git checkout\"\n> of that bad commit), but in a project where a branch switching is\n> not instantaneous, that might be problematic (unless the first step\n> the user would have done is to check it out anyway, of course).\n\nWell,  if you just finished bisecting you are probably on a commit close\nto the first bad one so it probably will be fast.  However I don't\nreally like that idea because what I generally want to do is read the\npatch so having the hash printed so I can copy it and run git show -p\n$hash or something is nice.  Though I guess if the first bad commit is\nchecked out you can just skip the copy paste and use HEAD.\n\nTrev\n"},{"id":"261009","messageId":"CAGZ79kZwjpxP0mDt8YRvvhsOsTfzjNbMFUPwUBfBr-uKaghQTw@mail.gmail.com","threadId":"39281","inReplyTo":"20150511181719.GB18112@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-11T18:28:24Z","receivedAt":"2015-05-11T18:28:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 11, 2015 at 11:17 AM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:\n> On Mon, May 11, 2015 at 09:54:15AM -0700, Junio C Hamano wrote:\n>> Christian Couder <christian.couder@gmail.com> writes:\n>>\n>> > On Mon, May 11, 2015 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> >> Jeff King <peff@peff.net> writes:\n>> >>\n>> >>> I'd argue for simply never showing the diff (dropping the \"opt.diff = 1\"\n>> >>> line from bisect.c:show_diff_tree), but that is mostly my personal\n>> >>> opinion.\n>> >>\n>> >> Yeah, I think that is sensible. It may even be OK to just give a\n>> >> \"log --oneline\".\n>> >\n>> > Or maybe we could let the user configure the diff options or even the\n>> > command used when the first bad commit is found?\n>>\n>> That is a separate discussion.  I do not mind but I doubt many\n>> people would use it (I was tempted to say \"doubt anybody would\", but\n>> then was reminded how many people use Git, and toned it down), as\n>> long as we have a good default.  And I thought that this discussion\n>> was about coming up with a good-enough default.\n>\n> agreed\n>\n>> To be bluntly honest, I think the current one is sufficient as a\n>> good-enough default.  The first thing I would do after seeing that\n>> message is to either \"git checkout <commit-object-name>\" or \"git\n>> show <commit-object-name>\", and the current full 40-hex output gives\n>> me an easier mouse-double-click target than the proposed abbreviated\n>> one, so in that sense the original proposal may even be a usability\n>> regression.\n>\n> I think printing the full 40 chars once is reasonable, but twice in 2\n> lines seems a bit excessive.  I was thinking of changing the format to\n> be\n>\n> the first bad commit is\n> $(git log -1 <bad sha1>)\n>\n>> It is tempting to say that the output can be eliminated by always\n>> checking out the first-bad-commit (i.e. only when the last answer\n>> that led to the first-bad decision was \"good\", do a \"git checkout\"\n>> of that bad commit), but in a project where a branch switching is\n>> not instantaneous, that might be problematic (unless the first step\n>> the user would have done is to check it out anyway, of course).\n>\n> Well,  if you just finished bisecting you are probably on a commit close\n> to the first bad one so it probably will be fast.  However I don't\n> really like that idea because what I generally want to do is read the\n> patch so having the hash printed so I can copy it and run git show -p\n> $hash or something is nice.  Though I guess if the first bad commit is\n> checked out you can just skip the copy paste and use HEAD.\n\nOnly if you are operating within your local repository only. If you want\nto check a build bot or another database for continuous builds or anything\noutside your local repository you need the hash and not HEAD.\nSo I'd think\n\n    the first bad commit is\n    $(git log -1 <bad sha1>)\n\nis fine.\n\n>\n> Trev\n>\n"},{"id":"261044","messageId":"CAP8UFD3LzM3uuUzWYS-o6mhtH-x5+-kyGhDvYnv6ZPRTC18C6w@mail.gmail.com","threadId":"39281","inReplyTo":"xmqqfv73f420.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-12T09:21:57Z","receivedAt":"2015-05-12T09:21:57Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, May 11, 2015 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> On Mon, May 11, 2015 at 6:33 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Jeff King <peff@peff.net> writes:\n>>>\n>>>> I'd argue for simply never showing the diff (dropping the \"opt.diff = 1\"\n>>>> line from bisect.c:show_diff_tree), but that is mostly my personal\n>>>> opinion.\n>>>\n>>> Yeah, I think that is sensible. It may even be OK to just give a\n>>> \"log --oneline\".\n>>\n>> Or maybe we could let the user configure the diff options or even the\n>> command used when the first bad commit is found?\n>\n> That is a separate discussion.  I do not mind but I doubt many\n> people would use it (I was tempted to say \"doubt anybody would\", but\n> then was reminded how many people use Git, and toned it down), as\n> long as we have a good default.  And I thought that this discussion\n> was about coming up with a good-enough default.\n>\n> To be bluntly honest, I think the current one is sufficient as a\n> good-enough default.  The first thing I would do after seeing that\n> message is to either \"git checkout <commit-object-name>\" or \"git\n> show <commit-object-name>\", and the current full 40-hex output gives\n> me an easier mouse-double-click target than the proposed abbreviated\n> one, so in that sense the original proposal may even be a usability\n> regression.\n\nYeah, it might also be a regression if some users have scripts that\ndepend on the current behavior. That's why with a config option,\npeople annoyed by the current behavior can get exactly what they want,\nand it makes it possible to more safely change the default to\nsomething more user friendly the next time we change the major version\nnumber.\n\n> It is tempting to say that the output can be eliminated by always\n> checking out the first-bad-commit (i.e. only when the last answer\n> that led to the first-bad decision was \"good\", do a \"git checkout\"\n> of that bad commit), but in a project where a branch switching is\n> not instantaneous, that might be problematic (unless the first step\n> the user would have done is to check it out anyway, of course).\n\nYeah, and speaking of regressions, elimiting the output might be a\nmore serious regression.\n\nBest,\nChristian.\n"},{"id":"261053","messageId":"xmqq7fsd201d.fsf@gitster.dls.corp.google.com","threadId":"39281","inReplyTo":"CAP8UFD3LzM3uuUzWYS-o6mhtH-x5+-kyGhDvYnv6ZPRTC18C6w@mail.gmail.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-12T17:11:42Z","receivedAt":"2015-05-12T17:11:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> On Mon, May 11, 2015 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> To be bluntly honest, I think the current one is sufficient as a\n>> good-enough default.  The first thing I would do after seeing that\n>> message is to either \"git checkout <commit-object-name>\" or \"git\n>> show <commit-object-name>\", and the current full 40-hex output gives\n>> me an easier mouse-double-click target than the proposed abbreviated\n>> one, so in that sense the original proposal may even be a usability\n>> regression.\n>\n> Yeah, it might also be a regression if some users have scripts that\n> depend on the current behavior.\n> ...\n>> It is tempting to say that the output can be eliminated by always\n>> checking out the first-bad-commit (i.e. only when the last answer\n>> that led to the first-bad decision was \"good\", do a \"git checkout\"\n>> of that bad commit), but in a project where a branch switching is\n>> not instantaneous, that might be problematic (unless the first step\n>> the user would have done is to check it out anyway, of course).\n>\n> Yeah, and speaking of regressions, elimiting the output might be a\n> more serious regression.\n\nI am getting somewhat annoyed by this line of thought.\n\nWho said bisect output is meant to be parseable and be read by\nscripts in the first place?  If that were the case, we wouldn't be\nhaving this discussion thread in the first place.\n"},{"id":"261076","messageId":"CAP8UFD0k-=ESEu-7jhf8Y5wz+5A=MHsjtMnC7YJv_DRi30TmDw@mail.gmail.com","threadId":"39281","inReplyTo":"xmqq7fsd201d.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-12T20:43:29Z","receivedAt":"2015-05-12T20:43:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, May 12, 2015 at 7:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> On Mon, May 11, 2015 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>> To be bluntly honest, I think the current one is sufficient as a\n>>> good-enough default.  The first thing I would do after seeing that\n>>> message is to either \"git checkout <commit-object-name>\" or \"git\n>>> show <commit-object-name>\", and the current full 40-hex output gives\n>>> me an easier mouse-double-click target than the proposed abbreviated\n>>> one, so in that sense the original proposal may even be a usability\n>>> regression.\n>>\n>> Yeah, it might also be a regression if some users have scripts that\n>> depend on the current behavior.\n>> ...\n>>> It is tempting to say that the output can be eliminated by always\n>>> checking out the first-bad-commit (i.e. only when the last answer\n>>> that led to the first-bad decision was \"good\", do a \"git checkout\"\n>>> of that bad commit), but in a project where a branch switching is\n>>> not instantaneous, that might be problematic (unless the first step\n>>> the user would have done is to check it out anyway, of course).\n>>\n>> Yeah, and speaking of regressions, elimiting the output might be a\n>> more serious regression.\n>\n> I am getting somewhat annoyed by this line of thought.\n>\n> Who said bisect output is meant to be parseable and be read by\n> scripts in the first place?  If that were the case, we wouldn't be\n> having this discussion thread in the first place.\n\nWell \"git bisect run\" is all about automating bisecting and we know\nthat some people have been using it for a long time.\n\nSee for example this message from 2007:\n\nhttp://lkml.iu.edu/hypermail/linux/kernel/0711.1/1443.html\n\nwhere there is:\n\n\"Today we can autonomouly\nbisect build bugs via a simple shell command around \"git-bisect run\",\nwithout any human interaction!\"\n\nSo it is reasonnable to wonder if some scripts might be parsing\nthe output.\n"},{"id":"261077","messageId":"CAGZ79kZG=9BkEGB_GOsg7F-2mN5iTjmTFK+vUohj_7wJLfPtig@mail.gmail.com","threadId":"39281","inReplyTo":"CAP8UFD0k-=ESEu-7jhf8Y5wz+5A=MHsjtMnC7YJv_DRi30TmDw@mail.gmail.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-12T20:58:56Z","receivedAt":"2015-05-12T20:58:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, May 12, 2015 at 1:43 PM, Christian Couder\n<christian.couder@gmail.com> wrote:\n> On Tue, May 12, 2015 at 7:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Christian Couder <christian.couder@gmail.com> writes:\n>>\n>>> On Mon, May 11, 2015 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>\n>>>> To be bluntly honest, I think the current one is sufficient as a\n>>>> good-enough default.  The first thing I would do after seeing that\n>>>> message is to either \"git checkout <commit-object-name>\" or \"git\n>>>> show <commit-object-name>\", and the current full 40-hex output gives\n>>>> me an easier mouse-double-click target than the proposed abbreviated\n>>>> one, so in that sense the original proposal may even be a usability\n>>>> regression.\n>>>\n>>> Yeah, it might also be a regression if some users have scripts that\n>>> depend on the current behavior.\n>>> ...\n>>>> It is tempting to say that the output can be eliminated by always\n>>>> checking out the first-bad-commit (i.e. only when the last answer\n>>>> that led to the first-bad decision was \"good\", do a \"git checkout\"\n>>>> of that bad commit), but in a project where a branch switching is\n>>>> not instantaneous, that might be problematic (unless the first step\n>>>> the user would have done is to check it out anyway, of course).\n>>>\n>>> Yeah, and speaking of regressions, elimiting the output might be a\n>>> more serious regression.\n>>\n>> I am getting somewhat annoyed by this line of thought.\n>>\n>> Who said bisect output is meant to be parseable and be read by\n>> scripts in the first place?  If that were the case, we wouldn't be\n>> having this discussion thread in the first place.\n>\n> Well \"git bisect run\" is all about automating bisecting and we know\n> that some people have been using it for a long time.\n>\n> See for example this message from 2007:\n>\n> http://lkml.iu.edu/hypermail/linux/kernel/0711.1/1443.html\n>\n> where there is:\n>\n> \"Today we can autonomouly\n> bisect build bugs via a simple shell command around \"git-bisect run\",\n> without any human interaction!\"\n>\n> So it is reasonnable to wonder if some scripts might be parsing\n> the output.\n\nThis reasoning sounds to me, that the lack of a plumbing counterpart\nto bisect(porcelain) made it a de facto plumbing command,\nwhich is unfortunate for discussing changes like these.\n\nSo how to proceed here?\n* one way would be to ignore the scripts out there, \"because it's\n  porcelain, so nobody sane would have written a script using it anyway\"\n  but this attitude is not well perceived in the community I'd assume.\n* declare the current bisect command a plumbing layer command and\n  introduce a new porcelain command, how about \"git find\" which can address\n  a variety of issues such as also having the capability to find a fix\ninstead of\n  just regressions (make good/bad markers less confusing)\n  Depending on the implementation this may be a lot of work\n  -> copy/paste is fast and involves less work now, but more in the future\n  -> or having a new plumbing-bisect header making calls from the porcelain\n      to the plumbing bisect tool.\n"},{"id":"261102","messageId":"20150512234014.GE31257@tsaunders-iceball.corp.tor1.mozilla.com","threadId":"39281","inReplyTo":"CAGZ79kZG=9BkEGB_GOsg7F-2mN5iTjmTFK+vUohj_7wJLfPtig@mail.gmail.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Trevor Saunders","fromEmail":"tbsaunde@tbsaunde.org","sentAt":"2015-05-12T23:40:14Z","receivedAt":"2015-05-12T23:40:14Z","isPatch":true,"sender":{"key":"tbsaunde@tbsaunde.org","avatar":null},"body":"On Tue, May 12, 2015 at 01:58:56PM -0700, Stefan Beller wrote:\n> On Tue, May 12, 2015 at 1:43 PM, Christian Couder\n> <christian.couder@gmail.com> wrote:\n> > On Tue, May 12, 2015 at 7:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> >> Christian Couder <christian.couder@gmail.com> writes:\n> >>\n> >>> On Mon, May 11, 2015 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> >>>\n> >>>> To be bluntly honest, I think the current one is sufficient as a\n> >>>> good-enough default.  The first thing I would do after seeing that\n> >>>> message is to either \"git checkout <commit-object-name>\" or \"git\n> >>>> show <commit-object-name>\", and the current full 40-hex output gives\n> >>>> me an easier mouse-double-click target than the proposed abbreviated\n> >>>> one, so in that sense the original proposal may even be a usability\n> >>>> regression.\n> >>>\n> >>> Yeah, it might also be a regression if some users have scripts that\n> >>> depend on the current behavior.\n> >>> ...\n> >>>> It is tempting to say that the output can be eliminated by always\n> >>>> checking out the first-bad-commit (i.e. only when the last answer\n> >>>> that led to the first-bad decision was \"good\", do a \"git checkout\"\n> >>>> of that bad commit), but in a project where a branch switching is\n> >>>> not instantaneous, that might be problematic (unless the first step\n> >>>> the user would have done is to check it out anyway, of course).\n> >>>\n> >>> Yeah, and speaking of regressions, elimiting the output might be a\n> >>> more serious regression.\n> >>\n> >> I am getting somewhat annoyed by this line of thought.\n> >>\n> >> Who said bisect output is meant to be parseable and be read by\n> >> scripts in the first place?  If that were the case, we wouldn't be\n> >> having this discussion thread in the first place.\n> >\n> > Well \"git bisect run\" is all about automating bisecting and we know\n> > that some people have been using it for a long time.\n> >\n> > See for example this message from 2007:\n> >\n> > http://lkml.iu.edu/hypermail/linux/kernel/0711.1/1443.html\n> >\n> > where there is:\n> >\n> > \"Today we can autonomouly\n> > bisect build bugs via a simple shell command around \"git-bisect run\",\n> > without any human interaction!\"\n> >\n> > So it is reasonnable to wonder if some scripts might be parsing\n> > the output.\n> \n> This reasoning sounds to me, that the lack of a plumbing counterpart\n> to bisect(porcelain) made it a de facto plumbing command,\n> which is unfortunate for discussing changes like these.\n> \n> So how to proceed here?\n> * one way would be to ignore the scripts out there, \"because it's\n>   porcelain, so nobody sane would have written a script using it anyway\"\n>   but this attitude is not well perceived in the community I'd assume.\n\nhonestly in this case I'd be inclined to go that route since the output\nisn't really great for parsing so I do find it hard to believe there is\na reasonable number of scripts that use git bisect, and depend on its\noutput.\n\n> * declare the current bisect command a plumbing layer command and\n>   introduce a new porcelain command, how about \"git find\" which can address\n>   a variety of issues such as also having the capability to find a fix\n> instead of\n>   just regressions (make good/bad markers less confusing)\n\nSolving that issue would be nice, but I think git find is much less\nintuitive to new people than bisect, but I'm not really a good judge of\nthat.\n\n>   Depending on the implementation this may be a lot of work\n>   -> copy/paste is fast and involves less work now, but more in the future\n>   -> or having a new plumbing-bisect header making calls from the porcelain\n>       to the plumbing bisect tool.\n\nmy sense is that the division between git-bisect.sh and bisect--helper.c\nisn't really great and could already use refactoring so I suspect it'd\nbe a fair amount of work.\n\nTrev\n"},{"id":"261177","messageId":"CAP8UFD1eA8U8DnjY2qqCxR5HEtd3EzFJ0Ck8CNrWAh_25YyaXQ@mail.gmail.com","threadId":"39281","inReplyTo":"20150512234014.GE31257@tsaunders-iceball.corp.tor1.mozilla.com","subject":"Re: [PATCH] bisect: print abbrev sha1 for first bad commit","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-05-13T13:24:40Z","receivedAt":"2015-05-13T13:24:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, May 13, 2015 at 1:40 AM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:\n>\n> my sense is that the division between git-bisect.sh and bisect--helper.c\n> isn't really great and could already use refactoring so I suspect it'd\n> be a fair amount of work.\n\nAbout the division between git-bisect.sh and bisect--helper.c, yeah I\nstarted converting git-bisect.sh into C code, but haven't finished\nthat.\n"}]}