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

STALE3715d

4 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Lucian Poston <hidden>
Date: 2016-06-15 22:53:20

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

From: Lucian Poston <hidden>
Date: 2016-06-15 22:53:20

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

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:53:20

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 <redacted>
---
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.)
quoted hunk
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

From: Lucian Poston <hidden>
Date: 2016-06-15 22:53:22

2012/3/20 Johannes Schindelin [off-list ref]:
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help