[PATCH] bisect: print abbrev sha1 for first bad commit

When bisect finds the first bad commit it prints the full commit hash
followed by " is the first bad commit".  That's not terribly readable,
and its rather silly especially considering the next line contains the
full hash again.  So change bisect to print the unique abbrev hash and
then "is the first bad commit".


---
 bisect.c                    |  3 ++-
 t/t6030-bisect-porcelain.sh | 28 +++++++++++++++++-----------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/bisect.c b/bisect.c
index 10f5e57..7cdb805 100644
--- a/bisect.c
+++ b/bisect.c

@@ -942,7 +942,8 @@ 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);
+		printf("%s is the first bad commit\n",
+			find_unique_abbrev(bisect_rev, DEFAULT_ABBREV));
 		show_diff_tree(prefix, revs.commits->item);
 		/* This means the bisection process succeeded. */
 		exit(10);

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 06b4868..14232ed 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh

@@ -26,6 +26,12 @@ add_line_into_file()
     git commit --quiet -m "$MSG" $_file
 }
 
+short()
+{
+	return git rev-parse --short $1
+}
+
+
 HASH1=
 HASH2=
 HASH3=

@@ -189,7 +195,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
+	grep "$(short $HASH2) is the first bad commit" my_bisect_log.txt
 '
 
 # $HASH1 is good, $HASH4 is bad, we skip $HASH3 and $HASH2

@@ -254,7 +260,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 &&
+     grep "$(short $HASH3) is the first bad commit" my_bisect_log.txt &&
      git bisect reset'
 
 # We want to automatically find the commit that

@@ -267,7 +273,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 &&
+     grep "$(short $HASH4) is the first bad commit" my_bisect_log.txt &&
      git bisect reset'
 
 # $HASH1 is good, $HASH5 is bad, we skip $HASH3

@@ -280,14 +286,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 &&
+	grep "$(short $HASH5) is the first bad commit" my_bisect_log.txt &&
 	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 &&
+		grep "$(short $HASH5) is the first bad commit" my_bisect_log.txt &&
 	git bisect reset
 '
 

@@ -328,7 +334,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
+	grep "$(short $HASH6) is the first bad commit" my_bisect_log.txt
 '
 
 test_expect_success 'bisect skip only one range' '

@@ -378,7 +384,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 &&
+	grep "$(short $HASH7) is the first bad commit" my_bisect_log.txt &&
 	git bisect reset &&
 	rev_hash6=$(git rev-parse --verify bisect) &&
 	test "$rev_hash6" = "$HASH6" &&

@@ -527,7 +533,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
+	grep "$(short $PARA_HASH1) is the first bad commit" my_bisect_log.txt
 '
 
 test_expect_success 'restricting bisection on one dir and a file' '

@@ -545,7 +551,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
+	grep "$(short $PARA_HASH4) is the first bad commit" my_bisect_log.txt
 '
 
 test_expect_success 'skipping away from skipped commit' '

@@ -576,7 +582,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
+		grep "$(short $HASH3) is the first bad commit" nocheckout.log
 '
 
 

@@ -591,7 +597,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
+		grep "$(short $HASH3) is the first bad commit" defaulted.log
 '
 
 #

-- 
2.4.0

On Fri, May 8, 2015 at 4:46 PM, Trevor Saunders [off-list ref] wrote:

its rather silly especially considering the next line contains the
full hash again.

Maybe we can omit it altogether then?

On Fri, May 08, 2015 at 05:29:42PM -0700, Stefan Beller wrote:

On Fri, May 8, 2015 at 4:46 PM, Trevor Saunders [off-list ref] wrote:

quoted

its rather silly especially considering the next line contains the
full hash again.

Maybe we can omit it altogether then?

SO we'd print something like

the first bad commit is
Commit abcdefabcdefabcdefabcdefabcdefabcdefabcd
Author foo@ba.com

blah blah blah

? That seems reasonable to me.  If we're going that far does it also
make sense to drop printingthe lines about which trees have changed and
just print the commit message / author / hash?

Trev

On Fri, May 08, 2015 at 10:03:41PM -0400, Trevor Saunders wrote:

On Fri, May 08, 2015 at 05:29:42PM -0700, Stefan Beller wrote:

quoted

On Fri, May 8, 2015 at 4:46 PM, Trevor Saunders [off-list ref] wrote:

quoted

its rather silly especially considering the next line contains the
full hash again.

Maybe we can omit it altogether then?

SO we'd print something like

the first bad commit is
Commit abcdefabcdefabcdefabcdefabcdefabcdefabcd
Author foo@ba.com

blah blah blah

? That seems reasonable to me.  If we're going that far does it also
make sense to drop printingthe lines about which trees have changed and
just print the commit message / author / hash?

Yeah, I have always found bisect's output somewhat silly. It prints the
"--raw" diff output, which is not incredibly useful. And then to top it
off, it does not feed the "--recursive" switch to the diff, so you don't
even get to see the real list of changed files.

I suspect the most minimal we could go is:

  git log --format='The first bad commit is %h %s' $bad

and then let the user inspect further from there using the hash. But I
think it would also be reasonable to just do a straight "git log -1
$bad" with no with no diff.

(Actually, it looks like all this is generated in bisect.c:show_diff_tree,
so it would have to be written in C; but it should be pretty easy to
tweak the display options).

-Peff

On Sat, May 09, 2015 at 12:07:04AM -0400, Jeff King wrote:

On Fri, May 08, 2015 at 10:03:41PM -0400, Trevor Saunders wrote:

quoted

On Fri, May 08, 2015 at 05:29:42PM -0700, Stefan Beller wrote:

quoted

On Fri, May 8, 2015 at 4:46 PM, Trevor Saunders [off-list ref] wrote:

quoted

its rather silly especially considering the next line contains the
full hash again.

Maybe we can omit it altogether then?

SO we'd print something like

the first bad commit is
Commit abcdefabcdefabcdefabcdefabcdefabcdefabcd
Author foo@ba.com

blah blah blah

? That seems reasonable to me.  If we're going that far does it also
make sense to drop printingthe lines about which trees have changed and
just print the commit message / author / hash?

Yeah, I have always found bisect's output somewhat silly. It prints the
"--raw" diff output, which is not incredibly useful. And then to top it
off, it does not feed the "--recursive" switch to the diff, so you don't
even get to see the real list of changed files.

 So, fun fact it doesn't actually always print the raw diffoutput if
 there is no diff, for example a merge where both sides only touched
 different files as in test 40 in t6030.

(Actually, it looks like all this is generated in bisect.c:show_diff_tree,
so it would have to be written in C; but it should be pretty easy to
tweak the display options).

yeah, that seems pretty straight forward, but I'm not really sure what
to do about this case where no diff is printed, I guess I should figure
out what bits need to be set for the commit to be shown anyway.

Trev

On Sun, May 10, 2015 at 07:12:45PM -0400, Trevor Saunders wrote:

quoted

Yeah, I have always found bisect's output somewhat silly. It prints the
"--raw" diff output, which is not incredibly useful. And then to top it
off, it does not feed the "--recursive" switch to the diff, so you don't
even get to see the real list of changed files.

 So, fun fact it doesn't actually always print the raw diffoutput if
 there is no diff, for example a merge where both sides only touched
 different files as in test 40 in t6030.

Ah, that makes sense. It's basically just feeding the commit to
"diff-tree" (except doing it internally rather than running it as a
separate program). And the defaults there do not show anything for merge
commits. It could do the equivalent of "--cc" (i.e., set the
dense_combined_merges flag in the "struct rev_info").

quoted

(Actually, it looks like all this is generated in bisect.c:show_diff_tree,
so it would have to be written in C; but it should be pretty easy to
tweak the display options).

yeah, that seems pretty straight forward, but I'm not really sure what
to do about this case where no diff is printed, I guess I should figure
out what bits need to be set for the commit to be shown anyway.

I'd argue for simply never showing the diff (dropping the "opt.diff = 1"
line from bisect.c:show_diff_tree), but that is mostly my personal
opinion. If we are going to show a diff, perhaps "--recursive
--name-status" would be the most friendly, with "--cc" for the merge
commits.

Translated into C, something like (this is completely untested):

diff --git a/bisect.c b/bisect.c
index 10f5e57..62786cf 100644
--- a/bisect.c
+++ b/bisect.c

@@ -876,6 +876,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 	opt.abbrev = 0;
 	opt.diff = 1;
+	opt.combine_merges = 1;
+	opt.dense_combined_merges = 1;
 
 	/* This is what "--pretty" does */
 	opt.verbose_header = 1;

@@ -884,7 +886,8 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 
 	/* diff-tree init */
 	if (!opt.diffopt.output_format)
-		opt.diffopt.output_format = DIFF_FORMAT_RAW;
+		opt.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;
+	DIFF_OPT_SET(&opt.diffopt, RECURSIVE);
 
 	log_tree_commit(&opt, commit);
 }