{"thread":{"id":"30004","subject":"[PATCH 0/1] Adjust diff stat width calculations so lines do not wrap in terminal when using --graph","startedAt":"2012-03-20T07:38:16Z","lastAt":"2012-03-22T19:39:37Z","messageCount":7,"participants":["Lucian Poston","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"187306","messageId":"1332229097-19262-1-git-send-email-lucian.poston@gmail.com","threadId":"30004","inReplyTo":null,"subject":"[PATCH 0/1] Adjust diff stat width calculations so lines do not wrap in terminal when using --graph","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-20T07:38:16Z","receivedAt":"2012-03-20T07:38:16Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"When diff stats are displayed in the terminal, the width is scaled to fit\nwithin the available $COLUMNS. The current stat width calculations do not\nfactor in the diff output prefix that is displayed when when graphing the\ncommit history e.g. `git log --graph --stat`. Consequently, the diff stats can\nwrap to next line breaking the text graph representation.\n\nThis patch adjusts the diff stat width calculations to prevent line wrapping\nwhen the text-based graph representation is displayed along with the diff\nstats.\n\n\nLucian Poston (1):\n Fix --stat width calculations to handle --graph\n\n diff.c | 55 ++++++++++++++++++++++++++++++++---------------\n t/t4052-stat-output.sh | 24 +++++++++++++++++++-\n 2 files changed, 59 insertions(+), 20 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"187307","messageId":"1332229097-19262-2-git-send-email-lucian.poston@gmail.com","threadId":"30004","inReplyTo":"1332229097-19262-1-git-send-email-lucian.poston@gmail.com","subject":"[PATCH 1/1] Fix --stat width calculations to handle --graph","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-20T07:38:17Z","receivedAt":"2012-03-20T07:38:17Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"Adjusted stat width calculations to take into consideration the diff output\nprefix e.g. the graph prefix generated by `git log --graph --stat`.\n\nThis change fixes the line wrapping that occurs when diff stats are large\nenough to be scaled to fit within the terminal's columns. This issue only\nappears when using --stat and --graph together on large diffs.\n\nAdjusted stat output tests accordingly. The scaled output tests are closer to\nthe target 5:3 ratio.\n\nAdded test that verifies the output of --stat --graph is truncated to fit\nwithin the available terminal $COLUMNS\n\nSigned-off-by: Lucian Poston \n---\n diff.c | 55 ++++++++++++++++++++++++++++++++---------------\n t/t4052-stat-output.sh | 24 +++++++++++++++++++-\n 2 files changed, 59 insertions(+), 20 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 377ec1e..3a26561 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1382,7 +1382,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \tint total_files = data->nr;\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+\tconst char *line_prefix = \"\", *line_prefix_iter;\n+\tunsigned int line_prefix_length = 0;\n+\tunsigned int reserved_character_count;\n \tint extra_shown = 0;\n \tstruct strbuf *msg = NULL;\n \n@@ -1392,6 +1394,18 @@ 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+\n+\t\t/*\n+\t\t * line_prefix can contain color codes, so only pipes '|' and\n+\t\t * spaces ' ' are counted.\n+\t\t */\n+\t\tline_prefix_iter = line_prefix;\n+\t\twhile (*line_prefix_iter != '\\0') {\n+\t\t\tif (*line_prefix_iter == ' ' || *line_prefix_iter == '|') {\n+\t\t\t\tline_prefix_length++;\n+\t\t\t}\n+\t\t\tline_prefix_iter += 1;\n+\t\t}\n \t}\n \n \tcount = options->stat_count ? options->stat_count : data->nr;\n@@ -1427,22 +1441,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 * and the rest for reserved characters + 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 * (5/8 gives 50 for filename and 30 for the reserved characters + graph\n+\t * for the standard terminal size assuming there is no line prefix).\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 +1472,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 +1491,16 @@ 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\tif (graph_width > (width - reserved_character_count) * 3/8)\n+\t\t\tgraph_width = (width - reserved_character_count) * 3/8;\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\tif (name_width > width - reserved_character_count - graph_width)\n+\t\t\tname_width = width - reserved_character_count - graph_width;\n \t\telse\n-\t\t\tgraph_width = width - number_width - 6 - name_width;\n+\t\t\tgraph_width = width - reserved_character_count - name_width;\n \t}\n \n \t/*\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 328aa8f..84dd8bb 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@@ -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":"187333","messageId":"alpine.DEB.1.00.1203201109370.3340@s15462909.onlinehome-server.info","threadId":"30004","inReplyTo":"1332229097-19262-2-git-send-email-lucian.poston@gmail.com","subject":"Re: [PATCH 1/1] Fix --stat width calculations to handle --graph","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2012-03-20T16:17:18Z","receivedAt":"2012-03-20T16:17:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lucian,\n\nOn Tue, 20 Mar 2012, Lucian Poston wrote:\n\n> Adjusted stat width calculations to take into consideration the diff output\n> prefix e.g. the graph prefix generated by `git log --graph --stat`.\n> \n> This change fixes the line wrapping that occurs when diff stats are large\n> enough to be scaled to fit within the terminal's columns. This issue only\n> appears when using --stat and --graph together on large diffs.\n> \n> Adjusted stat output tests accordingly. The scaled output tests are closer to\n> the target 5:3 ratio.\n> \n> Added test that verifies the output of --stat --graph is truncated to fit\n> within the available terminal $COLUMNS\n> \n> Signed-off-by: Lucian Poston \n> ---\n\nGood. Just a quick question before everything else: are the commit\nmessages cut off/wrapped to the same number of columns? If so, where do\nthey get the indent from? (Sorry for asking, but I figured that you're\nalready deep in the code so you might know of the top of your head.)\n\n> diff --git a/diff.c b/diff.c\n> index 377ec1e..3a26561 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1382,7 +1382,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \tint total_files = data->nr;\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> +\tconst char *line_prefix = \"\", *line_prefix_iter;\n> +\tunsigned int line_prefix_length = 0;\n> +\tunsigned int reserved_character_count;\n> \tint extra_shown = 0;\n> \tstruct strbuf *msg = NULL;\n> \n> @@ -1392,6 +1394,18 @@ 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> +\n> +\t\t/*\n> +\t\t * line_prefix can contain color codes, so only pipes '|' and\n> +\t\t * spaces ' ' are counted.\n> +\t\t */\n> +\t\tline_prefix_iter = line_prefix;\n> +\t\twhile (*line_prefix_iter != '\\0') {\n> +\t\t\tif (*line_prefix_iter == ' ' || *line_prefix_iter == '|') {\n> +\t\t\t\tline_prefix_length++;\n> +\t\t\t}\n> +\t\t\tline_prefix_iter += 1;\n> +\t\t}\n> \t}\n\nMy 1st reaction was: why is the current indent width not stored in the options?\nBut you're right, the indent is generated dynamically from output_prefix()\nwhich is a method of diff_options, so there is little chance to do it\ndifferently from your solution.\n\nHowever, a little nit, since this list is so famous for \"just a little\nnit\": I'd prefer to factor-out the indent width measuring, like so:\n\nstatic int count_pipes_and_spaces(const char *string)\n{\n\tint count;\n\n\tfor (count = 0; *string; string++)\n\t\tif (*string == '|' || *string == ' ')\n\t\t\tcount++;\n\n\treturn count;\n}\n\nIt's not only that that new function cannot mess with the local variables\nof show_stats(), it also documents a bit better what the code is supposed\nto do (and all that without a single /* ... */! Isn't that fab? ;)\n\nAs for the complete patch: nicely done. I especially like that it is\nminimally intrusive and that you took great care of updating the comments\n-- not something everybody does!\n\nMy nits aside: this is good to go.\n\nCiao,\nDscho\n"},{"id":"187338","messageId":"7vehsn6vy1.fsf@alter.siamese.dyndns.org","threadId":"30004","inReplyTo":"1332229097-19262-2-git-send-email-lucian.poston@gmail.com","subject":"Re: [PATCH 1/1] Fix --stat width calculations to handle --graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-20T17:09:26Z","receivedAt":"2012-03-20T17:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lucian Poston writes:\n\n> Adjusted stat width calculations to take into consideration the diff output\n> prefix e.g. the graph prefix generated by `git log --graph --stat`.\n>\n> This change fixes the line wrapping that occurs when diff stats are large\n> enough to be scaled to fit within the terminal's columns. This issue only\n> appears when using --stat and --graph together on large diffs.\n>\n> Adjusted stat output tests accordingly. The scaled output tests are closer to\n> the target 5:3 ratio.\n>\n> Added test that verifies the output of --stat --graph is truncated to fit\n> within the available terminal $COLUMNS\n\nThanks.\n\nRegarding the log message:\n\n - Please start it with a problem description. Describe both what the\n current code shows, and why you think it is wrong or suboptimal.\n I.e. the observation of the problem in your second paragraph comes at\n the beginning\n\n - Our log message usually gives an order to the codebase or to the person\n who is applying the patch in order to address the problem you described\n in the earlier part of the log message, instead of tells a story of\n what happened in the past.\n\nE.g.\n\n The recent change to compute the width of diff --stat based on the\n terminal width did not take the width needed to show the --graph\n output into account, and makes lines in \"log --graph --stat\" too long.\n \n Adjust stat width calculation to take the width of graph prefix into\n account. ...\n\n> @@ -1392,6 +1394,18 @@ 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> +\n> +\t\t/*\n> +\t\t * line_prefix can contain color codes, so only pipes '|' and\n> +\t\t * spaces ' ' are counted.\n> +\t\t */\n> +\t\tline_prefix_iter = line_prefix;\n> +\t\twhile (*line_prefix_iter != '\\0') {\n> +\t\t\tif (*line_prefix_iter == ' ' || *line_prefix_iter == '|') {\n> +\t\t\t\tline_prefix_length++;\n> +\t\t\t}\n> +\t\t\tline_prefix_iter += 1;\n> +\t\t}\n\nYikes.\n\nThis code relies on \"Count only ' ' and '|', because these are the only\nones we currently happen to use\", which is fragile. The next person who\nwill update graph.c can change the set of letters used in the graph to\nimprove the output without even knowing your code exists or the assumption\nyour code makes, so she is likely not going to update it.\n\nI think the caller should be taught to pass the exact width it carves out\nof the available width for use by the ancestry graph output, and if we are\nto do so, adding \"int output_prefix_len\" field (which usually is 0) to\ndiff_options, and seting it in graph.c::diff_output_prefix_callback() (at\nthat point, graph->width has the number you want, I think), may be the way\nto go.\n\n> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n> index 328aa8f..84dd8bb 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\nIsn't it a sign that the change is doing a lot more than justified that it\nhas to change the test vector for cases where --graph is *NOT* involved at\nall?\n\n> @@ -179,7 +179,7 @@ log -1 --stat\n> EOF\n> \n> cat >expect80 <<'EOF'\n> - ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n> + ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++++++++\n\nLikewise.\n\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"},{"id":"187339","messageId":"7vaa3b6v9z.fsf@alter.siamese.dyndns.org","threadId":"30004","inReplyTo":"alpine.DEB.1.00.1203201109370.3340@s15462909.onlinehome-server.info","subject":"Re: [PATCH 1/1] Fix --stat width calculations to handle --graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-20T17:23:52Z","receivedAt":"2012-03-20T17:23:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> However, a little nit, since this list is so famous for \"just a little\n> nit\": I'd prefer to factor-out the indent width measuring, like so:\n>\n> static int count_pipes_and_spaces(const char *string)\n> {\n> \tint count;\n>\n> \tfor (count = 0; *string; string++)\n> \t\tif (*string == '|' || *string == ' ')\n> \t\t\tcount++;\n>\n> \treturn count;\n> }\n>\n\nI agree that this is much better than the original by Lucian, but if we\nwere to go this route, I would prefer to see it *not* count pipes and\nspaces, but actually measure the display width of the string. Both the\nname of the function and the implementation would have to change, of\ncourse.\n\nEven though I didn't look very closely, I do not think it should be too\nhard for graph.c to tell the diff_options structure how wide a prefix it\nplaced in the output_prefix, so use of such a \"display_columns()\" function\nwould be wasteful for this particular case, but for a more general case,\nit would come in handy as a helper function, and at that point, this\nshould not hide in diff.c as a static function.\n\nThanks.\n"},{"id":"187508","messageId":"CACz_eyfc+X8zUCBs+mfvWvPaCaki8ma8-wxeQ4QtsQC=d-Caag@mail.gmail.com","threadId":"30004","inReplyTo":"alpine.DEB.1.00.1203201109370.3340@s15462909.onlinehome-server.info","subject":"Re: [PATCH 1/1] Fix --stat width calculations to handle --graph","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-22T19:33:41Z","receivedAt":"2012-03-22T19:33:41Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"2012/3/20 Johannes Schindelin :\n> Good. Just a quick question before everything else: are the commit\n> messages cut off/wrapped to the same number of columns? If so, where do\n> they get the indent from? (Sorry for asking, but I figured that you're\n> already deep in the code so you might know of the top of your head.)\n\nThanks for reviewing the patch! The commit part of git log is handled\nin log-tree.c:show_log().\n\n> My 1st reaction was: why is the current indent width not stored in the options?\n\nThe new patch adds a output_prefix_length field to struct diff_options.\n\nThanks!\nLucian\n"},{"id":"187509","messageId":"CACz_eyeyni0EkM25neWdPXF7Nu8GnZv1am-UkRz3BOxBvvA1Xg@mail.gmail.com","threadId":"30004","inReplyTo":"7vehsn6vy1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/1] Fix --stat width calculations to handle --graph","fromName":"Lucian Poston","fromEmail":"lucian.poston@gmail.com","sentAt":"2012-03-22T19:39:37Z","receivedAt":"2012-03-22T19:39:37Z","isPatch":true,"sender":{"key":"lucian.poston@gmail.com","avatar":"https://avatars.githubusercontent.com/u/646121?v=4"},"body":"2012/3/20 Junio C Hamano :\n> Regarding the log message:\n>\n>  - Please start it with a problem description. Describe both what the\n>   current code shows, and why you think it is wrong or suboptimal.\n>   I.e. the observation of the problem in your second paragraph comes at\n>   the beginning\n>\n>  - Our log message usually gives an order to the codebase or to the person\n>   who is applying the patch in order to address the problem you described\n>   in the earlier part of the log message, instead of tells a story of\n>   what happened in the past.\n>\n> E.g.\n>\n>    The recent change to compute the width of diff --stat based on the\n>    terminal width did not take the width needed to show the --graph\n>    output into account, and makes lines in \"log --graph --stat\" too long.\n>\n>    Adjust stat width calculation to take the width of graph prefix into\n>    account. ...\n\nThanks for letting me know. Patch v2 has updated log messages. Let me\nknow whether they meet the conventions.\n\n> I think the caller should be taught to pass the exact width it carves out\n> of the available width for use by the ancestry graph output, and if we are\n> to do so, adding \"int output_prefix_len\" field (which usually is 0) to\n> diff_options, and seting it in graph.c::diff_output_prefix_callback() (at\n> that point, graph->width has the number you want, I think), may be the way\n> to go.\n\nAdded outout_prefix_length to struct diff_options in patch v2.\n\n>> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n>> index 328aa8f..84dd8bb 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>\n> Isn't it a sign that the change is doing a lot more than justified that it\n> has to change the test vector for cases where --graph is *NOT* involved at\n> all?\n\nIt is, and I didn't make that clear in the log message. In patch v2,\nthe log message describes what has changed to in the calculation to\ncause this.\n\nThanks!\nLucian\n"}]}