{"thread":{"id":"47667","subject":"[PATCH] format-patch: set diffstat width to 70 instead of default 80","startedAt":"2018-01-22T12:32:07Z","lastAt":"2018-02-02T18:42:12Z","messageCount":17,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"337079","messageId":"20180122123154.8301-1-pclouds@gmail.com","threadId":"47667","inReplyTo":null,"subject":"[PATCH] format-patch: set diffstat width to 70 instead of default 80","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-22T12:31:54Z","receivedAt":"2018-01-22T12:32:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Patches or cover letters generated by format-patch are meant to be\nexchanged as emails, most of the time. And since it's generally agreed\nthat text in mails should be wrapped around 70 columns or so, make sure\nthese diffstat follow the convention.\n\nI noticed this when I quoted a diffstat line [1]. Should we do something\nlike this? diffstat is rarely quoted though so perhaps the stat width\nshould be something like 75.\n\nt4052 fails but I don't think it's worth fixing until it's clear if it's\nworth doing this.\n\n[1] https://public-inbox.org/git/20180122121426.GD5980@ash/T/#u\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 14fdf39165..6be79656c5 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n \topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n+\topts.diffopt.stat_width = 70;\n \n \tdiff_setup_done(&opts);\n \n@@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--check does not make sense\"));\n \n \tif (!use_patch_format &&\n-\t\t(!rev.diffopt.output_format ||\n-\t\t rev.diffopt.output_format == DIFF_FORMAT_PATCH))\n+\t (!rev.diffopt.output_format ||\n+\t rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {\n \t\trev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;\n+\t\tif (!rev.diffopt.stat_width)\n+\t\t\trev.diffopt.stat_width = 70;\n+\t}\n \n \t/* Always generate a patch */\n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n-- \n2.16.0.47.g3d9b0fac3a\n\n"},{"id":"337118","messageId":"xmqqbmhljxmi.fsf@gitster.mtv.corp.google.com","threadId":"47667","inReplyTo":"20180122123154.8301-1-pclouds@gmail.com","subject":"Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-22T23:13:09Z","receivedAt":"2018-01-22T23:13:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Patches or cover letters generated by format-patch are meant to be\n> exchanged as emails, most of the time. And since it's generally agreed\n> that text in mails should be wrapped around 70 columns or so, make sure\n> these diffstat follow the convention.\n>\n> I noticed this when I quoted a diffstat line [1]. Should we do something\n> like this? diffstat is rarely quoted though so perhaps the stat width\n> should be something like 75.\n>\n> t4052 fails but I don't think it's worth fixing until it's clear if it's\n> worth doing this.\n\nI guess you meant this as an RFC/PATCH; FWIW, I personally am in\nfavor of this change.\n\n>\n> [1] https://public-inbox.org/git/20180122121426.GD5980@ash/T/#u\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/log.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 14fdf39165..6be79656c5 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \n> \tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n> \topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> +\topts.diffopt.stat_width = 70;\n> \n> \tdiff_setup_done(&opts);\n> \n> @@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\tdie(_(\"--check does not make sense\"));\n> \n> \tif (!use_patch_format &&\n> -\t\t(!rev.diffopt.output_format ||\n> -\t\t rev.diffopt.output_format == DIFF_FORMAT_PATCH))\n> +\t (!rev.diffopt.output_format ||\n> +\t rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {\n> \t\trev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;\n> +\t\tif (!rev.diffopt.stat_width)\n> +\t\t\trev.diffopt.stat_width = 70;\n> +\t}\n> \n> \t/* Always generate a patch */\n> \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n"},{"id":"337126","messageId":"20180122235202.GA26357@sigill.intra.peff.net","threadId":"47667","inReplyTo":"20180122123154.8301-1-pclouds@gmail.com","subject":"Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-22T23:52:02Z","receivedAt":"2018-01-22T23:52:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Patches or cover letters generated by format-patch are meant to be\n> exchanged as emails, most of the time. And since it's generally agreed\n> that text in mails should be wrapped around 70 columns or so, make sure\n> these diffstat follow the convention.\n> \n> I noticed this when I quoted a diffstat line [1]. Should we do something\n> like this? diffstat is rarely quoted though so perhaps the stat width\n> should be something like 75.\n\nI think the general idea is sensible. Somewhere I picked up \"72\" as the\nright size for email lines to accommodate quoting, but I'm pretty sure\nyou could justify any number between 70 and 75. :)\n\nA few thoughts looking at the patch:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 14fdf39165..6be79656c5 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \n> \tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n> \topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> +\topts.diffopt.stat_width = 70;\n> \n> \tdiff_setup_done(&opts);\n\nI wondered how this should interact with any config, but I don't think\nyou can actually configure the stat-width. You _can_ configure\ndiff.statgraphwidth, though, which seems like a funny inconsistency.\n\nAnyway, I'm not it would make sense to prefer any kind of generic\ndiff.statwidth to this value. The point is that the context here has to\ndo with emails, not just terminals, and the rules are different. So I\nthink you'd need format.statwidth or something. I'm perfectly willing to\npunt on that until somebody actually cares.\n\n> @@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\tdie(_(\"--check does not make sense\"));\n> \n> \tif (!use_patch_format &&\n> -\t\t(!rev.diffopt.output_format ||\n> -\t\t rev.diffopt.output_format == DIFF_FORMAT_PATCH))\n> +\t (!rev.diffopt.output_format ||\n> +\t rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {\n> \t\trev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;\n> +\t\tif (!rev.diffopt.stat_width)\n> +\t\t\trev.diffopt.stat_width = 70;\n> +\t}\n\nHmm, so if I say:\n\n git format-patch --stat --patch\n\nI'd get the larger default? That seems kind of funny. Should this\nstat_width setting be outside of this conditional (and if the user\nasks for a non-stat format, it would just be ignored)?\n\n-Peff\n\nPS I had a funny feeling that this had come up before not due to\n quoting, but just due to people with enormous terminals generating\n too-long lines. But I couldn't find any discussion, and my\n (admittedly brief) reading of the code is that we'd actually respect\n the terminal size by default.\n\n While digging, I did find this discussion, though:\n\n https://public-inbox.org/git/20080403102214.GA23121@coredump.intra.peff.net/\n"},{"id":"337130","messageId":"87r2qhh1xd.fsf@evledraar.gmail.com","threadId":"47667","inReplyTo":"20180122123154.8301-1-pclouds@gmail.com","subject":"Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-23T00:08:30Z","receivedAt":"2018-01-23T00:08:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 22 2018, Nguyễn Thái Ngọc Duy jotted:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 14fdf39165..6be79656c5 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n> \n> \tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n> \topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> +\topts.diffopt.stat_width = 70;\n> \n> \tdiff_setup_done(&opts);\n\nThere's no opts.diffopt. Presumably this should be squashed on top:\n\n - opts.diffopt.stat_width = 70;\n + rev->diffopt.stat_width = 70;\n"},{"id":"337131","messageId":"87po61h1to.fsf@evledraar.gmail.com","threadId":"47667","inReplyTo":"20180122235202.GA26357@sigill.intra.peff.net","subject":"Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-01-23T00:10:43Z","receivedAt":"2018-01-23T00:10:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 22 2018, Jeff King jotted:\n\n> On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>> +\topts.diffopt.stat_width = 70;\n>>\n>> \tdiff_setup_done(&opts);\n>\n> I wondered how this should interact with any config, but I don't think\n> you can actually configure the stat-width. You _can_ configure\n> diff.statgraphwidth, though, which seems like a funny inconsistency.\n\nIsn't the numeric argument to --stat (this works with/without this\npatch):\n\n $ git format-patch -10 --stdout --stat=30 -- t|grep -m 5 ' | '\n ...submodule-update.sh | 1 +\n ...ule-update.sh | 14 ++++++\n ...-addresses.sh | 27 ---\n t/t9000/test.pl | 67 ------\n ...send-email.sh | 19 ++++++\n $ git format-patch -10 --stdout --stat=90 -- t|grep -m 5 ' | '\n t/lib-submodule-update.sh | 1 +\n t/lib-submodule-update.sh | 14 ++++++++++++++\n t/t9000-addresses.sh | 27 -------------------------\n t/t9000/test.pl | 67 --------------------------------------------------------------\n t/t9001-send-email.sh | 19 +++++++++++++++++++\n"},{"id":"337132","messageId":"20180123001214.GC26357@sigill.intra.peff.net","threadId":"47667","inReplyTo":"87po61h1to.fsf@evledraar.gmail.com","subject":"Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-23T00:12:14Z","receivedAt":"2018-01-23T00:12:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 23, 2018 at 01:10:43AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> \n> On Mon, Jan 22 2018, Jeff King jotted:\n> \n> > On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> >> +\topts.diffopt.stat_width = 70;\n> >>\n> >> \tdiff_setup_done(&opts);\n> >\n> > I wondered how this should interact with any config, but I don't think\n> > you can actually configure the stat-width. You _can_ configure\n> > diff.statgraphwidth, though, which seems like a funny inconsistency.\n> \n> Isn't the numeric argument to --stat (this works with/without this\n> patch):\n> \n> $ git format-patch -10 --stdout --stat=30 -- t|grep -m 5 ' | '\n> ...submodule-update.sh | 1 +\n> ...ule-update.sh | 14 ++++++\n> ...-addresses.sh | 27 ---\n> t/t9000/test.pl | 67 ------\n> ...send-email.sh | 19 ++++++\n> $ git format-patch -10 --stdout --stat=90 -- t|grep -m 5 ' | '\n> t/lib-submodule-update.sh | 1 +\n> t/lib-submodule-update.sh | 14 ++++++++++++++\n> t/t9000-addresses.sh | 27 -------------------------\n> t/t9000/test.pl | 67 --------------------------------------------------------------\n> t/t9001-send-email.sh | 19 +++++++++++++++++++\n\nYeah, I meant by actual on-disk config. I didn't actually look at the\npatch closely, but I assumed that \"format-patch --stat=90\" would still\noverride this (if not, then I think that would be a bug).\n\n-Peff\n"},{"id":"337142","messageId":"CACsJy8AWNOz0opf4RSPPZs=fSLKppw2pg8mKn6unLXiyjM+=FQ@mail.gmail.com","threadId":"47667","inReplyTo":"20180122235202.GA26357@sigill.intra.peff.net","subject":"Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-23T02:42:12Z","receivedAt":"2018-01-23T02:42:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 23, 2018 at 6:52 AM, Jeff King wrote:\n> On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> Patches or cover letters generated by format-patch are meant to be\n>> exchanged as emails, most of the time. And since it's generally agreed\n>> that text in mails should be wrapped around 70 columns or so, make sure\n>> these diffstat follow the convention.\n>>\n>> I noticed this when I quoted a diffstat line [1]. Should we do something\n>> like this? diffstat is rarely quoted though so perhaps the stat width\n>> should be something like 75.\n>\n> I think the general idea is sensible. Somewhere I picked up \"72\" as the\n> right size for email lines to accommodate quoting, but I'm pretty sure\n> you could justify any number between 70 and 75. :)\n\nI think it's easy to settle on 72 because cover letter's shortlog\nalready wraps at 72 columns. No point in introducing another number\nhere.\n\n> PS I had a funny feeling that this had come up before not due to\n> quoting, but just due to people with enormous terminals generating\n> too-long lines. But I couldn't find any discussion, and my\n> (admittedly brief) reading of the code is that we'd actually respect\n> the terminal size by default.\n\nYeah, there are tests to check that we do ignore terminal size too.\n-- \nDuy\n"},{"id":"337362","messageId":"20180125115927.16196-1-pclouds@gmail.com","threadId":"47667","inReplyTo":"20180122123154.8301-1-pclouds@gmail.com","subject":"[PATCH v2 0/2] wrap format-patch diffstats around 72 columns","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-25T11:59:25Z","receivedAt":"2018-01-25T11:59:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Like v1, these changes keep diffstat generated by format-patch in 72\ncolumns. This constant is already used in the code, so it's a bit\nbetter than my random \"70 or 75\" value.\n\nGranted these hard coded values (both 80 and 72) are not really nice.\nBut I would wait for somebody to say \"I need or want this\" before I\nadd code to make the default configurable.\n\nNguyễn Thái Ngọc Duy (2):\n format-patch: keep cover-letter diffstat wrapped in 72 columns\n format-patch: reduce patch diffstat width to 72\n\n builtin/log.c | 7 ++++++-\n t/t4052-stat-output.sh | 28 ++++++++++++++--------------\n 2 files changed, 20 insertions(+), 15 deletions(-)\n\n-- \n2.16.1.200.g71ee9f6994\n\n"},{"id":"337363","messageId":"20180125115927.16196-2-pclouds@gmail.com","threadId":"47667","inReplyTo":"20180125115927.16196-1-pclouds@gmail.com","subject":"[PATCH v2 1/2] format-patch: keep cover-letter diffstat wrapped in 72 columns","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-25T11:59:26Z","receivedAt":"2018-01-25T11:59:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We already wrap shortlog around 72 columns in cover letters. Do the same\nfor diffstat (also in cover letters).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 46b4ca13e5..96af897403 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -29,6 +29,8 @@\n #include \"gpg-interface.h\"\n #include \"progress.h\"\n \n+#define MAIL_DEFAULT_WRAP 72\n+\n /* Set a default date-time format for git log (\"log.date\" config variable) */\n static const char *default_date_mode = NULL;\n \n@@ -1044,7 +1046,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tshortlog_init(&log);\n \tlog.wrap_lines = 1;\n-\tlog.wrap = 72;\n+\tlog.wrap = MAIL_DEFAULT_WRAP;\n \tlog.in1 = 2;\n \tlog.in2 = 4;\n \tlog.file = rev->diffopt.file;\n@@ -1061,6 +1063,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n \topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n+\topts.stat_width = MAIL_DEFAULT_WRAP;\n \n \tdiff_setup_done(&opts);\n \n-- \n2.16.1.200.g71ee9f6994\n\n"},{"id":"337364","messageId":"20180125115927.16196-3-pclouds@gmail.com","threadId":"47667","inReplyTo":"20180125115927.16196-1-pclouds@gmail.com","subject":"[PATCH v2 2/2] format-patch: reduce patch diffstat width to 72","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-25T11:59:27Z","receivedAt":"2018-01-25T12:00:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Patches generated by format-patch are meant to be exchanged as emails,\nmost of the time. And since it's generally agreed that text in mails\nshould be wrapped around 70 columns or so, make sure these diffstat\nfollow the convention (especially when used with --cover-letter since we\nalready defaults to wrapping 72 columns). The default can still be\noverriden with command line options.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 2 ++\n t/t4052-stat-output.sh | 28 ++++++++++++++--------------\n 2 files changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 96af897403..94ee177d56 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1617,6 +1617,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t(!rev.diffopt.output_format ||\n \t\t rev.diffopt.output_format == DIFF_FORMAT_PATCH))\n \t\trev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;\n+\tif (!rev.diffopt.stat_width)\n+\t\trev.diffopt.stat_width = MAIL_DEFAULT_WRAP;\n \n \t/* Always generate a patch */\n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 9f563db20a..1e62333b46 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -60,7 +60,7 @@ do\n \t\ttest_cmp expect actual\n \t'\n done <<\\EOF\n-format-patch -1 --stdout\n+format-patch --stat=80 -1 --stdout\n diff HEAD^ HEAD --stat\n show --stat\n log -1 --stat\n@@ -79,11 +79,11 @@ test_expect_success 'preparation for big change tests' '\n \tgit commit -m message abcd\n '\n \n-cat >expect80 <<'EOF'\n- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+cat >expect72 <<'EOF'\n+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-cat >expect80-graph <<'EOF'\n-| abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+cat >expect72-graph <<'EOF'\n+| abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n cat >expect200 <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n@@ -107,7 +107,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect200 diff HEAD^ HEAD --stat\n respects expect200 show --stat\n respects expect200 log -1 --stat\n@@ -135,7 +135,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect40 diff HEAD^ HEAD --stat\n respects expect40 show --stat\n respects expect40 log -1 --stat\n@@ -163,7 +163,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect40 diff HEAD^ HEAD --stat\n respects expect40 show --stat\n respects expect40 log -1 --stat\n@@ -250,11 +250,11 @@ show --stat\n log -1 --stat\n EOF\n \n-cat >expect80 <<'EOF'\n- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n+cat >expect72 <<'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++\n EOF\n-cat >expect80-graph <<'EOF'\n-| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n+cat >expect72-graph <<'EOF'\n+| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++\n EOF\n cat >expect200 <<'EOF'\n aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n@@ -278,7 +278,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect200 diff HEAD^ HEAD --stat\n respects expect200 show --stat\n respects expect200 log -1 --stat\n@@ -308,7 +308,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect1 diff HEAD^ HEAD --stat\n respects expect1 show --stat\n respects expect1 log -1 --stat\n-- \n2.16.1.200.g71ee9f6994\n\n"},{"id":"337575","messageId":"20180127164735.GA23478@sigill.intra.peff.net","threadId":"47667","inReplyTo":"20180125115927.16196-3-pclouds@gmail.com","subject":"Re: [PATCH v2 2/2] format-patch: reduce patch diffstat width to 72","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-27T16:47:35Z","receivedAt":"2018-01-27T16:47:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 25, 2018 at 06:59:27PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n> index 9f563db20a..1e62333b46 100755\n> --- a/t/t4052-stat-output.sh\n> +++ b/t/t4052-stat-output.sh\n> @@ -60,7 +60,7 @@ do\n> \t\ttest_cmp expect actual\n> \t'\n> done <<\\EOF\n> -format-patch -1 --stdout\n> +format-patch --stat=80 -1 --stdout\n> diff HEAD^ HEAD --stat\n> show --stat\n> log -1 --stat\n\nThis hunk confused me. I think what is going on is this:\n\n - we have a loop that runs the same test on several commands\n\n - that loop expects format-patch, diff, etc, to have the same output\n\n - now that format-patch differs from the other commands in its default\n length, we need to use a manual --stat-width to get identical output\n\nIt seems like that kind of nullifies the point of some of the tests in\nthe loop, though, since they are meant to check the behavior without\n--stat.\n\nOTOH, I think that case is tested later (in the other tests you\nadjusted). So I guess these tests are just covering the \"name vs bar\nlength\" part?\n\n-Peff\n"},{"id":"337576","messageId":"20180127164843.GB23478@sigill.intra.peff.net","threadId":"47667","inReplyTo":"20180125115927.16196-1-pclouds@gmail.com","subject":"Re: [PATCH v2 0/2] wrap format-patch diffstats around 72 columns","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-27T16:48:44Z","receivedAt":"2018-01-27T16:48:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 25, 2018 at 06:59:25PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Like v1, these changes keep diffstat generated by format-patch in 72\n> columns. This constant is already used in the code, so it's a bit\n> better than my random \"70 or 75\" value.\n> \n> Granted these hard coded values (both 80 and 72) are not really nice.\n> But I would wait for somebody to say \"I need or want this\" before I\n> add code to make the default configurable.\n> \n> Nguyễn Thái Ngọc Duy (2):\n> format-patch: keep cover-letter diffstat wrapped in 72 columns\n> format-patch: reduce patch diffstat width to 72\n\nThis looks OK to me. There was one head-scratcher in the second patch\nwhich I mentioned, but I think it's probably OK.\n\nI also notice that we have no tests for diffstat on a cover letter,\nwhich might be worth addressing.\n\n-Peff\n"},{"id":"337779","messageId":"CACsJy8B_LzT1UqPmFg72SbQ9c_c5U6y+Yjv+UMFBz+vaXeE0AQ@mail.gmail.com","threadId":"47667","inReplyTo":"20180127164735.GA23478@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] format-patch: reduce patch diffstat width to 72","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-01-30T10:22:17Z","receivedAt":"2018-01-30T10:22:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 27, 2018 at 11:47 PM, Jeff King wrote:\n> On Thu, Jan 25, 2018 at 06:59:27PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n>> index 9f563db20a..1e62333b46 100755\n>> --- a/t/t4052-stat-output.sh\n>> +++ b/t/t4052-stat-output.sh\n>> @@ -60,7 +60,7 @@ do\n>> test_cmp expect actual\n>> '\n>> done <<\\EOF\n>> -format-patch -1 --stdout\n>> +format-patch --stat=80 -1 --stdout\n>> diff HEAD^ HEAD --stat\n>> show --stat\n>> log -1 --stat\n>\n> This hunk confused me. I think what is going on is this:\n>\n> - we have a loop that runs the same test on several commands\n>\n> - that loop expects format-patch, diff, etc, to have the same output\n>\n> - now that format-patch differs from the other commands in its default\n> length, we need to use a manual --stat-width to get identical output\n>\n> It seems like that kind of nullifies the point of some of the tests in\n> the loop, though, since they are meant to check the behavior without\n> --stat.\n>\n> OTOH, I think that case is tested later (in the other tests you\n> adjusted). So I guess these tests are just covering the \"name vs bar\n> length\" part?\n\nMy bad. My thought was.. \"Hmm.. I would need to take format-patch out\nout the loop since it won't match exactly 80 columns anymore. That's a\nlot of work. Hey how about using this opportunity to test that --stat=\ncan still override default settings?\" I didn't realize the follow\ntests in that loop set stat width. I'll take format-patch out of the\nloop and deal with it separately.\n-- \nDuy\n"},{"id":"338023","messageId":"20180201124750.15032-1-pclouds@gmail.com","threadId":"47667","inReplyTo":"20180125115927.16196-1-pclouds@gmail.com","subject":"[PATCH v3 0/2] wrap format-patch diffstats around 72 columns","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-01T12:47:48Z","receivedAt":"2018-02-01T12:48:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v3 fixes tests in 2/2 that I overlooked (but Jeff didn't). Interdiff:\n\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 1e62333b46..6e2cf933f7 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -19,17 +19,33 @@ test_expect_success 'preparation' '\n \tgit commit -m message \"$name\"\n '\n \n+cat >expect72 <<-'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+EOF\n+test_expect_success \"format-patch: small change with long name gives more space to the name\" '\n+\tgit format-patch -1 --stdout >output &&\n+\tgrep \" | \" output >actual &&\n+\ttest_cmp expect72 actual\n+'\n+\n while read cmd args\n do\n-\tcat >expect <<-'EOF'\n+\tcat >expect80 <<-'EOF'\n \t ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n \tEOF\n \ttest_expect_success \"$cmd: small change with long name gives more space to the name\" '\n \t\tgit $cmd $args >output &&\n \t\tgrep \" | \" output >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_cmp expect80 actual\n \t'\n+done <<\\EOF\n+diff HEAD^ HEAD --stat\n+show --stat\n+log -1 --stat\n+EOF\n \n+while read cmd args\n+do\n \tcat >expect <<-'EOF'\n \t ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n \tEOF\n@@ -60,7 +76,7 @@ do\n \t\ttest_cmp expect actual\n \t'\n done <<\\EOF\n-format-patch --stat=80 -1 --stdout\n+format-patch -1 --stdout\n diff HEAD^ HEAD --stat\n show --stat\n log -1 --stat\n\n\nNguyễn Thái Ngọc Duy (2):\n format-patch: keep cover-letter diffstat wrapped in 72 columns\n format-patch: reduce patch diffstat width to 72\n\n builtin/log.c | 7 ++++++-\n t/t4052-stat-output.sh | 46 ++++++++++++++++++++++++++++--------------\n 2 files changed, 37 insertions(+), 16 deletions(-)\n\n-- \n2.16.1.205.g271f633410\n\n"},{"id":"338024","messageId":"20180201124750.15032-2-pclouds@gmail.com","threadId":"47667","inReplyTo":"20180201124750.15032-1-pclouds@gmail.com","subject":"[PATCH v3 1/2] format-patch: keep cover-letter diffstat wrapped in 72 columns","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-01T12:47:49Z","receivedAt":"2018-02-01T12:48:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"We already wrap shortlog around 72 columns in cover letters. Do the same\nfor diffstat (also in cover letters).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 46b4ca13e5..96af897403 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -29,6 +29,8 @@\n #include \"gpg-interface.h\"\n #include \"progress.h\"\n \n+#define MAIL_DEFAULT_WRAP 72\n+\n /* Set a default date-time format for git log (\"log.date\" config variable) */\n static const char *default_date_mode = NULL;\n \n@@ -1044,7 +1046,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tshortlog_init(&log);\n \tlog.wrap_lines = 1;\n-\tlog.wrap = 72;\n+\tlog.wrap = MAIL_DEFAULT_WRAP;\n \tlog.in1 = 2;\n \tlog.in2 = 4;\n \tlog.file = rev->diffopt.file;\n@@ -1061,6 +1063,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,\n \n \tmemcpy(&opts, &rev->diffopt, sizeof(opts));\n \topts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n+\topts.stat_width = MAIL_DEFAULT_WRAP;\n \n \tdiff_setup_done(&opts);\n \n-- \n2.16.1.205.g271f633410\n\n"},{"id":"338025","messageId":"20180201124750.15032-3-pclouds@gmail.com","threadId":"47667","inReplyTo":"20180201124750.15032-1-pclouds@gmail.com","subject":"[PATCH v3 2/2] format-patch: reduce patch diffstat width to 72","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-02-01T12:47:50Z","receivedAt":"2018-02-01T12:48:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Patches generated by format-patch are meant to be exchanged as emails,\nmost of the time. And since it's generally agreed that text in mails\nshould be wrapped around 70 columns or so, make sure these diffstat\nfollow the convention (especially when used with --cover-letter since we\nalready defaults to wrapping 72 columns). The default can still be\noverriden with command line options.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 2 ++\n t/t4052-stat-output.sh | 46 ++++++++++++++++++++++++++++--------------\n 2 files changed, 33 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 96af897403..94ee177d56 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1617,6 +1617,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t(!rev.diffopt.output_format ||\n \t\t rev.diffopt.output_format == DIFF_FORMAT_PATCH))\n \t\trev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;\n+\tif (!rev.diffopt.stat_width)\n+\t\trev.diffopt.stat_width = MAIL_DEFAULT_WRAP;\n \n \t/* Always generate a patch */\n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 9f563db20a..6e2cf933f7 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -19,17 +19,33 @@ test_expect_success 'preparation' '\n \tgit commit -m message \"$name\"\n '\n \n+cat >expect72 <<-'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+EOF\n+test_expect_success \"format-patch: small change with long name gives more space to the name\" '\n+\tgit format-patch -1 --stdout >output &&\n+\tgrep \" | \" output >actual &&\n+\ttest_cmp expect72 actual\n+'\n+\n while read cmd args\n do\n-\tcat >expect <<-'EOF'\n+\tcat >expect80 <<-'EOF'\n \t ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n \tEOF\n \ttest_expect_success \"$cmd: small change with long name gives more space to the name\" '\n \t\tgit $cmd $args >output &&\n \t\tgrep \" | \" output >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_cmp expect80 actual\n \t'\n+done <<\\EOF\n+diff HEAD^ HEAD --stat\n+show --stat\n+log -1 --stat\n+EOF\n \n+while read cmd args\n+do\n \tcat >expect <<-'EOF'\n \t ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n \tEOF\n@@ -79,11 +95,11 @@ test_expect_success 'preparation for big change tests' '\n \tgit commit -m message abcd\n '\n \n-cat >expect80 <<'EOF'\n- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+cat >expect72 <<'EOF'\n+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-cat >expect80-graph <<'EOF'\n-| abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+cat >expect72-graph <<'EOF'\n+| abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n cat >expect200 <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n@@ -107,7 +123,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect200 diff HEAD^ HEAD --stat\n respects expect200 show --stat\n respects expect200 log -1 --stat\n@@ -135,7 +151,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect40 diff HEAD^ HEAD --stat\n respects expect40 show --stat\n respects expect40 log -1 --stat\n@@ -163,7 +179,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect40 diff HEAD^ HEAD --stat\n respects expect40 show --stat\n respects expect40 log -1 --stat\n@@ -250,11 +266,11 @@ show --stat\n log -1 --stat\n EOF\n \n-cat >expect80 <<'EOF'\n- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n+cat >expect72 <<'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++\n EOF\n-cat >expect80-graph <<'EOF'\n-| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++\n+cat >expect72-graph <<'EOF'\n+| ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++\n EOF\n cat >expect200 <<'EOF'\n aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n@@ -278,7 +294,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect200 diff HEAD^ HEAD --stat\n respects expect200 show --stat\n respects expect200 log -1 --stat\n@@ -308,7 +324,7 @@ do\n \t\ttest_cmp \"$expect-graph\" actual\n \t'\n done <<\\EOF\n-ignores expect80 format-patch -1 --stdout\n+ignores expect72 format-patch -1 --stdout\n respects expect1 diff HEAD^ HEAD --stat\n respects expect1 show --stat\n respects expect1 log -1 --stat\n-- \n2.16.1.205.g271f633410\n\n"},{"id":"338106","messageId":"xmqq7ervdyiv.fsf@gitster-ct.c.googlers.com","threadId":"47667","inReplyTo":"20180201124750.15032-1-pclouds@gmail.com","subject":"Re: [PATCH v3 0/2] wrap format-patch diffstats around 72 columns","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-02T18:42:00Z","receivedAt":"2018-02-02T18:42:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> v3 fixes tests in 2/2 that I overlooked (but Jeff didn't). Interdiff:\n> ...\n> Nguyễn Thái Ngọc Duy (2):\n> format-patch: keep cover-letter diffstat wrapped in 72 columns\n> format-patch: reduce patch diffstat width to 72\n\nThanks, will replace. I think we are pretty in good shape with\nthis change now.\n\n"}]}