{"thread":{"id":"30028","subject":"[PATCH v2 1/3] Add output_prefix_length to diff_options","startedAt":"2012-03-22T19:27:39Z","lastAt":"2012-04-16T11:04:38Z","messageCount":15,"participants":["Lucian Poston","Zbigniew Jędrzejewski-Szmek","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"187505","messageId":"1332444461-11957-1-git-send-email-lucian.poston@gmail.com","threadId":"30028","inReplyTo":null,"subject":"[PATCH v2 1/3] Add output_prefix_length to diff_options","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-22T19:27:39Z","receivedAt":"2012-03-22T19:27:39Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"Add output_prefix_length to diff_options. Initialize the value to 0 and only\nset it when graph.c:diff_output_prefix_callback() is called.\n\nSigned-off-by: Lucian Poston \n---\n diff.h | 1 +\n graph.c | 3 +++\n 2 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.h b/diff.h\nindex cb68743..19d762f 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -150,6 +150,7 @@ struct diff_options {\n \tdiff_format_fn_t format_callback;\n \tvoid *format_callback_data;\n \tdiff_prefix_fn_t output_prefix;\n+\tint output_prefix_length;\n \tvoid *output_prefix_data;\n };\n \ndiff --git a/graph.c b/graph.c\nindex 7358416..7e0a099 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -194,8 +194,10 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void\n \tstruct git_graph *graph = data;\n \tstatic struct strbuf msgbuf = STRBUF_INIT;\n \n+\tassert(opt);\n \tassert(graph);\n \n+\topt->output_prefix_length = graph->width;\n \tstrbuf_reset(&msgbuf);\n \tgraph_padding_line(graph, &msgbuf);\n \treturn &msgbuf;\n@@ -245,6 +247,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n \t */\n \topt->diffopt.output_prefix = diff_output_prefix_callback;\n \topt->diffopt.output_prefix_data = graph;\n+\topt->diffopt.output_prefix_length = 0;\n \n \treturn graph;\n }\n-- \n1.7.3.4\n"},{"id":"187507","messageId":"1332444461-11957-2-git-send-email-lucian.poston@gmail.com","threadId":"30028","inReplyTo":"1332444461-11957-1-git-send-email-lucian.poston@gmail.com","subject":"[PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-22T19:27:40Z","receivedAt":"2012-03-22T19:27:40Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"The recent change to compute the width of diff --stat did not take into\nconsideration the output from --graph. The consequence is that when both\noptions are used, e.g. in 'log --stat --graph', the lines are too long.\n\nAdjust stat width calculations to take --graph output into account.\n\nAdjust stat width calculations to reserve space for required characters before\nscaling the widths for the filename and graph portions of the diff-stat. For\nexample, consider:\n\n\" diff.c | 66 ++-\"\n\nBefore calculating the widths allocated to the filename, \"diff.c\", and the\ngraph, \"++-\", reserve space for the initial \" \" and the part between the\nfilename and graph portions \" | 66 \". Then, divide the remaining space so\nthat 5/8ths is given to the filename and 3/8ths for the graph.\n\nUpdate the affected test, t4502.\n\nSigned-off-by: Lucian Poston \n---\n diff.c | 66 ++++++++++++++++++++++++++++++++---------------\n t/t4052-stat-output.sh | 4 +-\n 2 files changed, 47 insertions(+), 23 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 377ec1e..ed48480 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tint width, name_width, graph_width, number_width = 4, count;\n \tconst char *reset, *add_c, *del_c;\n \tconst char *line_prefix = \"\";\n+\tint line_prefix_length = 0;\n+\tint reserved_character_count;\n \tint extra_shown = 0;\n \tstruct strbuf *msg = NULL;\n \n@@ -1392,6 +1394,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tif (options->output_prefix) {\n \t\tmsg = options->output_prefix(options, options->output_prefix_data);\n \t\tline_prefix = msg->buf;\n+\t\tline_prefix_length = options->output_prefix_length;\n \t}\n \n \tcount = options->stat_count ? options->stat_count : data->nr;\n@@ -1427,22 +1430,27 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t * We have width = stat_width or term_columns() columns total.\n \t * We want a maximum of min(max_len, stat_name_width) for the name part.\n \t * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n-\t * We also need 1 for \" \" and 4 + decimal_width(max_change)\n-\t * for \" | NNNN \" and one the empty column at the end, altogether\n-\t * 6 + decimal_width(max_change).\n+\t * Each line needs space for the following characters:\n+\t * - line_prefix_length for the line_prefix\n+\t * - 1 for the initial \" \"\n+\t * - 4 + decimal_width(max_change) for \" | NNNN \"\n+\t * - 1 for the empty column at the end,\n+\t * Altogether, the reserved_character_count totals\n+\t * 6 + line_prefix_length + decimal_width(max_change).\n \t *\n-\t * If there's not enough space, we will use the smaller of\n-\t * stat_name_width (if set) and 5/8*width for the filename,\n-\t * and the rest for constant elements + graph part, but no more\n-\t * than stat_graph_width for the graph part.\n-\t * (5/8 gives 50 for filename and 30 for the constant parts + graph\n-\t * for the standard terminal size).\n+\t * If there's not enough space, we will use the smaller of stat_name_width\n+\t * (if set) and 5/8*(width - reserved) for the filename, and the rest for\n+\t * the graph part, but no more than stat_graph_width for the graph part.\n+\t * Assuming the line prefix is empty, on a standard 80 column terminal\n+\t * this ratio results in 44 characters for the filename and 26 characters\n+\t * for the graph (plus the 10 reserved characters).\n \t *\n \t * In other words: stat_width limits the maximum width, and\n \t * stat_name_width fixes the maximum width of the filename,\n \t * and is also used to divide available columns if there\n \t * aren't enough.\n \t */\n+\treserved_character_count = 6 + number_width + line_prefix_length;\n \n \tif (options->stat_width == -1)\n \t\twidth = term_columns();\n@@ -1453,11 +1461,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\toptions->stat_graph_width = diff_stat_graph_width;\n \n \t/*\n-\t * Guarantee 3/8*16==6 for the graph part\n-\t * and 5/8*16==10 for the filename part\n+\t * Guarantees at least 6 characters for the graph part [16 * 3/8]\n+\t * and at least 10 for the filename part [16 * 5/8]\n \t */\n-\tif (width < 16 + 6 + number_width)\n-\t\twidth = 16 + 6 + number_width;\n+\tif (width < 16 + reserved_character_count)\n+\t\twidth = 16 + reserved_character_count;\n \n \t/*\n \t * First assign sizes that are wanted, ignoring available width.\n@@ -1472,16 +1480,32 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t/*\n \t * Adjust adjustable widths not to exceed maximum width\n \t */\n-\tif (name_width + number_width + 6 + graph_width > width) {\n-\t\tif (graph_width > width * 3/8 - number_width - 6)\n-\t\t\tgraph_width = width * 3/8 - number_width - 6;\n+\tif (reserved_character_count + name_width + graph_width > width) {\n+\t\t/*\n+\t\t * Reduce graph_width to be at most 3/8 of the unreserved space.\n+\t\t */\n+\t\tif (graph_width > (width - reserved_character_count) * 3/8) {\n+\t\t\tgraph_width = (width - reserved_character_count) * 3/8;\n+\t\t}\n+\n+\t\t/*\n+\t\t * If the remaining unreserved space will not accomodate the\n+\t\t * filenames, adjust name_width to use all available remaining space.\n+\t\t * Otherwise, assign any extra space to graph_width.\n+\t\t */\n+\t\tif (name_width > width - reserved_character_count - graph_width) {\n+\t\t\tname_width = width - reserved_character_count - graph_width;\n+\t\t} else {\n+\t\t\tgraph_width = width - reserved_character_count - name_width;\n+\t\t}\n+\n+\t\t/*\n+\t\t * If stat-graph-width was specified, limit graph_width to its value.\n+\t\t */\n \t\tif (options->stat_graph_width &&\n-\t\t graph_width > options->stat_graph_width)\n+\t\t\t\tgraph_width > options->stat_graph_width) {\n \t\t\tgraph_width = options->stat_graph_width;\n-\t\tif (name_width > width - number_width - 6 - graph_width)\n-\t\t\tname_width = width - number_width - 6 - graph_width;\n-\t\telse\n-\t\t\tgraph_width = width - number_width - 6 - name_width;\n+\t\t}\n \t}\n \n \t/*\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 328aa8f..c95f120 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -162,7 +162,7 @@ test_expect_success 'preparation for long filename tests' '\n '\n \n cat >expect <<'EOF'\n- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++\n EOF\n while read cmd args\n do\n@@ -179,7 +179,7 @@ log -1 --stat\n EOF\n \n cat >expect80 <<'EOF'\n- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++\n EOF\n cat >expect200 <<'EOF'\n aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n-- \n1.7.3.4\n"},{"id":"187506","messageId":"1332444461-11957-3-git-send-email-lucian.poston@gmail.com","threadId":"30028","inReplyTo":"1332444461-11957-1-git-send-email-lucian.poston@gmail.com","subject":"[PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-22T19:27:41Z","receivedAt":"2012-03-22T19:27:41Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"Add test to verify that the commit graph tree output is taken into\nconsideration when the diff stat output width is calculated.\n\nSigned-off-by: Lucian Poston \n---\n t/t4052-stat-output.sh | 20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex c95f120..84dd8bb 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -198,6 +198,26 @@ respects expect200 show --stat\n respects expect200 log -1 --stat\n EOF\n \n+cat >expect80graphed <<'EOF'\n+| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++\n+EOF\n+cat >expect80 <<'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++\n+EOF\n+while read verb expect cmd args\n+do\n+\ttest_expect_success \"$cmd $verb 80 COLUMNS (long filename)\" '\n+\t\tCOLUMNS=80 git $cmd $args >output\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp \"$expect\" actual\n+\t'\n+done <<\\EOF\n+respects expect80 show --stat\n+respects expect80 log -1 --stat\n+respects expect80graphed show --stat --graph\n+respects expect80graphed log -1 --stat --graph\n+EOF\n+\n cat >expect <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-- \n1.7.3.4\n"},{"id":"187521","messageId":"4F6B8B59.4010106@in.waw.pl","threadId":"30028","inReplyTo":"1332444461-11957-2-git-send-email-lucian.poston@gmail.com","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-22T20:28:09Z","receivedAt":"2012-03-22T20:28:09Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 03/22/2012 08:27 PM, Lucian Poston wrote:\n> The recent change to compute the width of diff --stat did not take into\n> consideration the output from --graph. The consequence is that when both\n> options are used, e.g. in 'log --stat --graph', the lines are too long.\n>\n> Adjust stat width calculations to take --graph output into account.\n(1)\n> Adjust stat width calculations to reserve space for required characters before\n> scaling the widths for the filename and graph portions of the diff-stat. For\n> example, consider:\n>\n> \" diff.c | 66 ++-\"\n>\n> Before calculating the widths allocated to the filename, \"diff.c\", and the\n> graph, \"++-\", reserve space for the initial \" \" and the part between the\n> filename and graph portions \" | 66 \". Then, divide the remaining space so\n> that 5/8ths is given to the filename and 3/8ths for the graph.\n(2)\n\nHi,\n\nI think that (1) is good. It fixes the bug and even makes the code more \nreadable. But (2) should be separated, IMHO... There was a motivation \nfor the layout in 1b058bc30df5f: not changing previous behaviour (\"... \nat least 5/8 of available space is devoted to filenames. On a standard \n80 column terminal, or if not connected to a terminal and using the \ndefault of 80 columns, this gives the same partition as before.\").\n(2) would change the way format-patch --stat output looks, which \nprobably is not wanted.\n\n-\nZbyszek\n\n\n> Update the affected test, t4502.\n>\n> Signed-off-by: Lucian Poston\n> ---\n> diff.c | 66 ++++++++++++++++++++++++++++++++---------------\n> t/t4052-stat-output.sh | 4 +-\n> 2 files changed, 47 insertions(+), 23 deletions(-)\n"},{"id":"187526","messageId":"7vd384wejl.fsf@alter.siamese.dyndns.org","threadId":"30028","inReplyTo":"1332444461-11957-2-git-send-email-lucian.poston@gmail.com","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-22T20:45:18Z","receivedAt":"2012-03-22T20:45:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucian Poston writes:\n\nAdministrivia: You seem to be CC'ing people who haven't touched any of\nthe surrounding code for quite some time, including the now-defunct\naddress of mine. Please don't.\n\n> Adjust stat width calculations to reserve space for required characters before\n> scaling the widths for the filename and graph portions of the diff-stat. For\n> example, consider:\n>\n> \" diff.c | 66 ++-\"\n>\n> Before calculating the widths allocated to the filename, \"diff.c\", and the\n> graph, \"++-\", reserve space for the initial \" \" and the part between the\n> filename and graph portions \" | 66 \". Then, divide the remaining space so\n> that 5/8ths is given to the filename and 3/8ths for the graph.\n>\n> Update the affected test, t4502.\n\nThat explains the regression you are introducing, but does not justify it.\n\nWhen you start showing that line, do you already know how many columns at\nthe left edge of the display will be consumed by the ancestry graph part?\n\nWhen the command is run without \"--graph\" option, the answer would\nobviously be zero, but if it is non-zero, wouldn't it be a more sensible\nsolution to the problem to subtract that width from the total allowed\ndisplay width (e.g. on 200-column terminal, if the ancestry graph part at\nthe left edge uses 20-columns, you do exactly the same as the current\nalgorithm but use 180 as the width of the terminal). When --stat-width is\nexplicitly given, that specifies the width of whatever comes after the\nancestry graph part, so there is no need to change anything.\n\nAm I missing something, or is there something deeper going on?\n"},{"id":"187554","messageId":"CACz_eycFU564bz1aO6-QF3=6GV8oHvGYfMWHRfgT1-j9AcAX-g@mail.gmail.com","threadId":"30028","inReplyTo":"4F6B8B59.4010106@in.waw.pl","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-23T04:38:54Z","receivedAt":"2012-03-23T04:38:54Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"2012/3/22 Zbigniew Jędrzejewski-Szmek :\n> On 03/22/2012 08:27 PM, Lucian Poston wrote:\n>>\n>> The recent change to compute the width of diff --stat did not take into\n>> consideration the output from --graph. The consequence is that when both\n>> options are used, e.g. in 'log --stat --graph', the lines are too long.\n>>\n>> Adjust stat width calculations to take --graph output into account.\n>\n> (1)\n>\n>> Adjust stat width calculations to reserve space for required characters\n>> before\n>> scaling the widths for the filename and graph portions of the diff-stat.\n>> For\n>> example, consider:\n>>\n>> \" diff.c |   66 ++-\"\n>>\n>> Before calculating the widths allocated to the filename, \"diff.c\", and the\n>> graph, \"++-\", reserve space for the initial \" \" and the part between the\n>> filename and graph portions \" |   66 \". Then, divide the remaining space\n>> so\n>> that 5/8ths is given to the filename and 3/8ths for the graph.\n>\n> (2)\n>\n> Hi,\n>\n> I think that (1) is good. It fixes the bug and even makes the code more\n> readable. But (2) should be separated, IMHO... There was a motivation for\n> the layout in 1b058bc30df5f: not changing previous behaviour (\"... at least\n> 5/8 of available space is devoted to filenames. On a standard 80 column\n> terminal, or if not connected to a terminal and using the default of 80\n> columns, this gives the same partition as before.\").\n> (2) would change the way format-patch --stat output looks, which probably is\n> not wanted.\n\nI suppose changing the format of format-patch --stat output could be\nannoying to anyone expecting it to remain unchanged. I'll update the\npatch so that the diff-stat output using the default of 80 columns\nremains unmodified.\n"},{"id":"187555","messageId":"CACz_eye13q0BkBTTGgx8VDBKgBydOrAM8Wx6dx+j90ibbpRszA@mail.gmail.com","threadId":"30028","inReplyTo":"7vd384wejl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-23T04:44:48Z","receivedAt":"2012-03-23T04:44:48Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"2012/3/22 Junio C Hamano :\n> That explains the regression you are introducing, but does not justify it.\n>\n> When you start showing that line, do you already know how many columns at\n> the left edge of the display will be consumed by the ancestry graph part?\n>\n> When the command is run without \"--graph\" option, the answer would\n> obviously be zero, but if it is non-zero, wouldn't it be a more sensible\n> solution to the problem to subtract that width from the total allowed\n> display width (e.g. on 200-column terminal, if the ancestry graph part at\n> the left edge uses 20-columns, you do exactly the same as the current\n> algorithm but use 180 as the width of the terminal).  When --stat-width is\n> explicitly given, that specifies the width of whatever comes after the\n> ancestry graph part, so there is no need to change anything.\n>\n> Am I missing something, or is there something deeper going on?\n\nThe approach you describe would work. The only issue is the current\ncalculations slightly run off the rails when the number of columns is\nless than 26 or so (or, similarly and more frequently, when the\ndifference between the terminal columns and ancestry graph columns is\nless than ~26). To keep the diff-stat output (more or less)\nunmodified, I'll simply add a conditional to address this case, rather\nthan my more drastic approach.\n"},{"id":"187557","messageId":"1332482108-2659-1-git-send-email-lucian.poston@gmail.com","threadId":"30028","inReplyTo":"1332444461-11957-2-git-send-email-lucian.poston@gmail.com","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-23T05:54:55Z","receivedAt":"2012-03-23T05:54:55Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"The recent change to compute the width of diff --stat did not take into\nconsideration the output from --graph. The consequence is that when both\noptions are used, e.g. in 'log --stat --graph', the lines are too long.\n\nAdjust stat width calculations to take --graph output into account.\n\nPrevent graph width of diff-stat from falling below minimum.\n\nSigned-off-by: Lucian Poston \n---\n diff.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----------------\n 1 files changed, 52 insertions(+), 20 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 377ec1e..31ba10c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tint width, name_width, graph_width, number_width = 4, count;\n \tconst char *reset, *add_c, *del_c;\n \tconst char *line_prefix = \"\";\n+\tint line_prefix_length = 0;\n+\tint reserved_character_count;\n \tint extra_shown = 0;\n \tstruct strbuf *msg = NULL;\n \n@@ -1392,6 +1394,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tif (options->output_prefix) {\n \t\tmsg = options->output_prefix(options, options->output_prefix_data);\n \t\tline_prefix = msg->buf;\n+\t\tline_prefix_length = options->output_prefix_length;\n \t}\n \n \tcount = options->stat_count ? options->stat_count : data->nr;\n@@ -1427,37 +1430,46 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t * We have width = stat_width or term_columns() columns total.\n \t * We want a maximum of min(max_len, stat_name_width) for the name part.\n \t * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n-\t * We also need 1 for \" \" and 4 + decimal_width(max_change)\n-\t * for \" | NNNN \" and one the empty column at the end, altogether\n+\t * Each line needs space for the following characters:\n+\t * - 1 for the initial \" \"\n+\t * - 4 + decimal_width(max_change) for \" | NNNN \"\n+\t * - 1 for the empty column at the end,\n+\t * Altogether, the reserved_character_count totals\n \t * 6 + decimal_width(max_change).\n \t *\n-\t * If there's not enough space, we will use the smaller of\n-\t * stat_name_width (if set) and 5/8*width for the filename,\n-\t * and the rest for constant elements + graph part, but no more\n-\t * than stat_graph_width for the graph part.\n-\t * (5/8 gives 50 for filename and 30 for the constant parts + graph\n-\t * for the standard terminal size).\n+\t * Additionally, there may be a line_prefix, which reduces the available\n+\t * width by line_prefix_length.\n+\t *\n+\t * If there's not enough space, we will use the smaller of stat_name_width\n+\t * (if set) and 5/8*width for the filename, and the rest for the graph\n+\t * part, but no more than stat_graph_width for the graph part.\n+\t * Assuming the line prefix is empty, on a standard 80 column terminal\n+\t * this ratio results in 50 characters for the filename and 20 characters\n+\t * for the graph (plus the 10 reserved characters).\n \t *\n \t * In other words: stat_width limits the maximum width, and\n \t * stat_name_width fixes the maximum width of the filename,\n \t * and is also used to divide available columns if there\n \t * aren't enough.\n \t */\n+\treserved_character_count = 6 + number_width;\n \n \tif (options->stat_width == -1)\n \t\twidth = term_columns();\n \telse\n \t\twidth = options->stat_width ? options->stat_width : 80;\n \n+\twidth -= line_prefix_length;\n+\n \tif (options->stat_graph_width == -1)\n \t\toptions->stat_graph_width = diff_stat_graph_width;\n \n \t/*\n-\t * Guarantee 3/8*16==6 for the graph part\n-\t * and 5/8*16==10 for the filename part\n+\t * Guarantees at least 6 characters for the graph part [16 * 3/8]\n+\t * and at least 10 for the filename part [16 * 5/8]\n \t */\n-\tif (width < 16 + 6 + number_width)\n-\t\twidth = 16 + 6 + number_width;\n+\tif (width < 16 + reserved_character_count)\n+\t\twidth = 16 + reserved_character_count;\n \n \t/*\n \t * First assign sizes that are wanted, ignoring available width.\n@@ -1472,16 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t/*\n \t * Adjust adjustable widths not to exceed maximum width\n \t */\n-\tif (name_width + number_width + 6 + graph_width > width) {\n-\t\tif (graph_width > width * 3/8 - number_width - 6)\n-\t\t\tgraph_width = width * 3/8 - number_width - 6;\n+\tif (reserved_character_count + name_width + graph_width > width) {\n+\t\t/*\n+\t\t * Reduce graph_width to be at most 3/8 of the unreserved space and no\n+\t\t * less than 6, which leaves at least 5/8 for the filename.\n+\t\t */\n+\t\tif (graph_width > width * 3/8 - reserved_character_count) {\n+\t\t\tgraph_width = width * 3/8 - reserved_character_count;\n+\t\t\tif (graph_width < 6) {\n+\t\t\t\tgraph_width = 6;\n+\t\t\t}\n+\t\t}\n+\n+\t\t/*\n+\t\t * If the remaining unreserved space will not accomodate the\n+\t\t * filenames, adjust name_width to use all available remaining space.\n+\t\t * Otherwise, assign any extra space to graph_width.\n+\t\t */\n+\t\tif (name_width > width - reserved_character_count - graph_width) {\n+\t\t\tname_width = width - reserved_character_count - graph_width;\n+\t\t} else {\n+\t\t\tgraph_width = width - reserved_character_count - name_width;\n+\t\t}\n+\n+\t\t/*\n+\t\t * If stat-graph-width was specified, limit graph_width to its value.\n+\t\t */\n \t\tif (options->stat_graph_width &&\n-\t\t graph_width > options->stat_graph_width)\n+\t\t\t\tgraph_width > options->stat_graph_width) {\n \t\t\tgraph_width = options->stat_graph_width;\n-\t\tif (name_width > width - number_width - 6 - graph_width)\n-\t\t\tname_width = width - number_width - 6 - graph_width;\n-\t\telse\n-\t\t\tgraph_width = width - number_width - 6 - name_width;\n+\t\t}\n \t}\n \n \t/*\n-- \n1.7.3.4\n"},{"id":"187558","messageId":"1332482276-2787-1-git-send-email-lucian.poston@gmail.com","threadId":"30028","inReplyTo":"1332444461-11957-3-git-send-email-lucian.poston@gmail.com","subject":"Re: [PATCH v2 3/3] t4052: Test that stat width is adjusted for prefixes","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-23T05:57:51Z","receivedAt":"2012-03-23T05:57:51Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"Add test to verify that the commit graph tree output is taken into\nconsideration when the diff stat output width is calculated.\n\nSigned-off-by: Lucian Poston \n---\n t/t4052-stat-output.sh | 20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 328aa8f..8b6e34a 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -198,6 +198,26 @@ respects expect200 show --stat\n respects expect200 log -1 --stat\n EOF\n \n+cat >expect80graphed <<'EOF'\n+| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++\n+EOF\n+cat >expect80 <<'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n+EOF\n+while read verb expect cmd args\n+do\n+\ttest_expect_success \"$cmd $verb 80 COLUMNS (long filename)\" '\n+\t\tCOLUMNS=80 git $cmd $args >output\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp \"$expect\" actual\n+\t'\n+done <<\\EOF\n+respects expect80 show --stat\n+respects expect80 log -1 --stat\n+respects expect80graphed show --stat --graph\n+respects expect80graphed log -1 --stat --graph\n+EOF\n+\n cat >expect <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-- \n1.7.3.4\n"},{"id":"187561","messageId":"4F6C4C90.5050702@in.waw.pl","threadId":"30028","inReplyTo":"1332482108-2659-1-git-send-email-lucian.poston@gmail.com","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-03-23T10:12:32Z","receivedAt":"2012-03-23T10:12:32Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 03/23/2012 06:54 AM, Lucian Poston wrote:\n\n> diff --git a/diff.c b/diff.c\n> index 377ec1e..31ba10c 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \tint width, name_width, graph_width, number_width = 4, count;\n> \tconst char *reset, *add_c, *del_c;\n> \tconst char *line_prefix = \"\";\n> +\tint line_prefix_length = 0;\n> +\tint reserved_character_count;\n> \tint extra_shown = 0;\n> \tstruct strbuf *msg = NULL;\n> \n> @@ -1392,6 +1394,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \tif (options->output_prefix) {\n> \t\tmsg = options->output_prefix(options, options->output_prefix_data);\n> \t\tline_prefix = msg->buf;\n> +\t\tline_prefix_length = options->output_prefix_length;\n> \t}\nHi,\nline_prefix will only be used once. And options->output_prefix_length will\nbe 0 if !options->output_prefix, so line_prefix variable can be eliminated\nand options->output_prefix_length used directly instead.\n\n> \tcount = options->stat_count ? options->stat_count : data->nr;\n> @@ -1427,37 +1430,46 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t * We have width = stat_width or term_columns() columns total.\n> \t * We want a maximum of min(max_len, stat_name_width) for the name part.\n> \t * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n> -\t * We also need 1 for \" \" and 4 + decimal_width(max_change)\n> -\t * for \" | NNNN \" and one the empty column at the end, altogether\n> +\t * Each line needs space for the following characters:\n> +\t * - 1 for the initial \" \"\n> +\t * - 4 + decimal_width(max_change) for \" | NNNN \"\n> +\t * - 1 for the empty column at the end,\n> +\t * Altogether, the reserved_character_count totals\n> \t * 6 + decimal_width(max_change).\n> \t *\n> -\t * If there's not enough space, we will use the smaller of\n> -\t * stat_name_width (if set) and 5/8*width for the filename,\n> -\t * and the rest for constant elements + graph part, but no more\n> -\t * than stat_graph_width for the graph part.\n> -\t * (5/8 gives 50 for filename and 30 for the constant parts + graph\n> -\t * for the standard terminal size).\n> +\t * Additionally, there may be a line_prefix, which reduces the available\n> +\t * width by line_prefix_length.\n> +\t *\n> +\t * If there's not enough space, we will use the smaller of stat_name_width\n> +\t * (if set) and 5/8*width for the filename, and the rest for the graph\n> +\t * part, but no more than stat_graph_width for the graph part.\n> +\t * Assuming the line prefix is empty, on a standard 80 column terminal\n> +\t * this ratio results in 50 characters for the filename and 20 characters\n> +\t * for the graph (plus the 10 reserved characters).\n> \t *\n> \t * In other words: stat_width limits the maximum width, and\n> \t * stat_name_width fixes the maximum width of the filename,\n> \t * and is also used to divide available columns if there\n> \t * aren't enough.\n> \t */\n> +\treserved_character_count = 6 + number_width;\n> \n> \tif (options->stat_width == -1)\n> \t\twidth = term_columns();\n> \telse\n> \t\twidth = options->stat_width ? options->stat_width : 80;\n> \n> +\twidth -= line_prefix_length;\n> +\n> \tif (options->stat_graph_width == -1)\n> \t\toptions->stat_graph_width = diff_stat_graph_width;\n> \n> \t/*\n> -\t * Guarantee 3/8*16==6 for the graph part\n> -\t * and 5/8*16==10 for the filename part\n> +\t * Guarantees at least 6 characters for the graph part [16 * 3/8]\n> +\t * and at least 10 for the filename part [16 * 5/8]\n> \t */\n> -\tif (width< 16 + 6 + number_width)\n> -\t\twidth = 16 + 6 + number_width;\n> +\tif (width< 16 + reserved_character_count)\n> +\t\twidth = 16 + reserved_character_count;\n> \n> \t/*\n> \t * First assign sizes that are wanted, ignoring available width.\n> @@ -1472,16 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t/*\n> \t * Adjust adjustable widths not to exceed maximum width\n> \t */\n\nIn this part below, you add gratuitous braces around single line if-blocks.\nThis makes the code (and the diff) longer with no gain.\n\n> -\tif (name_width + number_width + 6 + graph_width> width) {\n> -\t\tif (graph_width> width * 3/8 - number_width - 6)\n> -\t\t\tgraph_width = width * 3/8 - number_width - 6;\n> +\tif (reserved_character_count + name_width + graph_width> width) {\n> +\t\t/*\n> +\t\t * Reduce graph_width to be at most 3/8 of the unreserved space and no\n> +\t\t * less than 6, which leaves at least 5/8 for the filename.\n> +\t\t */\n> +\t\tif (graph_width> width * 3/8 - reserved_character_count) {\n> +\t\t\tgraph_width = width * 3/8 - reserved_character_count;\n> +\t\t\tif (graph_width< 6) {\n> +\t\t\t\tgraph_width = 6;\n> +\t\t\t}\n> +\t\t}\nThis extra test is not necessary. Above, after /* Guarantees at least 6 characters\nfor the graph part [16 * 3/8] ... */, this should already by so that\n(width * 3/8 - reserved_character_count) is at least 6.\n\n> +\n> +\t\t/*\n> +\t\t * If the remaining unreserved space will not accomodate the\n> +\t\t * filenames, adjust name_width to use all available remaining space.\n> +\t\t * Otherwise, assign any extra space to graph_width.\n> +\t\t */\n> +\t\tif (name_width> width - reserved_character_count - graph_width) {\n> +\t\t\tname_width = width - reserved_character_count - graph_width;\n> +\t\t} else {\n> +\t\t\tgraph_width = width - reserved_character_count - name_width;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * If stat-graph-width was specified, limit graph_width to its value.\n> +\t\t */\n> \t\tif (options->stat_graph_width&&\n> -\t\t graph_width> options->stat_graph_width)\n> +\t\t\t\tgraph_width> options->stat_graph_width) {\n> \t\t\tgraph_width = options->stat_graph_width;\n> -\t\tif (name_width> width - number_width - 6 - graph_width)\n> -\t\t\tname_width = width - number_width - 6 - graph_width;\n> -\t\telse\n> -\t\t\tgraph_width = width - number_width - 6 - name_width;\n> +\t\t}\nHere, the order of the two tests\n(1) if (options->stat_graph_width && graph_width > options->stat_graph_width)\n(2) if (name_width > width - number_width - 6 - graph_width)\nis reversed. This is not OK, because this means that\noptions->stat_graph_width will be used unconditionally, while\nbefore it was subject to limiting by total width.\n\n> \t}\n> \n> \t/*\n\nThe tests:\nAfter the new tests are added, I see:\n\nok 53 - format-patch ignores COLUMNS (long filename)\nok 54 - diff respects COLUMNS (long filename)\nok 55 - show respects COLUMNS (long filename)\nok 56 - log respects COLUMNS (long filename)\nok 57 - show respects 80 COLUMNS (long filename) <=======\nok 58 - log respects 80 COLUMNS (long filename) <-------\nok 59 - show respects 80 COLUMNS (long filename) <=======\nok 60 - log respects 80 COLUMNS (long filename) <-------\n\nSo some tests descriptions are duplicated. Also it would be\nnice to test with --graph in more places. I'm attaching a\nreplacement patch which adds more tests. It should go *before*\nyour series, and your series should tweak the tests to pass,\nshowing what changed.\n\n------- 8< --------\nFrom 348d96dd9ae4a4ffd04aea4497b237a794e37727 Mon Sep 17 00:00:00 2001\nFrom: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= \nDate: Fri, 23 Mar 2012 11:04:21 +0100\nSubject: [PATCH] t4052: test --stat output with --graph\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\nAdd tests which show that the width of the --prefix added by --graph\nis not taken into consideration when the diff stat output width is\ncalculated.\n\nSigned-off-by: Zbigniew Jędrzejewski-Szmek \n---\n t/t4052-stat-output.sh | 78 +++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 74 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 328aa8f..da14984 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -82,11 +82,15 @@ test_expect_success 'preparation for big change tests' '\n cat >expect80 <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-\n+cat >expect80-graph <<'EOF'\n+| abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+EOF\n cat >expect200 <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-\n+cat >expect200-graph <<'EOF'\n+| abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+EOF\n while read verb expect cmd args\n do\n \ttest_expect_success \"$cmd $verb COLUMNS (big change)\" '\n@@ -94,6 +98,14 @@ do\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect\" actual\n \t'\n+\n+\ttest \"$cmd\" != diff || continue\n+\n+\ttest_expect_success \"$cmd --graph $verb COLUMNS (big change)\" '\n+\t\tCOLUMNS=200 git $cmd $args --graph >output\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp \"$expect-graph\" actual\n+\t'\n done <<\\EOF\n ignores expect80 format-patch -1 --stdout\n respects expect200 diff HEAD^ HEAD --stat\n@@ -104,7 +116,9 @@ EOF\n cat >expect40 <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++\n EOF\n-\n+cat >expect40-graph <<'EOF'\n+| abcd | 1000 ++++++++++++++++++++++++++\n+EOF\n while read verb expect cmd args\n do\n \ttest_expect_success \"$cmd $verb not enough COLUMNS (big change)\" '\n@@ -118,6 +132,20 @@ do\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect\" actual\n \t'\n+\n+\ttest \"$cmd\" != diff || continue\n+\n+\ttest_expect_success \"$cmd --graph $verb not enough COLUMNS (big change)\" '\n+\t\tCOLUMNS=40 git $cmd $args --graph >output\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp \"$expect-graph\" actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --graph $verb statGraphWidth config\" '\n+\t\tgit -c diff.statGraphWidth=26 $cmd $args --graph >output\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp \"$expect-graph\" actual\n+\t'\n done <<\\EOF\n ignores expect80 format-patch -1 --stdout\n respects expect40 diff HEAD^ HEAD --stat\n@@ -129,6 +157,9 @@ EOF\n cat >expect <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++\n EOF\n+cat >expect-graph <<'EOF'\n+| abcd | 1000 ++++++++++++++++++++++++++\n+EOF\n while read cmd args\n do\n \ttest_expect_success \"$cmd --stat=width with big change\" '\n@@ -143,11 +174,25 @@ do\n \t\ttest_cmp expect actual\n \t'\n \n-\ttest_expect_success \"$cmd --stat-graph--width with big change\" '\n+\ttest_expect_success \"$cmd --stat-graph-width with big change\" '\n \t\tgit $cmd $args --stat-graph-width=26 >output\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp expect actual\n \t'\n+\n+\ttest \"$cmd\" != diff || continue\n+\n+\ttest_expect_success \"$cmd --stat-width=width --graph with big change\" '\n+\t\tgit $cmd $args --stat-width=40 --graph >output\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp expect-graph actual\n+\t'\n+\n+\ttest_expect_success \"$cmd --stat-graph-width --graph with big change\" '\n+\t\tgit $cmd $args --stat-graph-width=26 --graph >output\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp expect-graph actual\n+\t'\n done <<\\EOF\n format-patch -1 --stdout\n diff HEAD^ HEAD --stat\n@@ -164,6 +209,9 @@ test_expect_success 'preparation for long filename tests' '\n cat >expect <<'EOF'\n ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++\n EOF\n+cat >expect-graph <<'EOF'\n+| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++\n+EOF\n while read cmd args\n do\n \ttest_expect_success \"$cmd --stat=width with big change is more balanced\" '\n@@ -171,6 +219,14 @@ do\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp expect actual\n \t'\n+\n+\ttest \"$cmd\" != diff || continue\n+\n+\ttest_expect_success \"$cmd --stat=width --graph with big change is balanced\" '\n+\t\tgit $cmd $args --stat-width=60 --graph >output &&\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp expect-graph actual\n+\t'\n done <<\\EOF\n format-patch -1 --stdout\n diff HEAD^ HEAD --stat\n@@ -181,9 +237,15 @@ EOF\n cat >expect80 <<'EOF'\n ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n EOF\n+cat >expect80-graph <<'EOF'\n+| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n+EOF\n cat >expect200 <<'EOF'\n aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n+cat >expect200-graph <<'EOF'\n+| aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+EOF\n while read verb expect cmd args\n do\n \ttest_expect_success \"$cmd $verb COLUMNS (long filename)\" '\n@@ -191,6 +253,14 @@ do\n \t\tgrep \" | \" output >actual &&\n \t\ttest_cmp \"$expect\" actual\n \t'\n+\n+\ttest \"$cmd\" != diff || continue\n+\n+\ttest_expect_success \"$cmd --graph $verb COLUMNS (long filename)\" '\n+\t\tCOLUMNS=200 git $cmd $args --graph >output\n+\t\tgrep \" | \" output >actual &&\n+\t\ttest_cmp \"$expect-graph\" actual\n+\t'\n done <<\\EOF\n ignores expect80 format-patch -1 --stdout\n respects expect200 diff HEAD^ HEAD --stat\n-- \n1.7.10.rc1.225.gba57e\n\n\n------- >8 --------\n"},{"id":"187592","messageId":"7vy5qrtcca.fsf@alter.siamese.dyndns.org","threadId":"30028","inReplyTo":"1332482108-2659-1-git-send-email-lucian.poston@gmail.com","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-23T18:13:25Z","receivedAt":"2012-03-23T18:13:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucian Poston writes:\n\n> The recent change to compute the width of diff --stat did not take into\n> consideration the output from --graph. The consequence is that when both\n> options are used, e.g. in 'log --stat --graph', the lines are too long.\n>\n> Adjust stat width calculations to take --graph output into account.\n>\n> Prevent graph width of diff-stat from falling below minimum.\n>\n> Signed-off-by: Lucian Poston \n> ---\n> diff.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----------------\n> 1 files changed, 52 insertions(+), 20 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 377ec1e..31ba10c 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \tint width, name_width, graph_width, number_width = 4, count;\n> \tconst char *reset, *add_c, *del_c;\n> \tconst char *line_prefix = \"\";\n> +\tint line_prefix_length = 0;\n> +\tint reserved_character_count;\n> \tint extra_shown = 0;\n> \tstruct strbuf *msg = NULL;\n> \n> @@ -1392,6 +1394,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \tif (options->output_prefix) {\n> \t\tmsg = options->output_prefix(options, options->output_prefix_data);\n> \t\tline_prefix = msg->buf;\n> +\t\tline_prefix_length = options->output_prefix_length;\n> \t}\n> \n> \tcount = options->stat_count ? options->stat_count : data->nr;\n> @@ -1427,37 +1430,46 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t * We have width = stat_width or term_columns() columns total.\n> \t * We want a maximum of min(max_len, stat_name_width) for the name part.\n> \t * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n> -\t * We also need 1 for \" \" and 4 + decimal_width(max_change)\n> -\t * for \" | NNNN \" and one the empty column at the end, altogether\n> +\t * Each line needs space for the following characters:\n> +\t * - 1 for the initial \" \"\n> +\t * - 4 + decimal_width(max_change) for \" | NNNN \"\n> +\t * - 1 for the empty column at the end,\n> +\t * Altogether, the reserved_character_count totals\n> \t * 6 + decimal_width(max_change).\n> \t *\n> -\t * If there's not enough space, we will use the smaller of\n> -\t * stat_name_width (if set) and 5/8*width for the filename,\n> -\t * and the rest for constant elements + graph part, but no more\n> -\t * than stat_graph_width for the graph part.\n> -\t * (5/8 gives 50 for filename and 30 for the constant parts + graph\n> -\t * for the standard terminal size).\n> +\t * Additionally, there may be a line_prefix, which reduces the available\n> +\t * width by line_prefix_length.\n> +\t *\n> +\t * If there's not enough space, we will use the smaller of stat_name_width\n> +\t * (if set) and 5/8*width for the filename, and the rest for the graph\n> +\t * part, but no more than stat_graph_width for the graph part.\n> +\t * Assuming the line prefix is empty, on a standard 80 column terminal\n> +\t * this ratio results in 50 characters for the filename and 20 characters\n> +\t * for the graph (plus the 10 reserved characters).\n\nPlease do not do reflowing of the text in the same patch as modifying the\nlogic. It is unreadable for the purpose of finding out what you really\nchanged.\n\n> \t *\n> \t * In other words: stat_width limits the maximum width, and\n> \t * stat_name_width fixes the maximum width of the filename,\n> \t * and is also used to divide available columns if there\n> \t * aren't enough.\n> \t */\n> +\treserved_character_count = 6 + number_width;\n\nAs far as I can tell, this introduces a variable that is set (and is meant\nto be set) only at a single place, namely, here, and used throughout the\nrest of the function. But it invites later patches to mistakenly update\nthe variable. I do not see the merit of it.\n\nIf you wanted to have a symbolic name for (6+number_width), #define would\nhave served better.\n\nAlso as we see in the later part of the review, this name is probably way\ntoo long to be useful. We need a shorter and sweeter name to call it.\n\n> \tif (options->stat_width == -1)\n> \t\twidth = term_columns();\n> \telse\n> \t\twidth = options->stat_width ? options->stat_width : 80;\n> \n> +\twidth -= line_prefix_length;\n> +\n\nIsn't this wrong? This is not a rhetoric question, iow, I am not\ndeclaring that this is wrong --- I just cannot see why the above is a good\nchange, as I do not see a sound reasoning behind it.\n\nWhen the user said \"--stat-width=80\", she means that the diffstat part\n(name and bargraph) is to extend 80 places, and she does not expect it to\nbe reduced by the width of the ancestry graph. If the user wanted to clip\nthe entire width, she would have used COLUMNS=80 instead.\n\nAm I missing something?\n\n> @@ -1472,16 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t/*\n> \t * Adjust adjustable widths not to exceed maximum width\n> \t */\n> -\tif (name_width + number_width + 6 + graph_width > width) {\n> -\t\tif (graph_width > width * 3/8 - number_width - 6)\n> -\t\t\tgraph_width = width * 3/8 - number_width - 6;\n> +\tif (reserved_character_count + name_width + graph_width > width) {\n> +\t\t/*\n> +\t\t * Reduce graph_width to be at most 3/8 of the unreserved space and no\n> +\t\t * less than 6, which leaves at least 5/8 for the filename.\n> +\t\t */\n> +\t\tif (graph_width > width * 3/8 - reserved_character_count) {\n> +\t\t\tgraph_width = width * 3/8 - reserved_character_count;\n> +\t\t\tif (graph_width < 6) {\n> +\t\t\t\tgraph_width = 6;\n> +\t\t\t}\n> +\t\t}\n\nWhat is this about? reserved_character_count already knows about the\nmagic number 6 and here you have another magic number 6. How are they\nrelated with each other?\n\nIn other words, shouldn't the added code be more like this?\n\n\tif (graph_width < reserved_character_count - number_width)\n\t\tgraph_width = reserved_character_count - number_width;\n\n> +\t\t/*\n> +\t\t * If the remaining unreserved space will not accomodate the\n> +\t\t * filenames, adjust name_width to use all available remaining space.\n> +\t\t * Otherwise, assign any extra space to graph_width.\n> +\t\t */\n> +\t\tif (name_width > width - reserved_character_count - graph_width) {\n> +\t\t\tname_width = width - reserved_character_count - graph_width;\n> +\t\t} else {\n> +\t\t\tgraph_width = width - reserved_character_count - name_width;\n> +\t\t}\n> +\t\t/*\n> +\t\t * If stat-graph-width was specified, limit graph_width to its value.\n> +\t\t */\n> \t\tif (options->stat_graph_width &&\n> -\t\t graph_width > options->stat_graph_width)\n> +\t\t\t\tgraph_width > options->stat_graph_width) {\n> \t\t\tgraph_width = options->stat_graph_width;\n> -\t\tif (name_width > width - number_width - 6 - graph_width)\n> -\t\t\tname_width = width - number_width - 6 - graph_width;\n> -\t\telse\n> -\t\t\tgraph_width = width - number_width - 6 - name_width;\n> +\t\t}\n> \t}\n\nYikes. It took me three minutes to realize that the only thing you did in\nthis hunk was to move the \"if stat-graph-width is set\" logic down. Not\nyour fault, but this is one of the times I wish our diff generation logic\nmatched \"corresponding\" block of lines better.\n"},{"id":"189086","messageId":"CACz_eyc0AjvU0U2FGzqhUTZ_nncuFoAxZ6VGw0=7LVXH4SLqwA@mail.gmail.com","threadId":"30028","inReplyTo":"4F6C4C90.5050702@in.waw.pl","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-04-12T07:47:50Z","receivedAt":"2012-04-12T07:47:50Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"On Fri, Mar 23, 2012 at 03:12, Zbigniew Jędrzejewski-Szmek\n wrote:\n> On 03/23/2012 06:54 AM, Lucian Poston wrote:\n>\n>> diff --git a/diff.c b/diff.c\n>> index 377ec1e..31ba10c 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>       int width, name_width, graph_width, number_width = 4, count;\n>>       const char *reset, *add_c, *del_c;\n>>       const char *line_prefix = \"\";\n>> +     int line_prefix_length = 0;\n>> +     int reserved_character_count;\n>>       int extra_shown = 0;\n>>       struct strbuf *msg = NULL;\n>>\n>> @@ -1392,6 +1394,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>       if (options->output_prefix) {\n>>               msg = options->output_prefix(options, options->output_prefix_data);\n>>               line_prefix = msg->buf;\n>> +             line_prefix_length = options->output_prefix_length;\n>>       }\n> Hi,\n> line_prefix will only be used once. And options->output_prefix_length will\n> be 0 if !options->output_prefix, so line_prefix variable can be eliminated\n> and options->output_prefix_length used directly instead.\n\nRather than adding the line_prefix_length variable, the next patch\nwill use options->output_prefix_length directly.\n\n>>       count = options->stat_count ? options->stat_count : data->nr;\n>> @@ -1427,37 +1430,46 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>        * We have width = stat_width or term_columns() columns total.\n>>        * We want a maximum of min(max_len, stat_name_width) for the name part.\n>>        * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n>> -      * We also need 1 for \" \" and 4 + decimal_width(max_change)\n>> -      * for \" | NNNN \" and one the empty column at the end, altogether\n>> +      * Each line needs space for the following characters:\n>> +      *   - 1 for the initial \" \"\n>> +      *   - 4 + decimal_width(max_change) for \" | NNNN \"\n>> +      *   - 1 for the empty column at the end,\n>> +      * Altogether, the reserved_character_count totals\n>>        * 6 + decimal_width(max_change).\n>>        *\n>> -      * If there's not enough space, we will use the smaller of\n>> -      * stat_name_width (if set) and 5/8*width for the filename,\n>> -      * and the rest for constant elements + graph part, but no more\n>> -      * than stat_graph_width for the graph part.\n>> -      * (5/8 gives 50 for filename and 30 for the constant parts + graph\n>> -      * for the standard terminal size).\n>> +      * Additionally, there may be a line_prefix, which reduces the available\n>> +      * width by line_prefix_length.\n>> +      *\n>> +      * If there's not enough space, we will use the smaller of stat_name_width\n>> +      * (if set) and 5/8*width for the filename, and the rest for the graph\n>> +      * part, but no more than stat_graph_width for the graph part.\n>> +      * Assuming the line prefix is empty, on a standard 80 column terminal\n>> +      * this ratio results in 50 characters for the filename and 20 characters\n>> +      * for the graph (plus the 10 reserved characters).\n>>        *\n>>        * In other words: stat_width limits the maximum width, and\n>>        * stat_name_width fixes the maximum width of the filename,\n>>        * and is also used to divide available columns if there\n>>        * aren't enough.\n>>        */\n>> +     reserved_character_count = 6 + number_width;\n>>\n>>       if (options->stat_width == -1)\n>>               width = term_columns();\n>>       else\n>>               width = options->stat_width ? options->stat_width : 80;\n>>\n>> +     width -= line_prefix_length;\n>> +\n>>       if (options->stat_graph_width == -1)\n>>               options->stat_graph_width = diff_stat_graph_width;\n>>\n>>       /*\n>> -      * Guarantee 3/8*16==6 for the graph part\n>> -      * and 5/8*16==10 for the filename part\n>> +      * Guarantees at least 6 characters for the graph part [16 * 3/8]\n>> +      * and at least 10 for the filename part [16 * 5/8]\n>>        */\n>> -     if (width<  16 + 6 + number_width)\n>> -             width = 16 + 6 + number_width;\n>> +     if (width<  16 + reserved_character_count)\n>> +             width = 16 + reserved_character_count;\n>>\n>>       /*\n>>        * First assign sizes that are wanted, ignoring available width.\n>> @@ -1472,16 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>       /*\n>>        * Adjust adjustable widths not to exceed maximum width\n>>        */\n>\n> In this part below, you add gratuitous braces around single line if-blocks.\n> This makes the code (and the diff) longer with no gain.\n\nI prefer gratuitous braces, particularly when conditionals are nested\nas they are here. It helps later when maintaining the code if someone\nwants to add a debug statement or comment out a line.\n\nI'll remove braces from single line conditionals to keep with the\nexisting conventions.\n\n>> -     if (name_width + number_width + 6 + graph_width>  width) {\n>> -             if (graph_width>  width * 3/8 - number_width - 6)\n>> -                     graph_width = width * 3/8 - number_width - 6;\n>> +     if (reserved_character_count + name_width + graph_width>  width) {\n>> +             /*\n>> +              * Reduce graph_width to be at most 3/8 of the unreserved space and no\n>> +              * less than 6, which leaves at least 5/8 for the filename.\n>> +              */\n>> +             if (graph_width>  width * 3/8 - reserved_character_count) {\n>> +                     graph_width = width * 3/8 - reserved_character_count;\n>> +                     if (graph_width<  6) {\n>> +                             graph_width = 6;\n>> +                     }\n>> +             }\n> This extra test is not necessary. Above, after /* Guarantees at least 6 characters\n> for the graph part [16 * 3/8] ... */, this should already by so that\n> (width * 3/8 - reserved_character_count) is at least 6.\n\nAhh, this is because the calculations go haywire when the number of\ncolumns is small. I briefly mentioned it here:\nhttp://thread.gmane.org/gmane.comp.version-control.git/193694/focus=193744\n\ngraph_width actually can have a negative value under certain\nconditions, and this check compensates for that edge case. My earlier\npatches took a less conservative approach and adjusted the\ncalculations so that the value of graph_width would be at least 6, but\nit caused several tests to regress. Since the intention of the\noriginal graph_width calculation was place a lower bound of 6 on its\nvalue, I simply enforce that here without affecting the general cases,\nwhich will remain unmodified in order to prevent test regressions.\n\n>> +\n>> +             /*\n>> +              * If the remaining unreserved space will not accomodate the\n>> +              * filenames, adjust name_width to use all available remaining space.\n>> +              * Otherwise, assign any extra space to graph_width.\n>> +              */\n>> +             if (name_width>  width - reserved_character_count - graph_width) {\n>> +                     name_width = width - reserved_character_count - graph_width;\n>> +             } else {\n>> +                     graph_width = width - reserved_character_count - name_width;\n>> +             }\n>> +\n>> +             /*\n>> +              * If stat-graph-width was specified, limit graph_width to its value.\n>> +              */\n>>               if (options->stat_graph_width&&\n>> -                 graph_width>  options->stat_graph_width)\n>> +                             graph_width>  options->stat_graph_width) {\n>>                       graph_width = options->stat_graph_width;\n>> -             if (name_width>  width - number_width - 6 - graph_width)\n>> -                     name_width = width - number_width - 6 - graph_width;\n>> -             else\n>> -                     graph_width = width - number_width - 6 - name_width;\n>> +             }\n> Here, the order of the two tests\n> (1) if (options->stat_graph_width && graph_width > options->stat_graph_width)\n> (2) if (name_width > width - number_width - 6 - graph_width)\n> is reversed. This is not OK, because this means that\n> options->stat_graph_width will be used unconditionally, while\n> before it was subject to limiting by total width.\n\nIf options->stat_graph_width is specified, it should always limit the\nvalue of graph_width, correct? Since (1) is the last test, it can only\ndecrease the value of graph_width, which would already be limited by\nthe total width.\n\nI just noticed that name_width isn't being limited to stat_name_width,\nif it is specified. I'll add a check for that.\n\n> The tests:\n> After the new tests are added, I see:\n>\n> ok 53 - format-patch ignores COLUMNS (long filename)\n> ok 54 - diff respects COLUMNS (long filename)\n> ok 55 - show respects COLUMNS (long filename)\n> ok 56 - log respects COLUMNS (long filename)\n> ok 57 - show respects 80 COLUMNS (long filename)  <=======\n> ok 58 - log respects 80 COLUMNS (long filename)   <-------\n> ok 59 - show respects 80 COLUMNS (long filename)  <=======\n> ok 60 - log respects 80 COLUMNS (long filename)   <-------\n>\n> So some tests descriptions are duplicated. Also it would be\n> nice to test with --graph in more places. I'm attaching a\n> replacement patch which adds more tests. It should go *before*\n> your series, and your series should  tweak the tests to pass,\n> showing what changed.\n\nThanks, I'll add these.\n"},{"id":"189091","messageId":"CACz_eyetzT3AFg1w3CbQegPLHfH0inwcYv5yhbffjog0cBqwug@mail.gmail.com","threadId":"30028","inReplyTo":"7vy5qrtcca.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-04-12T08:35:53Z","receivedAt":"2012-04-12T08:35:53Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"On Fri, Mar 23, 2012 at 11:13, Junio C Hamano wrote:\n> Please do not do reflowing of the text in the same patch as modifying the\n> logic.  It is unreadable for the purpose of finding out what you really\n> changed.\n\nI will un-reflow the text. :]\n\n>\n>>        *\n>>        * In other words: stat_width limits the maximum width, and\n>>        * stat_name_width fixes the maximum width of the filename,\n>>        * and is also used to divide available columns if there\n>>        * aren't enough.\n>>        */\n>> +     reserved_character_count = 6 + number_width;\n>\n> As far as I can tell, this introduces a variable that is set (and is meant\n> to be set) only at a single place, namely, here, and used throughout the\n> rest of the function. But it invites later patches to mistakenly update\n> the variable.  I do not see the merit of it.\n>\n> If you wanted to have a symbolic name for (6+number_width), #define would\n> have served better.\n>\n> Also as we see in the later part of the review, this name is probably way\n> too long to be useful.  We need a shorter and sweeter name to call it.\n\nI'll remove it.\n\n>>       if (options->stat_width == -1)\n>>               width = term_columns();\n>>       else\n>>               width = options->stat_width ? options->stat_width : 80;\n>>\n>> +     width -= line_prefix_length;\n>> +\n>\n> Isn't this wrong?  This is not a rhetoric question, iow, I am not\n> declaring that this is wrong --- I just cannot see why the above is a good\n> change, as I do not see a sound reasoning behind it.\n>\n> When the user said \"--stat-width=80\", she means that the diffstat part\n> (name and bargraph) is to extend 80 places, and she does not expect it to\n> be reduced by the width of the ancestry graph.  If the user wanted to clip\n> the entire width, she would have used COLUMNS=80 instead.\n>\n> Am I missing something?\n\nYou're right, the prefix length shouldn't be subtracted when\n--stat-width is specified.\n\n>> @@ -1472,16 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>       /*\n>>        * Adjust adjustable widths not to exceed maximum width\n>>        */\n>> -     if (name_width + number_width + 6 + graph_width > width) {\n>> -             if (graph_width > width * 3/8 - number_width - 6)\n>> -                     graph_width = width * 3/8 - number_width - 6;\n>> +     if (reserved_character_count + name_width + graph_width > width) {\n>> +             /*\n>> +              * Reduce graph_width to be at most 3/8 of the unreserved space and no\n>> +              * less than 6, which leaves at least 5/8 for the filename.\n>> +              */\n>> +             if (graph_width > width * 3/8 - reserved_character_count) {\n>> +                     graph_width = width * 3/8 - reserved_character_count;\n>> +                     if (graph_width < 6) {\n>> +                             graph_width = 6;\n>> +                     }\n>> +             }\n>\n> What is this about?  reserved_character_count already knows about the\n> magic number 6 and here you have another magic number 6.  How are they\n> related with each other?\n>\n> In other words, shouldn't the added code be more like this?\n>\n>        if (graph_width < reserved_character_count - number_width)\n>                graph_width = reserved_character_count - number_width;\n\nThere are two magic number 6's. From previous comments that explain\nhow reserved_character_count is calculated:\n\n * Each line needs space for the following characters:\n * - 1 for the initial \" \"\n * - 4 + decimal_width(max_change) for \" | NNNN \"\n * - 1 for the empty column at the end,\n * Altogether, the reserved_character_count totals\n * 6 + decimal_width(max_change).\n\nIn the case of deriving reserved_character_count, 6 arises because 1+4+1.\n\nThe second magic number 6 is the minimum value for graph_width. The\nintention of the original stat width calculation was to give the\nfilename portion 5/8ths of the total width and give the graph portion\n3/8ths of the total width. With 80 columns, that works out to 50 for\nfilename and 30 for the graph (plus reserved characters). With 16\ncolumns, that works out to be 10 and 6. I assume 5 and 3 would be too\nsmall, so 10 and 6 were probably chosen as the minimum values by the\nprevious author(s).\n\nSo now you ask why I added the if (graph_width < 6) conditional?\n\n> + if (graph_width > width * 3/8 - reserved_character_count) {\n> + graph_width = width * 3/8 - reserved_character_count;\n> + if (graph_width < 6) {\n> + graph_width = 6;\n> + }\n> + }\n\nThe calculation in the graph_width assignment (and the prior\nconditional) does not guarantee graph_width is at least 6. The\ncalculation should be ((width - reserved_character_count) * 3/8),\ninstead of (width * 3/8 - reserved_character_count). But as we saw in\nmy initial patch, adjusting this calculation causes test regressions.\nTherefore, I added a conditional to catch the edge case where\ngraph_width is less than 6.\n\nAssuming the $COLUMNS is 26 or less, graph_width will actually come\nout to -1, iirc.\n"},{"id":"189097","messageId":"4F86ABA7.8080703@in.waw.pl","threadId":"30028","inReplyTo":"CACz_eyc0AjvU0U2FGzqhUTZ_nncuFoAxZ6VGw0=7LVXH4SLqwA@mail.gmail.com","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-04-12T10:17:11Z","receivedAt":"2012-04-12T10:17:11Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 04/12/2012 09:47 AM, Lucian Poston wrote:\n> On Fri, Mar 23, 2012 at 03:12, Zbigniew Jędrzejewski-Szmek\n> wrote:\n>> On 03/23/2012 06:54 AM, Lucian Poston wrote:\n>>\n>>> diff --git a/diff.c b/diff.c\n>>> index 377ec1e..31ba10c 100644\n>>> --- a/diff.c\n>>> +++ b/diff.c\n>>> @@ -1383,6 +1383,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>> int width, name_width, graph_width, number_width = 4, count;\n>>> const char *reset, *add_c, *del_c;\n>>> const char *line_prefix = \"\";\n>>> + int line_prefix_length = 0;\n>>> + int reserved_character_count;\n>>> int extra_shown = 0;\n>>> struct strbuf *msg = NULL;\n>>>\n>>> @@ -1392,6 +1394,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>> if (options->output_prefix) {\n>>> msg = options->output_prefix(options, options->output_prefix_data);\n>>> line_prefix = msg->buf;\n>>> + line_prefix_length = options->output_prefix_length;\n>>> }\n>> Hi,\n>> line_prefix will only be used once. And options->output_prefix_length will\n>> be 0 if !options->output_prefix, so line_prefix variable can be eliminated\n>> and options->output_prefix_length used directly instead.\n>\n> Rather than adding the line_prefix_length variable, the next patch\n> will use options->output_prefix_length directly.\n>\n>>> count = options->stat_count ? options->stat_count : data->nr;\n>>> @@ -1427,37 +1430,46 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>> * We have width = stat_width or term_columns() columns total.\n>>> * We want a maximum of min(max_len, stat_name_width) for the name part.\n>>> * We want a maximum of min(max_change, stat_graph_width) for the +- part.\n>>> - * We also need 1 for \" \" and 4 + decimal_width(max_change)\n>>> - * for \" | NNNN \" and one the empty column at the end, altogether\n>>> + * Each line needs space for the following characters:\n>>> + * - 1 for the initial \" \"\n>>> + * - 4 + decimal_width(max_change) for \" | NNNN \"\n>>> + * - 1 for the empty column at the end,\n>>> + * Altogether, the reserved_character_count totals\n>>> * 6 + decimal_width(max_change).\n>>> *\n>>> - * If there's not enough space, we will use the smaller of\n>>> - * stat_name_width (if set) and 5/8*width for the filename,\n>>> - * and the rest for constant elements + graph part, but no more\n>>> - * than stat_graph_width for the graph part.\n>>> - * (5/8 gives 50 for filename and 30 for the constant parts + graph\n>>> - * for the standard terminal size).\n>>> + * Additionally, there may be a line_prefix, which reduces the available\n>>> + * width by line_prefix_length.\n>>> + *\n>>> + * If there's not enough space, we will use the smaller of stat_name_width\n>>> + * (if set) and 5/8*width for the filename, and the rest for the graph\n>>> + * part, but no more than stat_graph_width for the graph part.\n>>> + * Assuming the line prefix is empty, on a standard 80 column terminal\n>>> + * this ratio results in 50 characters for the filename and 20 characters\n>>> + * for the graph (plus the 10 reserved characters).\n>>> *\n>>> * In other words: stat_width limits the maximum width, and\n>>> * stat_name_width fixes the maximum width of the filename,\n>>> * and is also used to divide available columns if there\n>>> * aren't enough.\n>>> */\n>>> + reserved_character_count = 6 + number_width;\n>>>\n>>> if (options->stat_width == -1)\n>>> width = term_columns();\n>>> else\n>>> width = options->stat_width ? options->stat_width : 80;\n>>>\n>>> + width -= line_prefix_length;\n>>> +\n>>> if (options->stat_graph_width == -1)\n>>> options->stat_graph_width = diff_stat_graph_width;\n>>>\n>>> /*\n>>> - * Guarantee 3/8*16==6 for the graph part\n>>> - * and 5/8*16==10 for the filename part\n>>> + * Guarantees at least 6 characters for the graph part [16 * 3/8]\n>>> + * and at least 10 for the filename part [16 * 5/8]\n>>> */\n>>> - if (width< 16 + 6 + number_width)\n>>> - width = 16 + 6 + number_width;\n>>> + if (width< 16 + reserved_character_count)\n>>> + width = 16 + reserved_character_count;\n>>>\n>>> /*\n>>> * First assign sizes that are wanted, ignoring available width.\n>>> @@ -1472,16 +1484,36 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n>>> /*\n>>> * Adjust adjustable widths not to exceed maximum width\n>>> */\n>>\n>> In this part below, you add gratuitous braces around single line if-blocks.\n>> This makes the code (and the diff) longer with no gain.\n>\n> I prefer gratuitous braces, particularly when conditionals are nested\n> as they are here. It helps later when maintaining the code if someone\n> wants to add a debug statement or comment out a line.\n>\n> I'll remove braces from single line conditionals to keep with the\n> existing conventions.\nThanks.\n\n>>> - if (name_width + number_width + 6 + graph_width> width) {\n>>> - if (graph_width> width * 3/8 - number_width - 6)\n>>> - graph_width = width * 3/8 - number_width - 6;\n>>> + if (reserved_character_count + name_width + graph_width> width) {\n>>> + /*\n>>> + * Reduce graph_width to be at most 3/8 of the unreserved space and no\n>>> + * less than 6, which leaves at least 5/8 for the filename.\n>>> + */\n>>> + if (graph_width> width * 3/8 - reserved_character_count) {\n>>> + graph_width = width * 3/8 - reserved_character_count;\n>>> + if (graph_width< 6) {\n>>> + graph_width = 6;\n>>> + }\n>>> + }\n>> This extra test is not necessary. Above, after /* Guarantees at least 6 characters\n>> for the graph part [16 * 3/8] ... */, this should already by so that\n>> (width * 3/8 - reserved_character_count) is at least 6.\n>\n> Ahh, this is because the calculations go haywire when the number of\n> columns is small. I briefly mentioned it here:\n> http://thread.gmane.org/gmane.comp.version-control.git/193694/focus=193744\n>\n> graph_width actually can have a negative value under certain\n> conditions, and this check compensates for that edge case. My earlier\n> patches took a less conservative approach and adjusted the\n> calculations so that the value of graph_width would be at least 6, but\n> it caused several tests to regress. Since the intention of the\n> original graph_width calculation was place a lower bound of 6 on its\n> value, I simply enforce that here without affecting the general cases,\n> which will remain unmodified in order to prevent test regressions.\n>\n>>> +\n>>> + /*\n>>> + * If the remaining unreserved space will not accomodate the\n>>> + * filenames, adjust name_width to use all available remaining space.\n>>> + * Otherwise, assign any extra space to graph_width.\n>>> + */\n>>> + if (name_width> width - reserved_character_count - graph_width) {\n>>> + name_width = width - reserved_character_count - graph_width;\n>>> + } else {\n>>> + graph_width = width - reserved_character_count - name_width;\n>>> + }\n>>> +\n>>> + /*\n>>> + * If stat-graph-width was specified, limit graph_width to its value.\n>>> + */\n>>> if (options->stat_graph_width&&\n>>> - graph_width> options->stat_graph_width)\n>>> + graph_width> options->stat_graph_width) {\n>>> graph_width = options->stat_graph_width;\n>>> - if (name_width> width - number_width - 6 - graph_width)\n>>> - name_width = width - number_width - 6 - graph_width;\n>>> - else\n>>> - graph_width = width - number_width - 6 - name_width;\n>>> + }\n>> Here, the order of the two tests\n>> (1) if (options->stat_graph_width&& graph_width> options->stat_graph_width)\n>> (2) if (name_width> width - number_width - 6 - graph_width)\n>> is reversed. This is not OK, because this means that\n>> options->stat_graph_width will be used unconditionally, while\n>> before it was subject to limiting by total width.\n>\n> If options->stat_graph_width is specified, it should always limit the\n> value of graph_width, correct? Since (1) is the last test, it can only\n> decrease the value of graph_width, which would already be limited by\n> the total width.\nRight, but the way the tests are ordered now, we could end up decreasing\nname_width first (after (2)) and then graph_width (after (1)), actually\nusing less than full width.\n\n> I just noticed that name_width isn't being limited to stat_name_width,\n> if it is specified. I'll add a check for that.\nSounds good.\n\n>> The tests:\n>> After the new tests are added, I see:\n>>\n>> ok 53 - format-patch ignores COLUMNS (long filename)\n>> ok 54 - diff respects COLUMNS (long filename)\n>> ok 55 - show respects COLUMNS (long filename)\n>> ok 56 - log respects COLUMNS (long filename)\n>> ok 57 - show respects 80 COLUMNS (long filename)<=======\n>> ok 58 - log respects 80 COLUMNS (long filename)<-------\n>> ok 59 - show respects 80 COLUMNS (long filename)<=======\n>> ok 60 - log respects 80 COLUMNS (long filename)<-------\n>>\n>> So some tests descriptions are duplicated. Also it would be\n>> nice to test with --graph in more places. I'm attaching a\n>> replacement patch which adds more tests. It should go *before*\n>> your series, and your series should tweak the tests to pass,\n>> showing what changed.\n>\n> Thanks, I'll add these.\n\nRegards,\nZbyszek\n"},{"id":"189391","messageId":"CACz_eyfEpE8nZua3JkYtSV42aR_CKJRwvz=4TbOw2zCqSJuDOw@mail.gmail.com","threadId":"30028","inReplyTo":"4F86ABA7.8080703@in.waw.pl","subject":"Re: [PATCH v2 2/3] Adjust stat width calculations to take --graph output into account","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-04-16T11:04:38Z","receivedAt":"2012-04-16T11:04:38Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"On Thu, Apr 12, 2012 at 03:17, Zbigniew Jędrzejewski-Szmek\n wrote:\n>>>> +\n>>>> +             /*\n>>>> +              * If the remaining unreserved space will not accomodate\n>>>> the\n>>>> +              * filenames, adjust name_width to use all available\n>>>> remaining space.\n>>>> +              * Otherwise, assign any extra space to graph_width.\n>>>> +              */\n>>>> +             if (name_width>    width - reserved_character_count -\n>>>> graph_width) {\n>>>> +                     name_width = width - reserved_character_count -\n>>>> graph_width;\n>>>> +             } else {\n>>>> +                     graph_width = width - reserved_character_count -\n>>>> name_width;\n>>>> +             }\n>>>> +\n>>>> +             /*\n>>>> +              * If stat-graph-width was specified, limit graph_width to\n>>>> its value.\n>>>> +              */\n>>>>               if (options->stat_graph_width&&\n>>>> -                 graph_width>    options->stat_graph_width)\n>>>> +                             graph_width>    options->stat_graph_width)\n>>>> {\n>>>>                       graph_width = options->stat_graph_width;\n>>>> -             if (name_width>    width - number_width - 6 - graph_width)\n>>>> -                     name_width = width - number_width - 6 -\n>>>> graph_width;\n>>>> -             else\n>>>> -                     graph_width = width - number_width - 6 -\n>>>> name_width;\n>>>> +             }\n>>>\n>>> Here, the order of the two tests\n>>> (1) if (options->stat_graph_width&&  graph_width>\n>>>  options->stat_graph_width)\n>>>\n>>> (2) if (name_width>  width - number_width - 6 - graph_width)\n>>> is reversed. This is not OK, because this means that\n>>> options->stat_graph_width will be used unconditionally, while\n>>> before it was subject to limiting by total width.\n>>\n>>\n>> If options->stat_graph_width is specified, it should always limit the\n>> value of graph_width, correct? Since (1) is the last test, it can only\n>> decrease the value of graph_width, which would already be limited by\n>> the total width.\n>\n> Right, but the way the tests are ordered now, we could end up decreasing\n> name_width first (after (2)) and then graph_width (after (1)), actually\n> using less than full width.\n\nAhh, I didn't think about that.\n\nI just reverted the order back to the original.\n\n>> I just noticed that name_width isn't being limited to stat_name_width,\n>> if it is specified. I'll add a check for that.\n>\n> Sounds good.\n\nFYI, in patch v3, I reverted the check for stat_graph_width as previously\nmentioned, and I ended up not adding a check for stat_name_width.\nIt remains the case that in certain scenarios, name_width & graph_width\ncould be set to values greater than stat_name_width & stat_graph_width.\n"}]}