threads / patch / 39315

v2bisect: improve output when bad commit is found

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

## tl;dr

10 messages between May 12, 2015 and May 13, 2015. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Trevor Saunders· May 12, 2015, 23:19 UTC · lore

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(-)
Show changes to 3 files +29 −18

bisect.c, git-bisect.sh, t/t6030-bisect-porcelain.sh

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
Junio C Hamano· May 12, 2015, 23:24 UTC · re: Trevor Saunders · lore

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

Trevor Saunders <tbsaunde@tbsaunde.org> writes:
Show 16 quoted lines
> 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
Trevor Saunders· May 13, 2015, 00:54 UTC · re: Junio C Hamano · lore

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

On Tue, May 12, 2015 at 04:24:00PM -0700, Junio C Hamano wrote:
Show 23 quoted lines
> 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
Junio C Hamano· May 13, 2015, 01:13 UTC · re: Trevor Saunders · lore

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

Trevor Saunders <tbsaunde@tbsaunde.org> writes:
Show 28 quoted lines
> 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.

Jeff King· May 13, 2015, 01:36 UTC · re: Junio C Hamano · lore

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

On Tue, May 12, 2015 at 06:13:48PM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> >> 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.
Show 11 quoted lines
>  - 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
Junio C Hamano· May 13, 2015, 05:48 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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 ;-)
Show 10 quoted lines
>> 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.
Christian Couder· May 13, 2015, 09:39 UTC · re: Jeff King · lore

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

On Wed, May 13, 2015 at 3:36 AM, Jeff King <peff@peff.net> wrote:
Show 22 quoted lines
> 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?

Jeff King· May 13, 2015, 21:42 UTC · re: Christian Couder · lore

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

On Wed, May 13, 2015 at 11:39:41AM +0200, Christian Couder wrote:
Show 10 quoted lines
> > 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
Christian Couder· May 13, 2015, 09:10 UTC · re: Junio C Hamano · lore

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

On Wed, May 13, 2015 at 3:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 37 quoted lines
> 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.
Show 11 quoted lines
>  - 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"
Trevor Saunders· May 13, 2015, 17:25 UTC · re: Christian Couder · lore

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

On Wed, May 13, 2015 at 11:10:31AM +0200, Christian Couder wrote:
Show 49 quoted lines
> 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.

Show 13 quoted lines
> >  - 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

← back to recent threads