Re: [PATCH 1/1] Fix --stat width calculations to handle --graph

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2012, 17:09 UTC
Message-ID: <7vehsn6vy1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1332229097-19262-2-git-send-email-lucian.poston@gmail.com>

Lucian Poston <lucian.poston@gmail.com> writes:

Show 12 quoted lines

> Adjusted stat width calculations to take into consideration the diff output
> prefix e.g. the graph prefix generated by `git log --graph --stat`.
>
> This change fixes the line wrapping that occurs when diff stats are large
> enough to be scaled to fit within the terminal's columns. This issue only
> appears when using --stat and --graph together on large diffs.
>
> Adjusted stat output tests accordingly. The scaled output tests are closer to
> the target 5:3 ratio.
>
> Added test that verifies the output of --stat --graph is truncated to fit
> within the available terminal $COLUMNS

Thanks.

Regarding the log message:

 - Please start it with a problem description. Describe both what the
   current code shows, and why you think it is wrong or suboptimal.
   I.e. the observation of the problem in your second paragraph comes at
   the beginning

 - Our log message usually gives an order to the codebase or to the person
   who is applying the patch in order to address the problem you described
   in the earlier part of the log message, instead of tells a story of
   what happened in the past.

E.g.

    The recent change to compute the width of diff --stat based on the
    terminal width did not take the width needed to show the --graph
    output into account, and makes lines in "log --graph --stat" too long.
   
    Adjust stat width calculation to take the width of graph prefix into
    account. ...

Show 16 quoted lines

> @@ -1392,6 +1394,18 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  	if (options->output_prefix) {
>  		msg = options->output_prefix(options, options->output_prefix_data);
>  		line_prefix = msg->buf;
> +
> +		/*
> +		 * line_prefix can contain color codes, so only pipes '|' and
> +		 * spaces ' ' are counted.
> +		 */
> +		line_prefix_iter = line_prefix;
> +		while (*line_prefix_iter != '\0') {
> +			if (*line_prefix_iter == ' ' || *line_prefix_iter == '|') {
> +				line_prefix_length++;
> +			}
> +			line_prefix_iter += 1;
> +		}

Yikes.

This code relies on "Count only ' ' and '|', because these are the only ones we currently happen to use", which is fragile. The next person who will update graph.c can change the set of letters used in the graph to improve the output without even knowing your code exists or the assumption your code makes, so she is likely not going to update it.

I think the caller should be taught to pass the exact width it carves out of the available width for use by the ancestry graph output, and if we are to do so, adding "int output_prefix_len" field (which usually is 0) to diff_options, and seting it in graph.c::diff_output_prefix_callback() (at that point, graph->width has the number you want, I think), may be the way to go.

Show 11 quoted lines

> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> index 328aa8f..84dd8bb 100755
> --- a/t/t4052-stat-output.sh
> +++ b/t/t4052-stat-output.sh
> @@ -162,7 +162,7 @@ test_expect_success 'preparation for long filename tests' '
>  '
>  
>  cat >expect <<'EOF'
> - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++
>  EOF

Isn't it a sign that the change is doing a lot more than justified that it has to change the test vector for cases where --graph is *NOT* involved at all?

Show 6 quoted lines

> @@ -179,7 +179,7 @@ log -1 --stat
>  EOF
>  
>  cat >expect80 <<'EOF'
> - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++

Likewise.

Show 27 quoted lines

> @@ -198,6 +198,26 @@ respects expect200 show --stat
>  respects expect200 log -1 --stat
>  EOF
>  
> +cat >expect80graphed <<'EOF'
> +|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++
> +EOF
> +cat >expect80 <<'EOF'
> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++
> +EOF
> +while read verb expect cmd args
> +do
> +	test_expect_success "$cmd $verb 80 COLUMNS (long filename)" '
> +		COLUMNS=80 git $cmd $args >output
> +		grep " | " output >actual &&
> +		test_cmp "$expect" actual
> +	'
> +done <<\EOF
> +respects expect80 show --stat
> +respects expect80 log -1 --stat
> +respects expect80graphed show --stat --graph
> +respects expect80graphed log -1 --stat --graph
> +EOF
> +
>  cat >expect <<'EOF'
>   abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  EOF