* [PATCH v2 1/3] Add output_prefix_length to diff_options @ 2012-03-22 19:27 Lucian Poston 2012-03-22 19:27 ` [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account Lucian Poston 2012-03-22 19:27 ` [PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes Lucian Poston 0 siblings, 2 replies; 15+ messages in thread From: Lucian Poston @ 2012-03-22 19:27 UTC (permalink / raw) To: git; +Cc: Lucian Poston, Adam Simpkins, Jeff King, Junio C Hamano, Bo Yang 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-22 19:27 [PATCH v2 1/3] Add output_prefix_length to diff_options Lucian Poston @ 2012-03-22 19:27 ` Lucian Poston 2012-03-22 20:28 ` Zbigniew Jędrzejewski-Szmek ` (2 more replies) 2012-03-22 19:27 ` [PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes Lucian Poston 1 sibling, 3 replies; 15+ messages in thread From: Lucian Poston @ 2012-03-22 19:27 UTC (permalink / raw) To: git Cc: Lucian Poston, Johannes Schindelin, Junio C Hamano, Michael J Gruber, Junio C Hamano, Bo Yang, Zbigniew Jędrzejewski-Szmek 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-22 19:27 ` [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account Lucian Poston @ 2012-03-22 20:28 ` Zbigniew Jędrzejewski-Szmek 2012-03-23 4:38 ` Lucian Poston 2012-03-22 20:45 ` Junio C Hamano 2012-03-23 5:54 ` Lucian Poston 2 siblings, 1 reply; 15+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-22 20:28 UTC (permalink / raw) To: Lucian Poston Cc: git, Johannes Schindelin, Junio C Hamano, Michael J Gruber, Junio C Hamano, Bo Yang 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(-) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-22 20:28 ` Zbigniew Jędrzejewski-Szmek @ 2012-03-23 4:38 ` Lucian Poston 0 siblings, 0 replies; 15+ messages in thread From: Lucian Poston @ 2012-03-23 4:38 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek Cc: git, Johannes Schindelin, Michael J Gruber, Junio C Hamano, Bo Yang 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-22 19:27 ` [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account Lucian Poston 2012-03-22 20:28 ` Zbigniew Jędrzejewski-Szmek @ 2012-03-22 20:45 ` Junio C Hamano 2012-03-23 4:44 ` Lucian Poston 2012-03-23 5:54 ` Lucian Poston 2 siblings, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2012-03-22 20:45 UTC (permalink / raw) To: Lucian Poston Cc: git, Johannes Schindelin, Michael J Gruber, Bo Yang, Zbigniew Jędrzejewski-Szmek 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? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-22 20:45 ` Junio C Hamano @ 2012-03-23 4:44 ` Lucian Poston 0 siblings, 0 replies; 15+ messages in thread From: Lucian Poston @ 2012-03-23 4:44 UTC (permalink / raw) To: Junio C Hamano Cc: git, Johannes Schindelin, Michael J Gruber, Bo Yang, Zbigniew Jędrzejewski-Szmek 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-22 19:27 ` [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account Lucian Poston 2012-03-22 20:28 ` Zbigniew Jędrzejewski-Szmek 2012-03-22 20:45 ` Junio C Hamano @ 2012-03-23 5:54 ` Lucian Poston 2012-03-23 10:12 ` Zbigniew Jędrzejewski-Szmek 2012-03-23 18:13 ` Junio C Hamano 2 siblings, 2 replies; 15+ messages in thread From: Lucian Poston @ 2012-03-23 5:54 UTC (permalink / raw) To: git; +Cc: Lucian Poston, Junio C Hamano, Zbigniew Jędrzejewski-Szmek 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-23 5:54 ` Lucian Poston @ 2012-03-23 10:12 ` Zbigniew Jędrzejewski-Szmek 2012-04-12 7:47 ` Lucian Poston 2012-03-23 18:13 ` Junio C Hamano 1 sibling, 1 reply; 15+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-03-23 10:12 UTC (permalink / raw) To: Lucian Poston; +Cc: git, Junio C Hamano 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 -------- ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-23 10:12 ` Zbigniew Jędrzejewski-Szmek @ 2012-04-12 7:47 ` Lucian Poston 2012-04-12 10:17 ` Zbigniew Jędrzejewski-Szmek 0 siblings, 1 reply; 15+ messages in thread From: Lucian Poston @ 2012-04-12 7:47 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-04-12 7:47 ` Lucian Poston @ 2012-04-12 10:17 ` Zbigniew Jędrzejewski-Szmek 2012-04-16 11:04 ` Lucian Poston 0 siblings, 1 reply; 15+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-04-12 10:17 UTC (permalink / raw) To: Lucian Poston; +Cc: git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-04-12 10:17 ` Zbigniew Jędrzejewski-Szmek @ 2012-04-16 11:04 ` Lucian Poston 0 siblings, 0 replies; 15+ messages in thread From: Lucian Poston @ 2012-04-16 11:04 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-23 5:54 ` Lucian Poston 2012-03-23 10:12 ` Zbigniew Jędrzejewski-Szmek @ 2012-03-23 18:13 ` Junio C Hamano 2012-04-12 8:35 ` Lucian Poston 1 sibling, 1 reply; 15+ messages in thread From: Junio C Hamano @ 2012-03-23 18:13 UTC (permalink / raw) To: Lucian Poston; +Cc: git, Zbigniew Jędrzejewski-Szmek 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account 2012-03-23 18:13 ` Junio C Hamano @ 2012-04-12 8:35 ` Lucian Poston 0 siblings, 0 replies; 15+ messages in thread From: Lucian Poston @ 2012-04-12 8:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Zbigniew Jędrzejewski-Szmek 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes 2012-03-22 19:27 [PATCH v2 1/3] Add output_prefix_length to diff_options Lucian Poston 2012-03-22 19:27 ` [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account Lucian Poston @ 2012-03-22 19:27 ` Lucian Poston 2012-03-23 5:57 ` Lucian Poston 1 sibling, 1 reply; 15+ messages in thread From: Lucian Poston @ 2012-03-22 19:27 UTC (permalink / raw) To: git; +Cc: Lucian Poston, Junio C Hamano, Zbigniew Jędrzejewski-Szmek 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes 2012-03-22 19:27 ` [PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes Lucian Poston @ 2012-03-23 5:57 ` Lucian Poston 0 siblings, 0 replies; 15+ messages in thread From: Lucian Poston @ 2012-03-23 5:57 UTC (permalink / raw) To: git; +Cc: Lucian Poston, Junio C Hamano, Zbigniew Jędrzejewski-Szmek 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-04-16 11:04 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-03-22 19:27 [PATCH v2 1/3] Add output_prefix_length to diff_options Lucian Poston 2012-03-22 19:27 ` [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account Lucian Poston 2012-03-22 20:28 ` Zbigniew Jędrzejewski-Szmek 2012-03-23 4:38 ` Lucian Poston 2012-03-22 20:45 ` Junio C Hamano 2012-03-23 4:44 ` Lucian Poston 2012-03-23 5:54 ` Lucian Poston 2012-03-23 10:12 ` Zbigniew Jędrzejewski-Szmek 2012-04-12 7:47 ` Lucian Poston 2012-04-12 10:17 ` Zbigniew Jędrzejewski-Szmek 2012-04-16 11:04 ` Lucian Poston 2012-03-23 18:13 ` Junio C Hamano 2012-04-12 8:35 ` Lucian Poston 2012-03-22 19:27 ` [PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes Lucian Poston 2012-03-23 5:57 ` Lucian Poston
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.