[PATCH v2] bisect: improve output when bad commit is found

[PATCH v2] bisect: improve output when bad commit is found

[Trevor Saunders]

When the first bad commit has been found git bisect prints something
like this:

	<40 char sha1> is the first bad commit
	Commit <40 char sha1>
	...

	:100644 100644 10f5e57... a46cfeb... M  bisect.c
	:100755 100755 ae3fec2... 65a19fa... M  git-bisect.sh

The raw diff output is not really useful, and its kind of silly to print
the sha1 twice.  Instead lets print something like this:

	The first bad commit is
	Commit <sha1>
	...

This also fixes an odd inconsistancy where if the first bad commit is a
trivial merge git bisect will only print the first line.

Signed-off-by: Trevor Saunders <tbsaunde@tbsaunde.org>
---
 bisect.c                    |  9 +++------
 git-bisect.sh               |  2 +-
 t/t6030-bisect-porcelain.sh | 36 +++++++++++++++++++++++++-----------
 3 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/bisect.c b/bisect.c
index 10f5e57..a46cfeb 100644
--- a/bisect.c
+++ b/bisect.c
@@ -875,17 +875,14 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 	init_revisions(&opt, prefix);
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	opt.abbrev = 0;
-	opt.diff = 1;
+	opt.diff = 0;
+	opt.always_show_header = 1;
 
 	/* This is what "--pretty" does */
 	opt.verbose_header = 1;
 	opt.use_terminator = 0;
 	opt.commit_format = CMIT_FMT_DEFAULT;
 
-	/* diff-tree init */
-	if (!opt.diffopt.output_format)
-		opt.diffopt.output_format = DIFF_FORMAT_RAW;
-
 	log_tree_commit(&opt, commit);
 }
 
@@ -942,7 +939,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
 	if (!hashcmp(bisect_rev, current_bad_oid->hash)) {
 		exit_if_skipped_commits(tried, current_bad_oid);
-		printf("%s is the first bad commit\n", bisect_rev_hex);
+		puts("The first bad commit is");
 		show_diff_tree(prefix, revs.commits->item);
 		/* This means the bisection process succeeded. */
 		exit(10);
diff --git a/git-bisect.sh b/git-bisect.sh
index ae3fec2..65a19fa 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -480,7 +480,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 			exit $res
 		fi
 
-		if sane_grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" >/dev/null
+		if sane_grep "The first bad commit is" "$GIT_DIR/BISECT_RUN" >/dev/null
 		then
 			gettextln "bisect run success"
 			exit 0;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 06b4868..43f420d 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -26,6 +26,20 @@ add_line_into_file()
     git commit --quiet -m "$MSG" $_file
 }
 
+check_bisect_msg () {
+	file=$1
+	hash=$2
+	echo "The first bad commit is" > expect &&
+	git show -s --no-abbrev $hash >> expect &&
+	if test -n "$3"
+       	then
+		echo $3 >> expect || return $?
+	fi
+	cnt=$(wc -l <expect) &&
+	tail -n $cnt "$file" > actual &&
+	test_cmp expect actual
+}
+
 HASH1=
 HASH2=
 HASH3=
@@ -189,7 +203,7 @@ test_expect_success 'bisect skip: successful result' '
 	git bisect start $HASH4 $HASH1 &&
 	git bisect skip &&
 	git bisect bad > my_bisect_log.txt &&
-	grep "$HASH2 is the first bad commit" my_bisect_log.txt
+	check_bisect_msg my_bisect_log.txt $HASH2
 '
 
 # $HASH1 is good, $HASH4 is bad, we skip $HASH3 and $HASH2
@@ -254,7 +268,7 @@ test_expect_success \
      git bisect good $HASH1 &&
      git bisect bad $HASH4 &&
      git bisect run ./test_script.sh > my_bisect_log.txt &&
-     grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
+     check_bisect_msg my_bisect_log.txt $HASH3 "bisect run success" &&
      git bisect reset'
 
 # We want to automatically find the commit that
@@ -267,7 +281,7 @@ test_expect_success \
      chmod +x test_script.sh &&
      git bisect start $HASH4 $HASH1 &&
      git bisect run ./test_script.sh > my_bisect_log.txt &&
-     grep "$HASH4 is the first bad commit" my_bisect_log.txt &&
+     check_bisect_msg my_bisect_log.txt $HASH4 "bisect run success" &&
      git bisect reset'
 
 # $HASH1 is good, $HASH5 is bad, we skip $HASH3
@@ -280,14 +294,14 @@ test_expect_success 'bisect skip: add line and then a new test' '
 	git bisect start $HASH5 $HASH1 &&
 	git bisect skip &&
 	git bisect good > my_bisect_log.txt &&
-	grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
+	check_bisect_msg my_bisect_log.txt $HASH5 &&
 	git bisect log > log_to_replay.txt &&
 	git bisect reset
 '
 
 test_expect_success 'bisect skip and bisect replay' '
 	git bisect replay log_to_replay.txt > my_bisect_log.txt &&
-	grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
+	check_bisect_msg my_bisect_log.txt $HASH5 &&
 	git bisect reset
 '
 
@@ -328,7 +342,7 @@ test_expect_success 'bisect run & skip: find first bad' '
 	chmod +x test_script.sh &&
 	git bisect start $HASH7 $HASH1 &&
 	git bisect run ./test_script.sh > my_bisect_log.txt &&
-	grep "$HASH6 is the first bad commit" my_bisect_log.txt
+	check_bisect_msg my_bisect_log.txt $HASH6 "bisect run success"
 '
 
 test_expect_success 'bisect skip only one range' '
@@ -378,7 +392,7 @@ test_expect_success 'bisect does not create a "bisect" branch' '
 	rev_hash6=$(git rev-parse --verify HEAD) &&
 	test "$rev_hash6" = "$HASH6" &&
 	git bisect good > my_bisect_log.txt &&
-	grep "$HASH7 is the first bad commit" my_bisect_log.txt &&
+	check_bisect_msg my_bisect_log.txt $HASH7 &&
 	git bisect reset &&
 	rev_hash6=$(git rev-parse --verify bisect) &&
 	test "$rev_hash6" = "$HASH6" &&
@@ -527,7 +541,7 @@ test_expect_success 'restricting bisection on one dir' '
 	para1=$(git rev-parse --verify HEAD) &&
 	test "$para1" = "$PARA_HASH1" &&
 	git bisect bad > my_bisect_log.txt &&
-	grep "$PARA_HASH1 is the first bad commit" my_bisect_log.txt
+	check_bisect_msg my_bisect_log.txt $PARA_HASH1
 '
 
 test_expect_success 'restricting bisection on one dir and a file' '
@@ -545,7 +559,7 @@ test_expect_success 'restricting bisection on one dir and a file' '
 	para1=$(git rev-parse --verify HEAD) &&
 	test "$para1" = "$PARA_HASH1" &&
 	git bisect good > my_bisect_log.txt &&
-	grep "$PARA_HASH4 is the first bad commit" my_bisect_log.txt
+	check_bisect_msg my_bisect_log.txt $PARA_HASH4
 '
 
 test_expect_success 'skipping away from skipped commit' '
@@ -576,7 +590,7 @@ test_expect_success 'test bisection on bare repo - --no-checkout specified' '
 			"test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \
 			>../nocheckout.log
 	) &&
-	grep "$HASH3 is the first bad commit" nocheckout.log
+	check_bisect_msg  nocheckout.log $HASH3 "bisect run success"
 '
 
 
@@ -591,7 +605,7 @@ test_expect_success 'test bisection on bare repo - --no-checkout defaulted' '
 			"test \$(git rev-list BISECT_HEAD ^$HASH2 --max-count=1 | wc -l) = 0" \
 			>../defaulted.log
 	) &&
-	grep "$HASH3 is the first bad commit" defaulted.log
+	check_bisect_msg defaulted.log $HASH3 "bisect run success"
 '
 
 #
-- 
2.4.0.78.g7c6ecbf

Re: [PATCH v2] bisect: improve output when bad commit is found

[Junio C Hamano]

Trevor Saunders <tbsaunde@tbsaunde.org> writes:

> When the first bad commit has been found git bisect prints something
> like this:
>
> 	<40 char sha1> is the first bad commit
> 	Commit <40 char sha1>
> 	...
>
> 	:100644 100644 10f5e57... a46cfeb... M  bisect.c
> 	:100755 100755 ae3fec2... 65a19fa... M  git-bisect.sh
>
> The raw diff output is not really useful, and its kind of silly to print
> the sha1 twice.  Instead lets print something like this:
>
> 	The first bad commit is
> 	Commit <sha1>
> 	...

According to +CCouder, this change will break existing people's use
cases.

See $gmane/268881

Re: [PATCH v2] bisect: improve output when bad commit is found

[Trevor Saunders]

On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:
> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> 
> > When the first bad commit has been found git bisect prints something
> > like this:
> >
> > 	<40 char sha1> is the first bad commit
> > 	Commit <40 char sha1>
> > 	...
> >
> > 	:100644 100644 10f5e57... a46cfeb... M  bisect.c
> > 	:100755 100755 ae3fec2... 65a19fa... M  git-bisect.sh
> >
> > The raw diff output is not really useful, and its kind of silly to print
> > the sha1 twice.  Instead lets print something like this:
> >
> > 	The first bad commit is
> > 	Commit <sha1>
> > 	...
> 
> According to +CCouder, this change will break existing people's use
> cases.
> 
> See $gmane/268881

Well, technically he just said it might be that people are parsing the
output and could be broken, but if you'd rather not take that risk then
I guess we just have to leave things the way they are.

Trev

Re: [PATCH v2] bisect: improve output when bad commit is found

[Junio C Hamano]

Trevor Saunders <tbsaunde@tbsaunde.org> writes:

> On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:
>> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
>> 
>> > When the first bad commit has been found git bisect prints something
>> > like this:
>> >
>> > 	<40 char sha1> is the first bad commit
>> > 	Commit <40 char sha1>
>> > 	...
>> >
>> > 	:100644 100644 10f5e57... a46cfeb... M  bisect.c
>> > 	:100755 100755 ae3fec2... 65a19fa... M  git-bisect.sh
>> >
>> > The raw diff output is not really useful, and its kind of silly to print
>> > the sha1 twice.  Instead lets print something like this:
>> >
>> > 	The first bad commit is
>> > 	Commit <sha1>
>> > 	...
>> 
>> According to +CCouder, this change will break existing people's use
>> cases.
>> 
>> See $gmane/268881
>
> Well, technically he just said it might be that people are parsing the
> output and could be broken, but if you'd rather not take that risk then
> I guess we just have to leave things the way they are.

FWIW.

 - I personally do not agree that those who scripted around "git
   bisect" (as opposed to those who wrote scripts to be driven by
   the "bisect run" interface) are worth worrying about.  But I am
   not the whole of the Git world ;-)

 - I personally do not find two same 40-hex on two lines is silly at
   all.

 - I _do_ think diff-tree --raw output without recursive is silly.
   It is not useful for humans (it doesn't even give paths fully),
   and it is insufficient for scripts, which can grok more through
   information out of the 40-hex.

So perhaps if we keep

	<40 char sha1> is the first bad commit

and then replace the diff-tree output with "show -s", then the
result would be good enough, I would say.

Re: [PATCH v2] bisect: improve output when bad commit is found

[Jeff King]

On Tue, May 12, 2015 at 06:13:48PM -0700, Junio C Hamano wrote:

> >> See $gmane/268881
> >
> > Well, technically he just said it might be that people are parsing the
> > output and could be broken, but if you'd rather not take that risk then
> > I guess we just have to leave things the way they are.
> 
> FWIW.
> 
>  - I personally do not agree that those who scripted around "git
>    bisect" (as opposed to those who wrote scripts to be driven by
>    the "bisect run" interface) are worth worrying about.  But I am
>    not the whole of the Git world ;-)

It is not clear to me that people are actually scripting around the
output. Between the exit code and the stable output in BISECT_LOG, that
seems like a much more preferable way for scripted uses to find out what
happened.

Of course, that is not a guarantee that nobody scraped stderr, but at
least it makes me feel better that they're Doing It Wrong. :)

It would be nice if we had some actual data points. I followed the link
Christian gave to Ingo's old post, but I didn't see the actual script
there. There is:

  https://github.com/grosser/git-autobisect/blob/master/lib/git/autobisect.rb

which does seem to scrape stderr. Bleh.

>  - I _do_ think diff-tree --raw output without recursive is silly.
>    It is not useful for humans (it doesn't even give paths fully),
>    and it is insufficient for scripts, which can grok more through
>    information out of the 40-hex.
> 
> So perhaps if we keep
> 
> 	<40 char sha1> is the first bad commit
> 
> and then replace the diff-tree output with "show -s", then the
> result would be good enough, I would say.

That seems like a reasonable first step, at the very least. I wonder if
we should also better document the exit code and BISECT_LOG semantics,
and explicitly tell people not to scrape stderr. That at least frees us
up later to change the output as we see fit (I notice that most of the
messages in the script are already i18n'd; the ones from bisect--helper
are the odd ones out).

-Peff

Re: [PATCH v2] bisect: improve output when bad commit is found

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Of course, that is not a guarantee that nobody scraped stderr, but at
> least it makes me feel better that they're Doing It Wrong. :)
>
> It would be nice if we had some actual data points. I followed the link
> Christian gave to Ingo's old post, but I didn't see the actual script
> there. There is:
>
>   https://github.com/grosser/git-autobisect/blob/master/lib/git/autobisect.rb
>
> which does seem to scrape stderr. Bleh.

Heh, anything ending with .rb is a bleh to me ;-)

>> So perhaps if we keep
>> 
>> 	<40 char sha1> is the first bad commit
>> 
>> and then replace the diff-tree output with "show -s", then the
>> result would be good enough, I would say.
>
> That seems like a reasonable first step, at the very least. I wonder if
> we should also better document the exit code and BISECT_LOG semantics,
> and explicitly tell people not to scrape stderr.

Yeah, that would also be a good first step, whichever comes first.

Re: [PATCH v2] bisect: improve output when bad commit is found

[Christian Couder]

On Wed, May 13, 2015 at 3:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
>
>> On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:
>>> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
>>>
>>> > When the first bad commit has been found git bisect prints something
>>> > like this:
>>> >
>>> >    <40 char sha1> is the first bad commit
>>> >    Commit <40 char sha1>
>>> >    ...
>>> >
>>> >    :100644 100644 10f5e57... a46cfeb... M  bisect.c
>>> >    :100755 100755 ae3fec2... 65a19fa... M  git-bisect.sh
>>> >
>>> > The raw diff output is not really useful, and its kind of silly to print
>>> > the sha1 twice.  Instead lets print something like this:
>>> >
>>> >    The first bad commit is
>>> >    Commit <sha1>
>>> >    ...
>>>
>>> According to +CCouder, this change will break existing people's use
>>> cases.
>>>
>>> See $gmane/268881
>>
>> Well, technically he just said it might be that people are parsing the
>> output and could be broken, but if you'd rather not take that risk then
>> I guess we just have to leave things the way they are.
>
> FWIW.
>
>  - I personally do not agree that those who scripted around "git
>    bisect" (as opposed to those who wrote scripts to be driven by
>    the "bisect run" interface) are worth worrying about.  But I am
>    not the whole of the Git world ;-)

You know in git-bisect.sh:bisect_run() we do:

                if sane_grep "is the first bad commit"
"$GIT_DIR/BISECT_RUN" >/dev/null
                then
                        gettextln "bisect run success"
                        exit 0;
                fi

so we are doing it too!

>  - I personally do not find two same 40-hex on two lines is silly at
>    all.

I agree.

>  - I _do_ think diff-tree --raw output without recursive is silly.
>    It is not useful for humans (it doesn't even give paths fully),
>    and it is insufficient for scripts, which can grok more through
>    information out of the 40-hex.
>
> So perhaps if we keep
>
>         <40 char sha1> is the first bad commit
>
> and then replace the diff-tree output with "show -s", then the
> result would be good enough, I would say.

Yeah I agree.

And for people who want something else we can implement config options.

For example a bisect.outputformat that could be used like in the
following pseudo shell code:

format=$(git config bisect.outputformat)

test -z "$format" && format="medium"

git show -s --format="$format" "$firstbadcommit"

Re: [PATCH v2] bisect: improve output when bad commit is found

[Christian Couder]

On Wed, May 13, 2015 at 3:36 AM, Jeff King <peff@peff.net> wrote:
> On Tue, May 12, 2015 at 06:13:48PM -0700, Junio C Hamano wrote:
>
>> >> See $gmane/268881
>> >
>> > Well, technically he just said it might be that people are parsing the
>> > output and could be broken, but if you'd rather not take that risk then
>> > I guess we just have to leave things the way they are.
>>
>> FWIW.
>>
>>  - I personally do not agree that those who scripted around "git
>>    bisect" (as opposed to those who wrote scripts to be driven by
>>    the "bisect run" interface) are worth worrying about.  But I am
>>    not the whole of the Git world ;-)
>
> It is not clear to me that people are actually scripting around the
> output. Between the exit code and the stable output in BISECT_LOG, that
> seems like a much more preferable way for scripted uses to find out what
> happened.
>
> Of course, that is not a guarantee that nobody scraped stderr, but at
> least it makes me feel better that they're Doing It Wrong. :)

Aren't we sending the "XXXX is the first bad commit" and the diff-tree
to stdout?

Re: [PATCH v2] bisect: improve output when bad commit is found

[Trevor Saunders]

On Wed, May 13, 2015 at 11:10:31AM +0200, Christian Couder wrote:
> On Wed, May 13, 2015 at 3:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> > Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> >
> >> On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:
> >>> Trevor Saunders <tbsaunde@tbsaunde.org> writes:
> >>>
> >>> > When the first bad commit has been found git bisect prints something
> >>> > like this:
> >>> >
> >>> >    <40 char sha1> is the first bad commit
> >>> >    Commit <40 char sha1>
> >>> >    ...
> >>> >
> >>> >    :100644 100644 10f5e57... a46cfeb... M  bisect.c
> >>> >    :100755 100755 ae3fec2... 65a19fa... M  git-bisect.sh
> >>> >
> >>> > The raw diff output is not really useful, and its kind of silly to print
> >>> > the sha1 twice.  Instead lets print something like this:
> >>> >
> >>> >    The first bad commit is
> >>> >    Commit <sha1>
> >>> >    ...
> >>>
> >>> According to +CCouder, this change will break existing people's use
> >>> cases.
> >>>
> >>> See $gmane/268881
> >>
> >> Well, technically he just said it might be that people are parsing the
> >> output and could be broken, but if you'd rather not take that risk then
> >> I guess we just have to leave things the way they are.
> >
> > FWIW.
> >
> >  - I personally do not agree that those who scripted around "git
> >    bisect" (as opposed to those who wrote scripts to be driven by
> >    the "bisect run" interface) are worth worrying about.  But I am
> >    not the whole of the Git world ;-)
> 
> You know in git-bisect.sh:bisect_run() we do:
> 
>                 if sane_grep "is the first bad commit"
> "$GIT_DIR/BISECT_RUN" >/dev/null
>                 then
>                         gettextln "bisect run success"
>                         exit 0;
>                 fi
> 
> so we are doing it too!

I saw that, and thought it was pretty gross.

> >  - I personally do not find two same 40-hex on two lines is silly at
> >    all.
> 
> I agree.

*shrug* I'd probably agree more if it came last after the more useful
information the first bad commit has been found.

> >  - I _do_ think diff-tree --raw output without recursive is silly.
> >    It is not useful for humans (it doesn't even give paths fully),
> >    and it is insufficient for scripts, which can grok more through
> >    information out of the 40-hex.
> >
> > So perhaps if we keep
> >
> >         <40 char sha1> is the first bad commit
> >
> > and then replace the diff-tree output with "show -s", then the
> > result would be good enough, I would say.
> 
> Yeah I agree.

sounds like we all agree about that part at least :)

> And for people who want something else we can implement config options.
> 
> For example a bisect.outputformat that could be used like in the

I'd been thinking of a config option that was a little more user
friendly than writing "The first bad commit is%nCommit %h%n..." but
doing it that way is both simple and might allow us to replace the
printing code in bisect.c with a default format string.

Trev

Re: [PATCH v2] bisect: improve output when bad commit is found

[Jeff King]

On Wed, May 13, 2015 at 11:39:41AM +0200, Christian Couder wrote:

> > It is not clear to me that people are actually scripting around the
> > output. Between the exit code and the stable output in BISECT_LOG, that
> > seems like a much more preferable way for scripted uses to find out what
> > happened.
> >
> > Of course, that is not a guarantee that nobody scraped stderr, but at
> > least it makes me feel better that they're Doing It Wrong. :)
> 
> Aren't we sending the "XXXX is the first bad commit" and the diff-tree
> to stdout?

Good point. I'm much more sympathetic to people scraping stdout than
stderr. I do still think we would do better to direct them to more
robust formats, though.

-Peff