[PATCH v2 1/3] Add output_prefix_length to diff_options

[PATCH v2 1/3] Add output_prefix_length to diff_options

[Lucian Poston]

Add output_prefix_length to diff_options. Initialize the value to 0 and only
set it when graph.c:diff_output_prefix_callback() is called.

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 diff.h  |    1 +
 graph.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/diff.h b/diff.h
index cb68743..19d762f 100644
--- a/diff.h
+++ b/diff.h
@@ -150,6 +150,7 @@ struct diff_options {
 	diff_format_fn_t format_callback;
 	void *format_callback_data;
 	diff_prefix_fn_t output_prefix;
+	int output_prefix_length;
 	void *output_prefix_data;
 };
 
diff --git a/graph.c b/graph.c
index 7358416..7e0a099 100644
--- a/graph.c
+++ b/graph.c
@@ -194,8 +194,10 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
 	struct git_graph *graph = data;
 	static struct strbuf msgbuf = STRBUF_INIT;
 
+	assert(opt);
 	assert(graph);
 
+	opt->output_prefix_length = graph->width;
 	strbuf_reset(&msgbuf);
 	graph_padding_line(graph, &msgbuf);
 	return &msgbuf;
@@ -245,6 +247,7 @@ struct git_graph *graph_init(struct rev_info *opt)
 	 */
 	opt->diffopt.output_prefix = diff_output_prefix_callback;
 	opt->diffopt.output_prefix_data = graph;
+	opt->diffopt.output_prefix_length = 0;
 
 	return graph;
 }
-- 
1.7.3.4

[PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Lucian Poston]

The recent change to compute the width of diff --stat did not take into
consideration the output from --graph. The consequence is that when both
options are used, e.g. in 'log --stat --graph', the lines are too long.

Adjust stat width calculations to take --graph output into account.

Adjust stat width calculations to reserve space for required characters before
scaling the widths for the filename and graph portions of the diff-stat. For
example, consider:

" diff.c |   66 ++-"

Before calculating the widths allocated to the filename, "diff.c", and the
graph, "++-", reserve space for the initial " " and the part between the
filename and graph portions " |   66 ". Then, divide the remaining space so
that 5/8ths is given to the filename and 3/8ths for the graph.

Update the affected test, t4502.

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

diff --git a/diff.c b/diff.c
index 377ec1e..ed48480 100644
--- a/diff.c
+++ b/diff.c
@@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	int width, name_width, graph_width, number_width = 4, count;
 	const char *reset, *add_c, *del_c;
 	const char *line_prefix = "";
+	int line_prefix_length = 0;
+	int reserved_character_count;
 	int extra_shown = 0;
 	struct strbuf *msg = NULL;
 
@@ -1392,6 +1394,7 @@ 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_length = options->output_prefix_length;
 	}
 
 	count = options->stat_count ? options->stat_count : data->nr;
@@ -1427,22 +1430,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
-	 * 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).
+	 * If there's not enough space, we will use the smaller of stat_name_width
+	 * (if set) and 5/8*(width - reserved) for the filename, and the rest for
+	 * the graph part, but no more than stat_graph_width for the graph part.
+	 * Assuming the line prefix is empty, on a standard 80 column terminal
+	 * this ratio results in 44 characters for the filename and 26 characters
+	 * for the graph (plus the 10 reserved characters).
 	 *
 	 * 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 +1461,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 +1480,32 @@ 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) {
+		/*
+		 * Reduce graph_width to be at most 3/8 of the unreserved space.
+		 */
+		if (graph_width > (width - reserved_character_count) * 3/8) {
+			graph_width = (width - reserved_character_count) * 3/8;
+		}
+
+		/*
+		 * If the remaining unreserved space will not accomodate the
+		 * filenames, adjust name_width to use all available remaining space.
+		 * Otherwise, assign any extra space to graph_width.
+		 */
+		if (name_width > width - reserved_character_count - graph_width) {
+			name_width = width - reserved_character_count - graph_width;
+		} else {
+			graph_width = width - reserved_character_count - name_width;
+		}
+
+		/*
+		 * If stat-graph-width was specified, limit graph_width to its value.
+		 */
 		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;
-		else
-			graph_width = width - number_width - 6 - name_width;
+		}
 	}
 
 	/*
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 328aa8f..c95f120 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 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
-- 
1.7.3.4

[PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes

[Lucian Poston]

Add test to verify that the commit graph tree output is taken into
consideration when the diff stat output width is calculated.

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 t/t4052-stat-output.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index c95f120..84dd8bb 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -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 v2 2/3] Adjust stat width calculations to take --graph output into account

[Zbigniew Jędrzejewski-Szmek]

On 03/22/2012 08:27 PM, Lucian Poston wrote:
> The recent change to compute the width of diff --stat did not take into
> consideration the output from --graph. The consequence is that when both
> options are used, e.g. in 'log --stat --graph', the lines are too long.
>
> Adjust stat width calculations to take --graph output into account.
(1)
> Adjust stat width calculations to reserve space for required characters before
> scaling the widths for the filename and graph portions of the diff-stat. For
> example, consider:
>
> " diff.c |   66 ++-"
>
> Before calculating the widths allocated to the filename, "diff.c", and the
> graph, "++-", reserve space for the initial " " and the part between the
> filename and graph portions " |   66 ". Then, divide the remaining space so
> that 5/8ths is given to the filename and 3/8ths for the graph.
(2)

Hi,

I think that (1) is good. It fixes the bug and even makes the code more 
readable. But (2) should be separated, IMHO... There was a motivation 
for the layout in 1b058bc30df5f: not changing previous behaviour ("... 
at least 5/8 of available space is devoted to filenames. On a standard 
80 column terminal, or if not connected to a terminal and using the 
default of 80 columns, this gives the same partition as before.").
(2) would change the way format-patch --stat output looks, which 
probably is not wanted.

-
Zbyszek


> Update the affected test, t4502.
>
> Signed-off-by: Lucian Poston<lucian.poston@gmail.com>
> ---
>   diff.c                 |   66 ++++++++++++++++++++++++++++++++---------------
>   t/t4052-stat-output.sh |    4 +-
>   2 files changed, 47 insertions(+), 23 deletions(-)

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Junio C Hamano]

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

Administrivia: You seem to be CC'ing people who haven't touched any of
the surrounding code for quite some time, including the now-defunct
address of mine.  Please don't.

> Adjust stat width calculations to reserve space for required characters before
> scaling the widths for the filename and graph portions of the diff-stat. For
> example, consider:
>
> " diff.c |   66 ++-"
>
> Before calculating the widths allocated to the filename, "diff.c", and the
> graph, "++-", reserve space for the initial " " and the part between the
> filename and graph portions " |   66 ". Then, divide the remaining space so
> that 5/8ths is given to the filename and 3/8ths for the graph.
>
> Update the affected test, t4502.

That explains the regression you are introducing, but does not justify it.

When you start showing that line, do you already know how many columns at
the left edge of the display will be consumed by the ancestry graph part?

When the command is run without "--graph" option, the answer would
obviously be zero, but if it is non-zero, wouldn't it be a more sensible
solution to the problem to subtract that width from the total allowed
display width (e.g. on 200-column terminal, if the ancestry graph part at
the left edge uses 20-columns, you do exactly the same as the current
algorithm but use 180 as the width of the terminal).  When --stat-width is
explicitly given, that specifies the width of whatever comes after the
ancestry graph part, so there is no need to change anything.

Am I missing something, or is there something deeper going on?

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Lucian Poston]

2012/3/22 Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>:
> On 03/22/2012 08:27 PM, Lucian Poston wrote:
>>
>> The recent change to compute the width of diff --stat did not take into
>> consideration the output from --graph. The consequence is that when both
>> options are used, e.g. in 'log --stat --graph', the lines are too long.
>>
>> Adjust stat width calculations to take --graph output into account.
>
> (1)
>
>> Adjust stat width calculations to reserve space for required characters
>> before
>> scaling the widths for the filename and graph portions of the diff-stat.
>> For
>> example, consider:
>>
>> " diff.c |   66 ++-"
>>
>> Before calculating the widths allocated to the filename, "diff.c", and the
>> graph, "++-", reserve space for the initial " " and the part between the
>> filename and graph portions " |   66 ". Then, divide the remaining space
>> so
>> that 5/8ths is given to the filename and 3/8ths for the graph.
>
> (2)
>
> Hi,
>
> I think that (1) is good. It fixes the bug and even makes the code more
> readable. But (2) should be separated, IMHO... There was a motivation for
> the layout in 1b058bc30df5f: not changing previous behaviour ("... at least
> 5/8 of available space is devoted to filenames. On a standard 80 column
> terminal, or if not connected to a terminal and using the default of 80
> columns, this gives the same partition as before.").
> (2) would change the way format-patch --stat output looks, which probably is
> not wanted.

I suppose changing the format of format-patch --stat output could be
annoying to anyone expecting it to remain unchanged. I'll update the
patch so that the diff-stat output using the default of 80 columns
remains unmodified.

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Lucian Poston]

2012/3/22 Junio C Hamano <gitster@pobox.com>:
> That explains the regression you are introducing, but does not justify it.
>
> When you start showing that line, do you already know how many columns at
> the left edge of the display will be consumed by the ancestry graph part?
>
> When the command is run without "--graph" option, the answer would
> obviously be zero, but if it is non-zero, wouldn't it be a more sensible
> solution to the problem to subtract that width from the total allowed
> display width (e.g. on 200-column terminal, if the ancestry graph part at
> the left edge uses 20-columns, you do exactly the same as the current
> algorithm but use 180 as the width of the terminal).  When --stat-width is
> explicitly given, that specifies the width of whatever comes after the
> ancestry graph part, so there is no need to change anything.
>
> Am I missing something, or is there something deeper going on?

The approach you describe would work. The only issue is the current
calculations slightly run off the rails when the number of columns is
less than 26 or so (or, similarly and more frequently, when the
difference between the terminal columns and ancestry graph columns is
less than ~26). To keep the diff-stat output (more or less)
unmodified, I'll simply add a conditional to address this case, rather
than my more drastic approach.

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Lucian Poston]

The recent change to compute the width of diff --stat did not take into
consideration the output from --graph. The consequence is that when both
options are used, e.g. in 'log --stat --graph', the lines are too long.

Adjust stat width calculations to take --graph output into account.

Prevent graph width of diff-stat from falling below minimum.

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 diff.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/diff.c b/diff.c
index 377ec1e..31ba10c 100644
--- a/diff.c
+++ b/diff.c
@@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	int width, name_width, graph_width, number_width = 4, count;
 	const char *reset, *add_c, *del_c;
 	const char *line_prefix = "";
+	int line_prefix_length = 0;
+	int reserved_character_count;
 	int extra_shown = 0;
 	struct strbuf *msg = NULL;
 
@@ -1392,6 +1394,7 @@ 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_length = options->output_prefix_length;
 	}
 
 	count = options->stat_count ? options->stat_count : data->nr;
@@ -1427,37 +1430,46 @@ 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
+	 * Each line needs space for the following characters:
+	 *   - 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 + 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
-	 * 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).
+	 * Additionally, there may be a line_prefix, which reduces the available
+	 * width by line_prefix_length.
+	 *
+	 * 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 the graph
+	 * part, but no more than stat_graph_width for the graph part.
+	 * Assuming the line prefix is empty, on a standard 80 column terminal
+	 * this ratio results in 50 characters for the filename and 20 characters
+	 * for the graph (plus the 10 reserved characters).
 	 *
 	 * 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;
 
 	if (options->stat_width == -1)
 		width = term_columns();
 	else
 		width = options->stat_width ? options->stat_width : 80;
 
+	width -= line_prefix_length;
+
 	if (options->stat_graph_width == -1)
 		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 +1484,36 @@ 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) {
+		/*
+		 * Reduce graph_width to be at most 3/8 of the unreserved space and no
+		 * less than 6, which leaves at least 5/8 for the filename.
+		 */
+		if (graph_width > width * 3/8 - reserved_character_count) {
+			graph_width = width * 3/8 - reserved_character_count;
+			if (graph_width < 6) {
+				graph_width = 6;
+			}
+		}
+
+		/*
+		 * If the remaining unreserved space will not accomodate the
+		 * filenames, adjust name_width to use all available remaining space.
+		 * Otherwise, assign any extra space to graph_width.
+		 */
+		if (name_width > width - reserved_character_count - graph_width) {
+			name_width = width - reserved_character_count - graph_width;
+		} else {
+			graph_width = width - reserved_character_count - name_width;
+		}
+
+		/*
+		 * If stat-graph-width was specified, limit graph_width to its value.
+		 */
 		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;
-		else
-			graph_width = width - number_width - 6 - name_width;
+		}
 	}
 
 	/*
-- 
1.7.3.4

Re: [PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes

[Lucian Poston]

Add test to verify that the commit graph tree output is taken into
consideration when the diff stat output width is calculated.

Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
---
 t/t4052-stat-output.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 328aa8f..8b6e34a 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -198,6 +198,26 @@ respects expect200 show --stat
 respects expect200 log -1 --stat
 EOF
 
+cat >expect80graphed <<'EOF'
+|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++
+EOF
+cat >expect80 <<'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 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 v2 2/3] Adjust stat width calculations to take --graph output into account

[Zbigniew Jędrzejewski-Szmek]

On 03/23/2012 06:54 AM, Lucian Poston wrote:

> diff --git a/diff.c b/diff.c
> index 377ec1e..31ba10c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>   	int width, name_width, graph_width, number_width = 4, count;
>   	const char *reset, *add_c, *del_c;
>   	const char *line_prefix = "";
> +	int line_prefix_length = 0;
> +	int reserved_character_count;
>   	int extra_shown = 0;
>   	struct strbuf *msg = NULL;
> 
> @@ -1392,6 +1394,7 @@ 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_length = options->output_prefix_length;
>   	}
Hi,
line_prefix will only be used once. And options->output_prefix_length will
be 0 if !options->output_prefix, so line_prefix variable can be eliminated
and options->output_prefix_length used directly instead.

>   	count = options->stat_count ? options->stat_count : data->nr;
> @@ -1427,37 +1430,46 @@ 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
> +	 * Each line needs space for the following characters:
> +	 *   - 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 + 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
> -	 * 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).
> +	 * Additionally, there may be a line_prefix, which reduces the available
> +	 * width by line_prefix_length.
> +	 *
> +	 * 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 the graph
> +	 * part, but no more than stat_graph_width for the graph part.
> +	 * Assuming the line prefix is empty, on a standard 80 column terminal
> +	 * this ratio results in 50 characters for the filename and 20 characters
> +	 * for the graph (plus the 10 reserved characters).
>   	 *
>   	 * 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;
> 
>   	if (options->stat_width == -1)
>   		width = term_columns();
>   	else
>   		width = options->stat_width ? options->stat_width : 80;
> 
> +	width -= line_prefix_length;
> +
>   	if (options->stat_graph_width == -1)
>   		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 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>   	/*
>   	 * Adjust adjustable widths not to exceed maximum width
>   	 */

In this part below, you add gratuitous braces around single line if-blocks.
This makes the code (and the diff) longer with no gain.

> -	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) {
> +		/*
> +		 * Reduce graph_width to be at most 3/8 of the unreserved space and no
> +		 * less than 6, which leaves at least 5/8 for the filename.
> +		 */
> +		if (graph_width>  width * 3/8 - reserved_character_count) {
> +			graph_width = width * 3/8 - reserved_character_count;
> +			if (graph_width<  6) {
> +				graph_width = 6;
> +			}
> +		}
This extra test is not necessary. Above, after /* Guarantees at least 6 characters
for the graph part [16 * 3/8] ... */, this should already by so that
(width * 3/8 - reserved_character_count) is at least 6.

> +
> +		/*
> +		 * If the remaining unreserved space will not accomodate the
> +		 * filenames, adjust name_width to use all available remaining space.
> +		 * Otherwise, assign any extra space to graph_width.
> +		 */
> +		if (name_width>  width - reserved_character_count - graph_width) {
> +			name_width = width - reserved_character_count - graph_width;
> +		} else {
> +			graph_width = width - reserved_character_count - name_width;
> +		}
> +
> +		/*
> +		 * If stat-graph-width was specified, limit graph_width to its value.
> +		 */
>   		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;
> -		else
> -			graph_width = width - number_width - 6 - name_width;
> +		}
Here, the order of the two tests
(1) if (options->stat_graph_width && graph_width > options->stat_graph_width)
(2) if (name_width > width - number_width - 6 - graph_width)
is reversed. This is not OK, because this means that
options->stat_graph_width will be used unconditionally, while
before it was subject to limiting by total width.

>   	}
> 
>   	/*

The tests:
After the new tests are added, I see:

ok 53 - format-patch ignores COLUMNS (long filename)
ok 54 - diff respects COLUMNS (long filename)
ok 55 - show respects COLUMNS (long filename)
ok 56 - log respects COLUMNS (long filename)
ok 57 - show respects 80 COLUMNS (long filename)  <=======
ok 58 - log respects 80 COLUMNS (long filename)   <-------
ok 59 - show respects 80 COLUMNS (long filename)  <=======
ok 60 - log respects 80 COLUMNS (long filename)   <-------

So some tests descriptions are duplicated. Also it would be
nice to test with --graph in more places. I'm attaching a
replacement patch which adds more tests. It should go *before*
your series, and your series should  tweak the tests to pass,
showing what changed.

------- 8< --------
From 348d96dd9ae4a4ffd04aea4497b237a794e37727 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Fri, 23 Mar 2012 11:04:21 +0100
Subject: [PATCH] t4052: test --stat output with --graph
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Add tests which show that the width of the --prefix added by --graph
is not taken into consideration when the diff stat output width is
calculated.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 t/t4052-stat-output.sh |   78 +++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 328aa8f..da14984 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -82,11 +82,15 @@ test_expect_success 'preparation for big change tests' '
 cat >expect80 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
-
+cat >expect80-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+EOF
 cat >expect200 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
-
+cat >expect200-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (big change)" '
@@ -94,6 +98,14 @@ do
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
+		COLUMNS=200 git $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
 done <<\EOF
 ignores expect80 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
@@ -104,7 +116,9 @@ EOF
 cat >expect40 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++
 EOF
-
+cat >expect40-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++
+EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb not enough COLUMNS (big change)" '
@@ -118,6 +132,20 @@ do
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
+		COLUMNS=40 git $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
+
+	test_expect_success "$cmd --graph $verb statGraphWidth config" '
+		git -c diff.statGraphWidth=26 $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
 done <<\EOF
 ignores expect80 format-patch -1 --stdout
 respects expect40 diff HEAD^ HEAD --stat
@@ -129,6 +157,9 @@ EOF
 cat >expect <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++
 EOF
+cat >expect-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++
+EOF
 while read cmd args
 do
 	test_expect_success "$cmd --stat=width with big change" '
@@ -143,11 +174,25 @@ do
 		test_cmp expect actual
 	'
 
-	test_expect_success "$cmd --stat-graph--width with big change" '
+	test_expect_success "$cmd --stat-graph-width with big change" '
 		git $cmd $args --stat-graph-width=26 >output
 		grep " | " output >actual &&
 		test_cmp expect actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --stat-width=width --graph with big change" '
+		git $cmd $args --stat-width=40 --graph >output
+		grep " | " output >actual &&
+		test_cmp expect-graph actual
+	'
+
+	test_expect_success "$cmd --stat-graph-width --graph with big change" '
+		git $cmd $args --stat-graph-width=26 --graph >output
+		grep " | " output >actual &&
+		test_cmp expect-graph actual
+	'
 done <<\EOF
 format-patch -1 --stdout
 diff HEAD^ HEAD --stat
@@ -164,6 +209,9 @@ test_expect_success 'preparation for long filename tests' '
 cat >expect <<'EOF'
  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
 EOF
+cat >expect-graph <<'EOF'
+|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++
+EOF
 while read cmd args
 do
 	test_expect_success "$cmd --stat=width with big change is more balanced" '
@@ -171,6 +219,14 @@ do
 		grep " | " output >actual &&
 		test_cmp expect actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --stat=width --graph with big change is balanced" '
+		git $cmd $args --stat-width=60 --graph >output &&
+		grep " | " output >actual &&
+		test_cmp expect-graph actual
+	'
 done <<\EOF
 format-patch -1 --stdout
 diff HEAD^ HEAD --stat
@@ -181,9 +237,15 @@ EOF
 cat >expect80 <<'EOF'
  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
 EOF
+cat >expect80-graph <<'EOF'
+|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
+EOF
 cat >expect200 <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
+cat >expect200-graph <<'EOF'
+|  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+EOF
 while read verb expect cmd args
 do
 	test_expect_success "$cmd $verb COLUMNS (long filename)" '
@@ -191,6 +253,14 @@ do
 		grep " | " output >actual &&
 		test_cmp "$expect" actual
 	'
+
+	test "$cmd" != diff || continue
+
+	test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
+		COLUMNS=200 git $cmd $args --graph >output
+		grep " | " output >actual &&
+		test_cmp "$expect-graph" actual
+	'
 done <<\EOF
 ignores expect80 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
-- 
1.7.10.rc1.225.gba57e


------- >8 --------

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Junio C Hamano]

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

> The recent change to compute the width of diff --stat did not take into
> consideration the output from --graph. The consequence is that when both
> options are used, e.g. in 'log --stat --graph', the lines are too long.
>
> Adjust stat width calculations to take --graph output into account.
>
> Prevent graph width of diff-stat from falling below minimum.
>
> Signed-off-by: Lucian Poston <lucian.poston@gmail.com>
> ---
>  diff.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 377ec1e..31ba10c 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  	int width, name_width, graph_width, number_width = 4, count;
>  	const char *reset, *add_c, *del_c;
>  	const char *line_prefix = "";
> +	int line_prefix_length = 0;
> +	int reserved_character_count;
>  	int extra_shown = 0;
>  	struct strbuf *msg = NULL;
>  
> @@ -1392,6 +1394,7 @@ 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_length = options->output_prefix_length;
>  	}
>  
>  	count = options->stat_count ? options->stat_count : data->nr;
> @@ -1427,37 +1430,46 @@ 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
> +	 * Each line needs space for the following characters:
> +	 *   - 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 + 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
> -	 * 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).
> +	 * Additionally, there may be a line_prefix, which reduces the available
> +	 * width by line_prefix_length.
> +	 *
> +	 * 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 the graph
> +	 * part, but no more than stat_graph_width for the graph part.
> +	 * Assuming the line prefix is empty, on a standard 80 column terminal
> +	 * this ratio results in 50 characters for the filename and 20 characters
> +	 * for the graph (plus the 10 reserved characters).

Please do not do reflowing of the text in the same patch as modifying the
logic.  It is unreadable for the purpose of finding out what you really
changed.

>  	 *
>  	 * 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;

As far as I can tell, this introduces a variable that is set (and is meant
to be set) only at a single place, namely, here, and used throughout the
rest of the function. But it invites later patches to mistakenly update
the variable.  I do not see the merit of it.

If you wanted to have a symbolic name for (6+number_width), #define would
have served better.

Also as we see in the later part of the review, this name is probably way
too long to be useful.  We need a shorter and sweeter name to call it.

>  	if (options->stat_width == -1)
>  		width = term_columns();
>  	else
>  		width = options->stat_width ? options->stat_width : 80;
>  
> +	width -= line_prefix_length;
> +

Isn't this wrong?  This is not a rhetoric question, iow, I am not
declaring that this is wrong --- I just cannot see why the above is a good
change, as I do not see a sound reasoning behind it.

When the user said "--stat-width=80", she means that the diffstat part
(name and bargraph) is to extend 80 places, and she does not expect it to
be reduced by the width of the ancestry graph.  If the user wanted to clip
the entire width, she would have used COLUMNS=80 instead.

Am I missing something?

> @@ -1472,16 +1484,36 @@ 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) {
> +		/*
> +		 * Reduce graph_width to be at most 3/8 of the unreserved space and no
> +		 * less than 6, which leaves at least 5/8 for the filename.
> +		 */
> +		if (graph_width > width * 3/8 - reserved_character_count) {
> +			graph_width = width * 3/8 - reserved_character_count;
> +			if (graph_width < 6) {
> +				graph_width = 6;
> +			}
> +		}

What is this about?  reserved_character_count already knows about the
magic number 6 and here you have another magic number 6.  How are they
related with each other?

In other words, shouldn't the added code be more like this?

	if (graph_width < reserved_character_count - number_width)
		graph_width = reserved_character_count - number_width;

> +		/*
> +		 * If the remaining unreserved space will not accomodate the
> +		 * filenames, adjust name_width to use all available remaining space.
> +		 * Otherwise, assign any extra space to graph_width.
> +		 */
> +		if (name_width > width - reserved_character_count - graph_width) {
> +			name_width = width - reserved_character_count - graph_width;
> +		} else {
> +			graph_width = width - reserved_character_count - name_width;
> +		}
> +		/*
> +		 * If stat-graph-width was specified, limit graph_width to its value.
> +		 */
>  		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;
> -		else
> -			graph_width = width - number_width - 6 - name_width;
> +		}
>  	}

Yikes. It took me three minutes to realize that the only thing you did in
this hunk was to move the "if stat-graph-width is set" logic down.  Not
your fault, but this is one of the times I wish our diff generation logic
matched "corresponding" block of lines better.

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Lucian Poston]

On Fri, Mar 23, 2012 at 03:12, Zbigniew Jędrzejewski-Szmek
<zbyszek@in.waw.pl> wrote:
> On 03/23/2012 06:54 AM, Lucian Poston wrote:
>
>> diff --git a/diff.c b/diff.c
>> index 377ec1e..31ba10c 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>       int width, name_width, graph_width, number_width = 4, count;
>>       const char *reset, *add_c, *del_c;
>>       const char *line_prefix = "";
>> +     int line_prefix_length = 0;
>> +     int reserved_character_count;
>>       int extra_shown = 0;
>>       struct strbuf *msg = NULL;
>>
>> @@ -1392,6 +1394,7 @@ 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_length = options->output_prefix_length;
>>       }
> Hi,
> line_prefix will only be used once. And options->output_prefix_length will
> be 0 if !options->output_prefix, so line_prefix variable can be eliminated
> and options->output_prefix_length used directly instead.

Rather than adding the line_prefix_length variable, the next patch
will use options->output_prefix_length directly.

>>       count = options->stat_count ? options->stat_count : data->nr;
>> @@ -1427,37 +1430,46 @@ 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
>> +      * Each line needs space for the following characters:
>> +      *   - 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 + 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
>> -      * 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).
>> +      * Additionally, there may be a line_prefix, which reduces the available
>> +      * width by line_prefix_length.
>> +      *
>> +      * 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 the graph
>> +      * part, but no more than stat_graph_width for the graph part.
>> +      * Assuming the line prefix is empty, on a standard 80 column terminal
>> +      * this ratio results in 50 characters for the filename and 20 characters
>> +      * for the graph (plus the 10 reserved characters).
>>        *
>>        * 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;
>>
>>       if (options->stat_width == -1)
>>               width = term_columns();
>>       else
>>               width = options->stat_width ? options->stat_width : 80;
>>
>> +     width -= line_prefix_length;
>> +
>>       if (options->stat_graph_width == -1)
>>               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 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>       /*
>>        * Adjust adjustable widths not to exceed maximum width
>>        */
>
> In this part below, you add gratuitous braces around single line if-blocks.
> This makes the code (and the diff) longer with no gain.

I prefer gratuitous braces, particularly when conditionals are nested
as they are here. It helps later when maintaining the code if someone
wants to add a debug statement or comment out a line.

I'll remove braces from single line conditionals to keep with the
existing conventions.

>> -     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) {
>> +             /*
>> +              * Reduce graph_width to be at most 3/8 of the unreserved space and no
>> +              * less than 6, which leaves at least 5/8 for the filename.
>> +              */
>> +             if (graph_width>  width * 3/8 - reserved_character_count) {
>> +                     graph_width = width * 3/8 - reserved_character_count;
>> +                     if (graph_width<  6) {
>> +                             graph_width = 6;
>> +                     }
>> +             }
> This extra test is not necessary. Above, after /* Guarantees at least 6 characters
> for the graph part [16 * 3/8] ... */, this should already by so that
> (width * 3/8 - reserved_character_count) is at least 6.

Ahh, this is because the calculations go haywire when the number of
columns is small. I briefly mentioned it here:
http://thread.gmane.org/gmane.comp.version-control.git/193694/focus=193744

graph_width actually can have a negative value under certain
conditions, and this check compensates for that edge case. My earlier
patches took a less conservative approach and adjusted the
calculations so that the value of graph_width would be at least 6, but
it caused several tests to regress. Since the intention of the
original graph_width calculation was place a lower bound of 6 on its
value, I simply enforce that here without affecting the general cases,
which will remain unmodified in order to prevent test regressions.

>> +
>> +             /*
>> +              * If the remaining unreserved space will not accomodate the
>> +              * filenames, adjust name_width to use all available remaining space.
>> +              * Otherwise, assign any extra space to graph_width.
>> +              */
>> +             if (name_width>  width - reserved_character_count - graph_width) {
>> +                     name_width = width - reserved_character_count - graph_width;
>> +             } else {
>> +                     graph_width = width - reserved_character_count - name_width;
>> +             }
>> +
>> +             /*
>> +              * If stat-graph-width was specified, limit graph_width to its value.
>> +              */
>>               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;
>> -             else
>> -                     graph_width = width - number_width - 6 - name_width;
>> +             }
> Here, the order of the two tests
> (1) if (options->stat_graph_width && graph_width > options->stat_graph_width)
> (2) if (name_width > width - number_width - 6 - graph_width)
> is reversed. This is not OK, because this means that
> options->stat_graph_width will be used unconditionally, while
> before it was subject to limiting by total width.

If options->stat_graph_width is specified, it should always limit the
value of graph_width, correct? Since (1) is the last test, it can only
decrease the value of graph_width, which would already be limited by
the total width.

I just noticed that name_width isn't being limited to stat_name_width,
if it is specified. I'll add a check for that.

> The tests:
> After the new tests are added, I see:
>
> ok 53 - format-patch ignores COLUMNS (long filename)
> ok 54 - diff respects COLUMNS (long filename)
> ok 55 - show respects COLUMNS (long filename)
> ok 56 - log respects COLUMNS (long filename)
> ok 57 - show respects 80 COLUMNS (long filename)  <=======
> ok 58 - log respects 80 COLUMNS (long filename)   <-------
> ok 59 - show respects 80 COLUMNS (long filename)  <=======
> ok 60 - log respects 80 COLUMNS (long filename)   <-------
>
> So some tests descriptions are duplicated. Also it would be
> nice to test with --graph in more places. I'm attaching a
> replacement patch which adds more tests. It should go *before*
> your series, and your series should  tweak the tests to pass,
> showing what changed.

Thanks, I'll add these.

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Lucian Poston]

On Fri, Mar 23, 2012 at 11:13, Junio C Hamano <gitster@pobox.com> wrote:
> Please do not do reflowing of the text in the same patch as modifying the
> logic.  It is unreadable for the purpose of finding out what you really
> changed.

I will un-reflow the text. :]

>
>>        *
>>        * 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;
>
> As far as I can tell, this introduces a variable that is set (and is meant
> to be set) only at a single place, namely, here, and used throughout the
> rest of the function. But it invites later patches to mistakenly update
> the variable.  I do not see the merit of it.
>
> If you wanted to have a symbolic name for (6+number_width), #define would
> have served better.
>
> Also as we see in the later part of the review, this name is probably way
> too long to be useful.  We need a shorter and sweeter name to call it.

I'll remove it.

>>       if (options->stat_width == -1)
>>               width = term_columns();
>>       else
>>               width = options->stat_width ? options->stat_width : 80;
>>
>> +     width -= line_prefix_length;
>> +
>
> Isn't this wrong?  This is not a rhetoric question, iow, I am not
> declaring that this is wrong --- I just cannot see why the above is a good
> change, as I do not see a sound reasoning behind it.
>
> When the user said "--stat-width=80", she means that the diffstat part
> (name and bargraph) is to extend 80 places, and she does not expect it to
> be reduced by the width of the ancestry graph.  If the user wanted to clip
> the entire width, she would have used COLUMNS=80 instead.
>
> Am I missing something?

You're right, the prefix length shouldn't be subtracted when
--stat-width is specified.

>> @@ -1472,16 +1484,36 @@ 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) {
>> +             /*
>> +              * Reduce graph_width to be at most 3/8 of the unreserved space and no
>> +              * less than 6, which leaves at least 5/8 for the filename.
>> +              */
>> +             if (graph_width > width * 3/8 - reserved_character_count) {
>> +                     graph_width = width * 3/8 - reserved_character_count;
>> +                     if (graph_width < 6) {
>> +                             graph_width = 6;
>> +                     }
>> +             }
>
> What is this about?  reserved_character_count already knows about the
> magic number 6 and here you have another magic number 6.  How are they
> related with each other?
>
> In other words, shouldn't the added code be more like this?
>
>        if (graph_width < reserved_character_count - number_width)
>                graph_width = reserved_character_count - number_width;

There are two magic number 6's. From previous comments that explain
how reserved_character_count is calculated:

        * Each line needs space for the following characters:
        *   - 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 + decimal_width(max_change).

In the case of deriving reserved_character_count, 6 arises because 1+4+1.

The second magic number 6 is the minimum value for graph_width. The
intention of the original stat width calculation was to give the
filename portion 5/8ths of the total width and give the graph portion
3/8ths of the total width. With 80 columns, that works out to 50 for
filename and 30 for the graph (plus reserved characters). With 16
columns, that works out to be 10 and 6. I assume 5 and 3 would be too
small, so 10 and 6 were probably chosen as the minimum values by the
previous author(s).

So now you ask why I added the if (graph_width < 6) conditional?

> +             if (graph_width > width * 3/8 - reserved_character_count) {
> +                     graph_width = width * 3/8 - reserved_character_count;
> +                     if (graph_width < 6) {
> +                             graph_width = 6;
> +                     }
> +             }

The calculation in the graph_width assignment (and the prior
conditional) does not guarantee graph_width is at least 6. The
calculation should be ((width - reserved_character_count) * 3/8),
instead of (width * 3/8 - reserved_character_count). But as we saw in
my initial patch, adjusting this calculation causes test regressions.
Therefore, I added a conditional to catch the edge case where
graph_width is less than 6.

Assuming the $COLUMNS is 26 or less, graph_width will actually come
out to -1, iirc.

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Zbigniew Jędrzejewski-Szmek]

On 04/12/2012 09:47 AM, Lucian Poston wrote:
> On Fri, Mar 23, 2012 at 03:12, Zbigniew Jędrzejewski-Szmek
> <zbyszek@in.waw.pl>  wrote:
>> On 03/23/2012 06:54 AM, Lucian Poston wrote:
>>
>>> diff --git a/diff.c b/diff.c
>>> index 377ec1e..31ba10c 100644
>>> --- a/diff.c
>>> +++ b/diff.c
>>> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>>        int width, name_width, graph_width, number_width = 4, count;
>>>        const char *reset, *add_c, *del_c;
>>>        const char *line_prefix = "";
>>> +     int line_prefix_length = 0;
>>> +     int reserved_character_count;
>>>        int extra_shown = 0;
>>>        struct strbuf *msg = NULL;
>>>
>>> @@ -1392,6 +1394,7 @@ 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_length = options->output_prefix_length;
>>>        }
>> Hi,
>> line_prefix will only be used once. And options->output_prefix_length will
>> be 0 if !options->output_prefix, so line_prefix variable can be eliminated
>> and options->output_prefix_length used directly instead.
>
> Rather than adding the line_prefix_length variable, the next patch
> will use options->output_prefix_length directly.
>
>>>        count = options->stat_count ? options->stat_count : data->nr;
>>> @@ -1427,37 +1430,46 @@ 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
>>> +      * Each line needs space for the following characters:
>>> +      *   - 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 + 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
>>> -      * 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).
>>> +      * Additionally, there may be a line_prefix, which reduces the available
>>> +      * width by line_prefix_length.
>>> +      *
>>> +      * 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 the graph
>>> +      * part, but no more than stat_graph_width for the graph part.
>>> +      * Assuming the line prefix is empty, on a standard 80 column terminal
>>> +      * this ratio results in 50 characters for the filename and 20 characters
>>> +      * for the graph (plus the 10 reserved characters).
>>>         *
>>>         * 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;
>>>
>>>        if (options->stat_width == -1)
>>>                width = term_columns();
>>>        else
>>>                width = options->stat_width ? options->stat_width : 80;
>>>
>>> +     width -= line_prefix_length;
>>> +
>>>        if (options->stat_graph_width == -1)
>>>                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 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>>>        /*
>>>         * Adjust adjustable widths not to exceed maximum width
>>>         */
>>
>> In this part below, you add gratuitous braces around single line if-blocks.
>> This makes the code (and the diff) longer with no gain.
>
> I prefer gratuitous braces, particularly when conditionals are nested
> as they are here. It helps later when maintaining the code if someone
> wants to add a debug statement or comment out a line.
>
> I'll remove braces from single line conditionals to keep with the
> existing conventions.
Thanks.

>>> -     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) {
>>> +             /*
>>> +              * Reduce graph_width to be at most 3/8 of the unreserved space and no
>>> +              * less than 6, which leaves at least 5/8 for the filename.
>>> +              */
>>> +             if (graph_width>    width * 3/8 - reserved_character_count) {
>>> +                     graph_width = width * 3/8 - reserved_character_count;
>>> +                     if (graph_width<    6) {
>>> +                             graph_width = 6;
>>> +                     }
>>> +             }
>> This extra test is not necessary. Above, after /* Guarantees at least 6 characters
>> for the graph part [16 * 3/8] ... */, this should already by so that
>> (width * 3/8 - reserved_character_count) is at least 6.
>
> Ahh, this is because the calculations go haywire when the number of
> columns is small. I briefly mentioned it here:
> http://thread.gmane.org/gmane.comp.version-control.git/193694/focus=193744
>
> graph_width actually can have a negative value under certain
> conditions, and this check compensates for that edge case. My earlier
> patches took a less conservative approach and adjusted the
> calculations so that the value of graph_width would be at least 6, but
> it caused several tests to regress. Since the intention of the
> original graph_width calculation was place a lower bound of 6 on its
> value, I simply enforce that here without affecting the general cases,
> which will remain unmodified in order to prevent test regressions.
>
>>> +
>>> +             /*
>>> +              * If the remaining unreserved space will not accomodate the
>>> +              * filenames, adjust name_width to use all available remaining space.
>>> +              * Otherwise, assign any extra space to graph_width.
>>> +              */
>>> +             if (name_width>    width - reserved_character_count - graph_width) {
>>> +                     name_width = width - reserved_character_count - graph_width;
>>> +             } else {
>>> +                     graph_width = width - reserved_character_count - name_width;
>>> +             }
>>> +
>>> +             /*
>>> +              * If stat-graph-width was specified, limit graph_width to its value.
>>> +              */
>>>                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;
>>> -             else
>>> -                     graph_width = width - number_width - 6 - name_width;
>>> +             }
>> Here, the order of the two tests
>> (1) if (options->stat_graph_width&&  graph_width>  options->stat_graph_width)
>> (2) if (name_width>  width - number_width - 6 - graph_width)
>> is reversed. This is not OK, because this means that
>> options->stat_graph_width will be used unconditionally, while
>> before it was subject to limiting by total width.
>
> If options->stat_graph_width is specified, it should always limit the
> value of graph_width, correct? Since (1) is the last test, it can only
> decrease the value of graph_width, which would already be limited by
> the total width.
Right, but the way the tests are ordered now, we could end up decreasing
name_width first (after (2)) and then graph_width (after (1)), actually
using less than full width.

> I just noticed that name_width isn't being limited to stat_name_width,
> if it is specified. I'll add a check for that.
Sounds good.

>> The tests:
>> After the new tests are added, I see:
>>
>> ok 53 - format-patch ignores COLUMNS (long filename)
>> ok 54 - diff respects COLUMNS (long filename)
>> ok 55 - show respects COLUMNS (long filename)
>> ok 56 - log respects COLUMNS (long filename)
>> ok 57 - show respects 80 COLUMNS (long filename)<=======
>> ok 58 - log respects 80 COLUMNS (long filename)<-------
>> ok 59 - show respects 80 COLUMNS (long filename)<=======
>> ok 60 - log respects 80 COLUMNS (long filename)<-------
>>
>> So some tests descriptions are duplicated. Also it would be
>> nice to test with --graph in more places. I'm attaching a
>> replacement patch which adds more tests. It should go *before*
>> your series, and your series should  tweak the tests to pass,
>> showing what changed.
>
> Thanks, I'll add these.

Regards,
Zbyszek

Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account

[Lucian Poston]

On Thu, Apr 12, 2012 at 03:17, Zbigniew Jędrzejewski-Szmek
<zbyszek@in.waw.pl> wrote:
>>>> +
>>>> +             /*
>>>> +              * If the remaining unreserved space will not accomodate
>>>> the
>>>> +              * filenames, adjust name_width to use all available
>>>> remaining space.
>>>> +              * Otherwise, assign any extra space to graph_width.
>>>> +              */
>>>> +             if (name_width>    width - reserved_character_count -
>>>> graph_width) {
>>>> +                     name_width = width - reserved_character_count -
>>>> graph_width;
>>>> +             } else {
>>>> +                     graph_width = width - reserved_character_count -
>>>> name_width;
>>>> +             }
>>>> +
>>>> +             /*
>>>> +              * If stat-graph-width was specified, limit graph_width to
>>>> its value.
>>>> +              */
>>>>               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;
>>>> -             else
>>>> -                     graph_width = width - number_width - 6 -
>>>> name_width;
>>>> +             }
>>>
>>> Here, the order of the two tests
>>> (1) if (options->stat_graph_width&&  graph_width>
>>>  options->stat_graph_width)
>>>
>>> (2) if (name_width>  width - number_width - 6 - graph_width)
>>> is reversed. This is not OK, because this means that
>>> options->stat_graph_width will be used unconditionally, while
>>> before it was subject to limiting by total width.
>>
>>
>> If options->stat_graph_width is specified, it should always limit the
>> value of graph_width, correct? Since (1) is the last test, it can only
>> decrease the value of graph_width, which would already be limited by
>> the total width.
>
> Right, but the way the tests are ordered now, we could end up decreasing
> name_width first (after (2)) and then graph_width (after (1)), actually
> using less than full width.

Ahh, I didn't think about that.

I just reverted the order back to the original.

>> I just noticed that name_width isn't being limited to stat_name_width,
>> if it is specified. I'll add a check for that.
>
> Sounds good.

FYI, in patch v3, I reverted the check for stat_graph_width as previously
mentioned, and I ended up not adding a check for stat_name_width.
It remains the case that in certain scenarios, name_width & graph_width
could be set to values greater than stat_name_width & stat_graph_width.