[PATCH 0/1] Adjust diff stat width calculations so lines do not wrap in terminal when using --graph

[PATCH 0/1] Adjust diff stat width calculations so lines do not wrap in terminal when using --graph

[Lucian Poston]

When diff stats are displayed in the terminal, the width is scaled to fit
within the available $COLUMNS. The current stat width calculations do not
factor in the diff output prefix that is displayed when when graphing the
commit history e.g. `git log --graph --stat`. Consequently, the diff stats can
wrap to next line breaking the text graph representation.

This patch adjusts the diff stat width calculations to prevent line wrapping
when the text-based graph representation is displayed along with the diff
stats.


Lucian Poston (1):
  Fix --stat width calculations to handle --graph

 diff.c                 |   55 ++++++++++++++++++++++++++++++++---------------
 t/t4052-stat-output.sh |   24 +++++++++++++++++++-
 2 files changed, 59 insertions(+), 20 deletions(-)

-- 
1.7.3.4

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

[Lucian Poston]

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

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 diff.c                 |   55 ++++++++++++++++++++++++++++++++---------------
 t/t4052-stat-output.sh |   24 +++++++++++++++++++-
 2 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/diff.c b/diff.c
index 377ec1e..3a26561 100644
--- a/diff.c
+++ b/diff.c
@@ -1382,7 +1382,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	int total_files = data->nr;
 	int width, name_width, graph_width, number_width = 4, count;
 	const char *reset, *add_c, *del_c;
-	const char *line_prefix = "";
+	const char *line_prefix = "", *line_prefix_iter;
+	unsigned int line_prefix_length = 0;
+	unsigned int reserved_character_count;
 	int extra_shown = 0;
 	struct strbuf *msg = NULL;
 
@@ -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;
+		}
 	}
 
 	count = options->stat_count ? options->stat_count : data->nr;
@@ -1427,22 +1441,27 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	 * We have width = stat_width or term_columns() columns total.
 	 * We want a maximum of min(max_len, stat_name_width) for the name part.
 	 * We want a maximum of min(max_change, stat_graph_width) for the +- part.
-	 * We also need 1 for " " and 4 + decimal_width(max_change)
-	 * for " | NNNN " and one the empty column at the end, altogether
-	 * 6 + decimal_width(max_change).
+	 * Each line needs space for the following characters:
+	 *   - line_prefix_length for the line_prefix
+	 *   - 1 for the initial " "
+	 *   - 4 + decimal_width(max_change) for " | NNNN "
+	 *   - 1 for the empty column at the end,
+	 * Altogether, the reserved_character_count totals
+	 * 6 + line_prefix_length + decimal_width(max_change).
 	 *
 	 * If there's not enough space, we will use the smaller of
 	 * stat_name_width (if set) and 5/8*width for the filename,
-	 * and the rest for constant elements + graph part, but no more
+	 * and the rest for reserved characters + graph part, but no more
 	 * than stat_graph_width for the graph part.
-	 * (5/8 gives 50 for filename and 30 for the constant parts + graph
-	 * for the standard terminal size).
+	 * (5/8 gives 50 for filename and 30 for the reserved characters + graph
+	 * for the standard terminal size assuming there is no line prefix).
 	 *
 	 * In other words: stat_width limits the maximum width, and
 	 * stat_name_width fixes the maximum width of the filename,
 	 * and is also used to divide available columns if there
 	 * aren't enough.
 	 */
+	reserved_character_count = 6 + number_width + line_prefix_length;
 
 	if (options->stat_width == -1)
 		width = term_columns();
@@ -1453,11 +1472,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		options->stat_graph_width = diff_stat_graph_width;
 
 	/*
-	 * Guarantee 3/8*16==6 for the graph part
-	 * and 5/8*16==10 for the filename part
+	 * Guarantees at least 6 characters for the graph part [16 * 3/8]
+	 * and at least 10 for the filename part [16 * 5/8]
 	 */
-	if (width < 16 + 6 + number_width)
-		width = 16 + 6 + number_width;
+	if (width < 16 + reserved_character_count)
+		width = 16 + reserved_character_count;
 
 	/*
 	 * First assign sizes that are wanted, ignoring available width.
@@ -1472,16 +1491,16 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	/*
 	 * Adjust adjustable widths not to exceed maximum width
 	 */
-	if (name_width + number_width + 6 + graph_width > width) {
-		if (graph_width > width * 3/8 - number_width - 6)
-			graph_width = width * 3/8 - number_width - 6;
+	if (reserved_character_count + name_width + graph_width > width) {
+		if (graph_width > (width - reserved_character_count) * 3/8)
+			graph_width = (width - reserved_character_count) * 3/8;
 		if (options->stat_graph_width &&
-		    graph_width > options->stat_graph_width)
+				graph_width > options->stat_graph_width)
 			graph_width = options->stat_graph_width;
-		if (name_width > width - number_width - 6 - graph_width)
-			name_width = width - number_width - 6 - graph_width;
+		if (name_width > width - reserved_character_count - graph_width)
+			name_width = width - reserved_character_count - graph_width;
 		else
-			graph_width = width - number_width - 6 - name_width;
+			graph_width = width - reserved_character_count - name_width;
 	}
 
 	/*
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
 while read cmd args
 do
@@ -179,7 +179,7 @@ log -1 --stat
 EOF
 
 cat >expect80 <<'EOF'
- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++
 EOF
 cat >expect200 <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
@@ -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
-- 
1.7.3.4

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

[Johannes Schindelin]

Hi Lucian,

On Tue, 20 Mar 2012, Lucian Poston wrote:

> 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
> 
> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
> ---

Good. Just a quick question before everything else: are the commit
messages cut off/wrapped to the same number of columns? If so, where do
they get the indent from? (Sorry for asking, but I figured that you're
already deep in the code so you might know of the top of your head.)

> diff --git a/diff.c b/diff.c
> index 377ec1e..3a26561 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1382,7 +1382,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  	int total_files = data->nr;
>  	int width, name_width, graph_width, number_width = 4, count;
>  	const char *reset, *add_c, *del_c;
> -	const char *line_prefix = "";
> +	const char *line_prefix = "", *line_prefix_iter;
> +	unsigned int line_prefix_length = 0;
> +	unsigned int reserved_character_count;
>  	int extra_shown = 0;
>  	struct strbuf *msg = NULL;
>  
> @@ -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;
> +		}
>  	}

My 1st reaction was: why is the current indent width not stored in the options?
But you're right, the indent is generated dynamically from output_prefix()
which is a method of diff_options, so there is little chance to do it
differently from your solution.

However, a little nit, since this list is so famous for "just a little
nit": I'd prefer to factor-out the indent width measuring, like so:

static int count_pipes_and_spaces(const char *string)
{
	int count;

	for (count = 0; *string; string++)
		if (*string == '|' || *string == ' ')
			count++;

	return count;
}

It's not only that that new function cannot mess with the local variables
of show_stats(), it also documents a bit better what the code is supposed
to do (and all that without a single /* ... */! Isn't that fab? ;)

As for the complete patch: nicely done. I especially like that it is
minimally intrusive and that you took great care of updating the comments
-- not something everybody does!

My nits aside: this is good to go.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> However, a little nit, since this list is so famous for "just a little
> nit": I'd prefer to factor-out the indent width measuring, like so:
>
> static int count_pipes_and_spaces(const char *string)
> {
> 	int count;
>
> 	for (count = 0; *string; string++)
> 		if (*string == '|' || *string == ' ')
> 			count++;
>
> 	return count;
> }
>

I agree that this is much better than the original by Lucian, but if we
were to go this route, I would prefer to see it *not* count pipes and
spaces, but actually measure the display width of the string.  Both the
name of the function and the implementation would have to change, of
course.

Even though I didn't look very closely, I do not think it should be too
hard for graph.c to tell the diff_options structure how wide a prefix it
placed in the output_prefix, so use of such a "display_columns()" function
would be wasteful for this particular case, but for a more general case,
it would come in handy as a helper function, and at that point, this
should not hide in diff.c as a static function.

Thanks.

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

[Lucian Poston]

2012/3/20 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Good. Just a quick question before everything else: are the commit
> messages cut off/wrapped to the same number of columns? If so, where do
> they get the indent from? (Sorry for asking, but I figured that you're
> already deep in the code so you might know of the top of your head.)

Thanks for reviewing the patch! The commit part of git log is handled
in log-tree.c:show_log().

> My 1st reaction was: why is the current indent width not stored in the options?

The new patch adds a output_prefix_length field to struct diff_options.

Thanks!
Lucian

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

[Junio C Hamano]

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

> 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. ...

> @@ -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.

> 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?

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

Likewise.

> @@ -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

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

[Lucian Poston]

2012/3/20 Junio C Hamano <gitster@pobox.com>:
> 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. ...

Thanks for letting me know. Patch v2 has updated log messages. Let me
know whether they meet the conventions.

> 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.

Added outout_prefix_length to struct diff_options in patch v2.

>> 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?

It is, and I didn't make that clear in the log message. In patch v2,
the log message describes what has changed to in the calculation to
cause this.

Thanks!
Lucian