blame: fix alignment with --abbrev=40

The blame command internally adds 1 to any requested sha1
abbreviation length, and then subtracts it when outputting a
boundary commit. This lets regular and boundary sha1s line
up visually, but it misses one corner case.

When the requested length is 40, we bump the value to 41.
But since we only have 40 characters, that's all we can show
(fortunately the truncation is done by a printf precision
field, so it never tries to read past the end of the
buffer).  So a normal sha1 shows 40 hex characters, and a
boundary sha1 shows "^" plus 40 hex characters. The result
is misaligned.

The "-l" option to show long sha1s gets around this by
skipping the "abbrev" variable entirely and just always
using GIT_SHA1_HEXSZ.  This avoids the "+1" issue, but it
does mean that boundary commits only have 39 characters
printed.  This is somewhat odd, but it does look good
visually: the results are aligned and left-justified. The
alternative would be to allocate an extra column that would
contain either an extra space or the "^" boundary marker.

As this is by definition the human-readable view, it's
probably not that big a deal either way (and of course
--porcelain, etc, correctly produce correct 40-hex sha1s).
But for consistency, this patch teaches --abbrev=40 to
produce the same output as "-l" (always left-aligned, with
40-hex for normal sha1s, and "^" plus 39-hex for
boundaries).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/blame.c b/builtin/blame.c
index f618392e55f70966accc49aa7ff22de2d16f189f..cbb7dc2ad9e9ce1504a412cf13480fb90d31e6b7 100644 (file)
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2606,7 +2606,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
        } else if (show_progress < 0)
                show_progress = isatty(2);
 
-       if (0 < abbrev)
+       if (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)
                /* one more abbrev length is needed for the boundary commit */
                abbrev++;
 

diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index ab79de95441f4f474525b48b14313f1d04172fce..c6347ad8fde4fec09d96269ef8de4bcdd3bea275 100755 (executable)
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -86,4 +86,32 @@ test_expect_success 'blame with showEmail config true' '
        test_cmp expected_n result
 '
 
+test_expect_success 'set up abbrev tests' '
+       test_commit abbrev &&
+       sha1=$(git rev-parse --verify HEAD) &&
+       check_abbrev () {
+               expect=$1; shift
+               echo $sha1 | cut -c 1-$expect >expect &&
+               git blame "$@" abbrev.t >actual &&
+               perl -lne "/[0-9a-f]+/ and print \$&" <actual >actual.sha &&
+               test_cmp expect actual.sha
+       }
+'
+
+test_expect_success 'blame --abbrev=<n> works' '
+       # non-boundary commits get +1 for alignment
+       check_abbrev 31 --abbrev=30 HEAD &&
+       check_abbrev 30 --abbrev=30 ^HEAD
+'
+
+test_expect_success 'blame -l aligns regular and boundary commits' '
+       check_abbrev 40 -l HEAD &&
+       check_abbrev 39 -l ^HEAD
+'
+
+test_expect_success 'blame --abbrev=40 behaves like -l' '
+       check_abbrev 40 --abbrev=40 HEAD &&
+       check_abbrev 39 --abbrev=40 ^HEAD
+'
+
 test_done