Re: [PATCH] blame: reserve mark column only if necessary

[Junio C Hamano <gitster@pobox.com>]

René Scharfe <l.s.r@web.de> writes:

>  `--abbrev=<n>`::
> +	Instead of using the default _7_ hexadecimal digits as the
> +	abbreviated object name, use at least _<n>_ digits, but ensure
> +	the commit object names are unique.
> +	If commits marked with caret (boundary), question mark (ignored)
> +	or asterisk (unblamable) are shown, extend unmarked object names
> +	to align them.

OK.

> +static inline int maybe_putc(int c, FILE *out)
> +{
> +	return out ? putc(c, out) : 0;
> +}
> +
> +static size_t print_marks(FILE *out, const struct blame_entry *ent, int opt)
> +{
> +	size_t len = 0;
> +
> +	if ((ent->suspect->commit->object.flags & UNINTERESTING) &&
> +	    !blank_boundary && !(opt & OUTPUT_ANNOTATE_COMPAT)) {
> +		maybe_putc('^', out);
> +		len++;
> +	}
> +	if (mark_unblamable_lines && ent->unblamable) {
> +		maybe_putc('*', out);
> +		len++;
> +	}
> +	if (mark_ignored_lines && ent->ignored) {
> +		maybe_putc('?', out);
> +		len++;
> +	}
> +	return len;
> +}

Quite straight-forward.

> +static size_t count_marks(const struct blame_entry *ent, int opt)
> +{
> +	return print_marks(NULL, ent, opt);
> +}

OK.

> @@ -499,23 +529,10 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent,
>  		if (color)
>  			fputs(color, stdout);
>  
> -		if (suspect->commit->object.flags & UNINTERESTING) {
> -			if (blank_boundary) {
> -				memset(hex, ' ', strlen(hex));
> -			} else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
> -				length--;
> -				putchar('^');
> -			}
> -		}
> -
> -		if (mark_unblamable_lines && ent->unblamable) {
> -			length--;
> -			putchar('*');
> -		}
> -		if (mark_ignored_lines && ent->ignored) {
> -			length--;
> -			putchar('?');
> -		}
> +		if ((suspect->commit->object.flags & UNINTERESTING) &&
> +		    blank_boundary)
> +			memset(hex, ' ', strlen(hex));
> +		length -= print_marks(stdout, ent, opt);
>  
>  		printf("%.*s", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);
>  		if (opt & OUTPUT_ANNOTATE_COMPAT) {
> @@ -647,11 +664,15 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
>  	struct blame_entry *e;
>  	int compute_auto_abbrev = (abbrev < 0);
>  	int auto_abbrev = DEFAULT_ABBREV;
> +	size_t max_marks_count = 0;
>  
>  	for (e = sb->ent; e; e = e->next) {
>  		struct blame_origin *suspect = e->suspect;
>  		int num;
> +		size_t marks_count = count_marks(e, *option);
>  
> +		if (max_marks_count < marks_count)
> +			max_marks_count = marks_count;
>  		if (compute_auto_abbrev)
>  			auto_abbrev = update_auto_abbrev(auto_abbrev, suspect);
>  		if (strcmp(suspect->path, sb->path))
> @@ -685,8 +706,12 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
>  	max_score_digits = decimal_width(largest_score);
>  
>  	if (compute_auto_abbrev)
> -		/* one more abbrev length is needed for the boundary commit */
> -		abbrev = auto_abbrev + 1;
> +		abbrev = auto_abbrev;
> +	if (abbrev < (int)the_hash_algo->hexsz) {
> +		abbrev += max_marks_count;
> +		if (abbrev > (int)the_hash_algo->hexsz)
> +			abbrev = the_hash_algo->hexsz;
> +	}
>  }
>  
>  static void sanity_check_on_fail(struct blame_scoreboard *sb, int baa)
> @@ -1047,10 +1072,7 @@ int cmd_blame(int argc,
>  	} else if (show_progress < 0)
>  		show_progress = isatty(2);
>  
> -	if (0 < abbrev && abbrev < (int)the_hash_algo->hexsz)
> -		/* one more abbrev length is needed for the boundary commit */
> -		abbrev++;
> -	else if (!abbrev)
> +	if (!abbrev)
>  		abbrev = the_hash_algo->hexsz;

OK.


> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
> index 7822947f028..bf04b8273ef 100755
> --- a/t/t8002-blame.sh
> +++ b/t/t8002-blame.sh
> @@ -113,8 +113,7 @@ test_expect_success 'set up abbrev tests' '
>  '
>  
>  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 &&
>  	check_abbrev 30 --abbrev=30 ^HEAD
>  '
>  
> @@ -141,10 +140,8 @@ test_expect_success 'blame --abbrev gets truncated with boundary commit' '
>  '
>  
>  test_expect_success 'blame --abbrev -b truncates the blank boundary' '
> -	# Note that `--abbrev=` always gets incremented by 1, which is why we
> -	# expect 11 leading spaces and not 10.
>  	cat >expect <<-EOF &&
> -	$(printf "%11s" "") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev
> +	$(printf "%10s" "") (<author@example.com> 2005-04-07 15:45:13 -0700 1) abbrev
>  	EOF

OK.

>  	git blame -b --abbrev=10 ^HEAD -- abbrev.t >actual &&
>  	test_cmp expect actual