{"thread":{"id":"49767","subject":"[PATCH] format-patch: respect --stat when explicitly specified","startedAt":"2018-11-06T10:48:16Z","lastAt":"2018-11-07T22:21:42Z","messageCount":10,"participants":["Leif Lindholm","Duy Nguyen","Laszlo Ersek","Nguyễn Thái Ngọc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"362569","messageId":"20181106104811.14625-1-leif.lindholm@linaro.org","threadId":"49767","inReplyTo":null,"subject":"[PATCH] format-patch: respect --stat when explicitly specified","fromName":"Leif Lindholm","fromEmail":"leif.lindholm@linaro.org","sentAt":"2018-11-06T10:48:11Z","receivedAt":"2018-11-06T10:48:16Z","isPatch":true,"sender":{"key":"leif.lindholm@linaro.org","avatar":null},"body":"Commit 43662b23abbd\n(\"format-patch: keep cover-letter diffstat wrapped in 72 columns\") made\nformat-patch keep the diffstat to within 72 characters. However, it does\nthis even when --stat is explicitly set on the command line.\n\nMake it possible to explicitly override the new mechanism, using --stat,\nmatching the functionality before this change. This also matches the\noutput in the case of non-cover-letter files.\n\nCc: Nguyễn Thái Ngọc Duy \nCc: Junio C Hamano \nReported-by: Laszlo Ersek \nSigned-off-by: Leif Lindholm \n---\n\nNote:\nIn TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so\nour official submission guidelines specify the use of --stat.\n\n builtin/log.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 061d4fd86..07e6ae2c1 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,\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+\tif (rev->diffopt.stat_width == -1)\n+\t\topts.stat_width = MAIL_DEFAULT_WRAP;\n \n \tdiff_setup_done(&opts);\n \n-- \n2.11.0\n\n"},{"id":"362581","messageId":"CACsJy8AjY1-KT9x_CXuo+L3-g947OaC5Av0KwuJF-Xkz6VMiPQ@mail.gmail.com","threadId":"49767","inReplyTo":"20181106104811.14625-1-leif.lindholm@linaro.org","subject":"Re: [PATCH] format-patch: respect --stat when explicitly specified","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-06T15:56:11Z","receivedAt":"2018-11-06T15:56:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 6, 2018 at 11:48 AM Leif Lindholm wrote:\n>\n> Commit 43662b23abbd\n> (\"format-patch: keep cover-letter diffstat wrapped in 72 columns\") made\n> format-patch keep the diffstat to within 72 characters. However, it does\n> this even when --stat is explicitly set on the command line.\n>\n> Make it possible to explicitly override the new mechanism, using --stat,\n> matching the functionality before this change. This also matches the\n> output in the case of non-cover-letter files.\n>\n> Cc: Nguyễn Thái Ngọc Duy \n> Cc: Junio C Hamano \n> Reported-by: Laszlo Ersek \n> Signed-off-by: Leif Lindholm \n> ---\n>\n> Note:\n> In TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so\n> our official submission guidelines specify the use of --stat.\n>\n> builtin/log.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 061d4fd86..07e6ae2c1 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,\n>\n> memcpy(&opts, &rev->diffopt, sizeof(opts));\n> opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> - opts.stat_width = MAIL_DEFAULT_WRAP;\n> + if (rev->diffopt.stat_width == -1)\n\nI don't think we get -1 here when stat_width is not defined. The\n\"undefined\" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP\nin here unless you specify --stat.\n\nSo I think you can just drop the below assignment. But if you want to\nbe on the safe side, check for zero stat_width instead of -1 and set\nMAIL_DEFAULT_WRAP.\n\n> + opts.stat_width = MAIL_DEFAULT_WRAP;\n\nHow about a test to make sure this will not be broken in future?\n\n>\n> diff_setup_done(&opts);\n>\n> --\n> 2.11.0\n-- \nDuy\n"},{"id":"362586","messageId":"0171b18f-0732-48fa-b4be-efbc9ba22f85@redhat.com","threadId":"49767","inReplyTo":"CACsJy8AjY1-KT9x_CXuo+L3-g947OaC5Av0KwuJF-Xkz6VMiPQ@mail.gmail.com","subject":"Re: [PATCH] format-patch: respect --stat when explicitly specified","fromName":"Laszlo Ersek","fromEmail":"lersek@redhat.com","sentAt":"2018-11-06T16:17:59Z","receivedAt":"2018-11-06T16:18:02Z","isPatch":true,"sender":{"key":"lersek@redhat.com","avatar":null},"body":"On 11/06/18 16:56, Duy Nguyen wrote:\n> On Tue, Nov 6, 2018 at 11:48 AM Leif Lindholm wrote:\n>>\n>> Commit 43662b23abbd\n>> (\"format-patch: keep cover-letter diffstat wrapped in 72 columns\") made\n>> format-patch keep the diffstat to within 72 characters. However, it does\n>> this even when --stat is explicitly set on the command line.\n>>\n>> Make it possible to explicitly override the new mechanism, using --stat,\n>> matching the functionality before this change. This also matches the\n>> output in the case of non-cover-letter files.\n>>\n>> Cc: Nguyễn Thái Ngọc Duy \n>> Cc: Junio C Hamano \n>> Reported-by: Laszlo Ersek \n>> Signed-off-by: Leif Lindholm \n>> ---\n>>\n>> Note:\n>> In TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so\n>> our official submission guidelines specify the use of --stat.\n>>\n>> builtin/log.c | 3 ++-\n>> 1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index 061d4fd86..07e6ae2c1 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,\n>>\n>> memcpy(&opts, &rev->diffopt, sizeof(opts));\n>> opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n>> - opts.stat_width = MAIL_DEFAULT_WRAP;\n>> + if (rev->diffopt.stat_width == -1)\n> \n> I don't think we get -1 here when stat_width is not defined. The\n> \"undefined\" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP\n> in here unless you specify --stat.\n> \n> So I think you can just drop the below assignment. But if you want to\n> be on the safe side, check for zero stat_width instead of -1 and set\n> MAIL_DEFAULT_WRAP.\n\nLooks like I'll have to test v2 then...\n\n> \n>> + opts.stat_width = MAIL_DEFAULT_WRAP;\n> \n> How about a test to make sure this will not be broken in future?\n\nOh, looks like I won't have to test this patch at all! ;)\n\n(Just kidding, I'll test the next iteration.)\n\nThanks,\nLaszlo\n\n> \n>>\n>> diff_setup_done(&opts);\n>>\n>> --\n>> 2.11.0\n\n"},{"id":"362587","messageId":"CACsJy8CqfApePPY4zA9f=KOSTsA8pUA2ez_3KiXeOJQqtSNb7Q@mail.gmail.com","threadId":"49767","inReplyTo":"0171b18f-0732-48fa-b4be-efbc9ba22f85@redhat.com","subject":"Re: [PATCH] format-patch: respect --stat when explicitly specified","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-06T16:20:25Z","receivedAt":"2018-11-06T16:20:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 6, 2018 at 5:18 PM Laszlo Ersek wrote:\n> >> + opts.stat_width = MAIL_DEFAULT_WRAP;\n> >\n> > How about a test to make sure this will not be broken in future?\n>\n> Oh, looks like I won't have to test this patch at all! ;)\n>\n> (Just kidding, I'll test the next iteration.)\n\nJust to be clear I didn't mean you didn't test it. I meant adding a\nnew test case to our test suite.\n-- \nDuy\n"},{"id":"362589","messageId":"20181106163118.wkvcjvb7uxjvnryk@bivouac.eciton.net","threadId":"49767","inReplyTo":"CACsJy8AjY1-KT9x_CXuo+L3-g947OaC5Av0KwuJF-Xkz6VMiPQ@mail.gmail.com","subject":"Re: [PATCH] format-patch: respect --stat when explicitly specified","fromName":"Leif Lindholm","fromEmail":"leif.lindholm@linaro.org","sentAt":"2018-11-06T16:31:18Z","receivedAt":"2018-11-06T16:31:25Z","isPatch":true,"sender":{"key":"leif.lindholm@linaro.org","avatar":null},"body":"On Tue, Nov 06, 2018 at 04:56:11PM +0100, Duy Nguyen wrote:\n> On Tue, Nov 6, 2018 at 11:48 AM Leif Lindholm wrote:\n> >\n> > Commit 43662b23abbd\n> > (\"format-patch: keep cover-letter diffstat wrapped in 72 columns\") made\n> > format-patch keep the diffstat to within 72 characters. However, it does\n> > this even when --stat is explicitly set on the command line.\n> >\n> > Make it possible to explicitly override the new mechanism, using --stat,\n> > matching the functionality before this change. This also matches the\n> > output in the case of non-cover-letter files.\n> >\n> > Cc: Nguyễn Thái Ngọc Duy \n> > Cc: Junio C Hamano \n> > Reported-by: Laszlo Ersek \n> > Signed-off-by: Leif Lindholm \n> > ---\n> >\n> > Note:\n> > In TianoCore we have LotsOfGloriousFilesNamedInReallyLongCamelCase, so\n> > our official submission guidelines specify the use of --stat.\n> >\n> > builtin/log.c | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/log.c b/builtin/log.c\n> > index 061d4fd86..07e6ae2c1 100644\n> > --- a/builtin/log.c\n> > +++ b/builtin/log.c\n> > @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,\n> >\n> > memcpy(&opts, &rev->diffopt, sizeof(opts));\n> > opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> > - opts.stat_width = MAIL_DEFAULT_WRAP;\n> > + if (rev->diffopt.stat_width == -1)\n> \n> I don't think we get -1 here when stat_width is not defined. The\n> \"undefined\" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP\n> in here unless you specify --stat.\n\nFrom what I could tell, if nothing is specified on command line,\nrev->diffopt.stat_width is set to -1 at this point (I assumed by\nrev->cmd_log_init_defaults(), but didn't go digging).\n\nThe patched version certainly gives the <= 2.16.* behaviour with\n--stat and still restricts stat lines to 72 characters without.\n\n> So I think you can just drop the below assignment. But if you want to\n> be on the safe side, check for zero stat_width instead of -1 and set\n> MAIL_DEFAULT_WRAP.\n> \n> > + opts.stat_width = MAIL_DEFAULT_WRAP;\n> \n> How about a test to make sure this will not be broken in future?\n\nSure. Only today was the first time I had a look at the git sources,\nso some guidance would be most appreciated.\n\nShould I add a function to t/t4014-format-patch.sh and just put\nsomething longer than a/file for the expect template?\n\n/\n Leif\n\n> >\n> > diff_setup_done(&opts);\n> >\n> > --\n> > 2.11.0\n> -- \n> Duy\n"},{"id":"362591","messageId":"CACsJy8BGMDqiekYLdStfyxNBsW88HKv9Z_KwKjjGrEQuucnDPw@mail.gmail.com","threadId":"49767","inReplyTo":"20181106163118.wkvcjvb7uxjvnryk@bivouac.eciton.net","subject":"Re: [PATCH] format-patch: respect --stat when explicitly specified","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-06T17:13:00Z","receivedAt":"2018-11-06T17:13:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 6, 2018 at 5:31 PM Leif Lindholm wrote:\n> > > diff --git a/builtin/log.c b/builtin/log.c\n> > > index 061d4fd86..07e6ae2c1 100644\n> > > --- a/builtin/log.c\n> > > +++ b/builtin/log.c\n> > > @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,\n> > >\n> > > memcpy(&opts, &rev->diffopt, sizeof(opts));\n> > > opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> > > - opts.stat_width = MAIL_DEFAULT_WRAP;\n> > > + if (rev->diffopt.stat_width == -1)\n> >\n> > I don't think we get -1 here when stat_width is not defined. The\n> > \"undefined\" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP\n> > in here unless you specify --stat.\n>\n> From what I could tell, if nothing is specified on command line,\n> rev->diffopt.stat_width is set to -1 at this point (I assumed by\n> rev->cmd_log_init_defaults(), but didn't go digging).\n\nI thought the same but could find where cmd_log_.. is called by\nformat-patch. I was not even sure if I read the code correctly so I\nran the command through gdb. It was definitely not called.\n\n> The patched version certainly gives the <= 2.16.* behaviour with\n> --stat and still restricts stat lines to 72 characters without.\n>\n> > So I think you can just drop the below assignment. But if you want to\n> > be on the safe side, check for zero stat_width instead of -1 and set\n> > MAIL_DEFAULT_WRAP.\n> >\n> > > + opts.stat_width = MAIL_DEFAULT_WRAP;\n> >\n> > How about a test to make sure this will not be broken in future?\n>\n> Sure. Only today was the first time I had a look at the git sources,\n> so some guidance would be most appreciated.\n\nNo problem (and if you don't have time to do it, just say the word and\nI will continue; this is my bug after all)\n\n> Should I add a function to t/t4014-format-patch.sh and just put\n> something longer than a/file for the expect template?\n\nFirst of all the README file in that directory could give pretty good\nbasic instructions.\n\nBack to this test, I think you could start by copying to a new test\n(the whole \"test_expect_success\" block, optionally including the\n\"expect\" file creation too), add --stat there and see what it looks\nlike. For stat testing, t4052 could also be a good example. Or perhaps\nthe test should be added in t4052 because it already supports long\nfile name ($name is 120 char long).\n-- \nDuy\n"},{"id":"362593","messageId":"20181106172505.nw2ugm4yb3gmrnk3@bivouac.eciton.net","threadId":"49767","inReplyTo":"CACsJy8BGMDqiekYLdStfyxNBsW88HKv9Z_KwKjjGrEQuucnDPw@mail.gmail.com","subject":"Re: [PATCH] format-patch: respect --stat when explicitly specified","fromName":"Leif Lindholm","fromEmail":"leif.lindholm@linaro.org","sentAt":"2018-11-06T17:25:05Z","receivedAt":"2018-11-06T17:25:11Z","isPatch":true,"sender":{"key":"leif.lindholm@linaro.org","avatar":null},"body":"On Tue, Nov 06, 2018 at 06:13:00PM +0100, Duy Nguyen wrote:\n> On Tue, Nov 6, 2018 at 5:31 PM Leif Lindholm wrote:\n> > > > diff --git a/builtin/log.c b/builtin/log.c\n> > > > index 061d4fd86..07e6ae2c1 100644\n> > > > --- a/builtin/log.c\n> > > > +++ b/builtin/log.c\n> > > > @@ -1009,7 +1009,8 @@ static void show_diffstat(struct rev_info *rev,\n> > > >\n> > > > memcpy(&opts, &rev->diffopt, sizeof(opts));\n> > > > opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;\n> > > > - opts.stat_width = MAIL_DEFAULT_WRAP;\n> > > > + if (rev->diffopt.stat_width == -1)\n> > >\n> > > I don't think we get -1 here when stat_width is not defined. The\n> > > \"undefined\" value is zero but I'm pretty sure we get MAIL_DEFAULT_WRAP\n> > > in here unless you specify --stat.\n> >\n> > From what I could tell, if nothing is specified on command line,\n> > rev->diffopt.stat_width is set to -1 at this point (I assumed by\n> > rev->cmd_log_init_defaults(), but didn't go digging).\n> \n> I thought the same but could find where cmd_log_.. is called by\n> format-patch. I was not even sure if I read the code correctly so I\n> ran the command through gdb. It was definitely not called.\n\nHuh...\n\n> > The patched version certainly gives the <= 2.16.* behaviour with\n> > --stat and still restricts stat lines to 72 characters without.\n> >\n> > > So I think you can just drop the below assignment. But if you want to\n> > > be on the safe side, check for zero stat_width instead of -1 and set\n> > > MAIL_DEFAULT_WRAP.\n> > >\n> > > > + opts.stat_width = MAIL_DEFAULT_WRAP;\n> > >\n> > > How about a test to make sure this will not be broken in future?\n> >\n> > Sure. Only today was the first time I had a look at the git sources,\n> > so some guidance would be most appreciated.\n> \n> No problem (and if you don't have time to do it, just say the word and\n> I will continue; this is my bug after all)\n\nWeeeell, if you're offering, I would certainly appreciate not having\nto dig deeper into this. I've got a patch review backlog the length of\nmy arm in another project...\n\n> > Should I add a function to t/t4014-format-patch.sh and just put\n> > something longer than a/file for the expect template?\n> \n> First of all the README file in that directory could give pretty good\n> basic instructions.\n> \n> Back to this test, I think you could start by copying to a new test\n> (the whole \"test_expect_success\" block, optionally including the\n> \"expect\" file creation too), add --stat there and see what it looks\n> like. For stat testing, t4052 could also be a good example. Or perhaps\n> the test should be added in t4052 because it already supports long\n> file name ($name is 120 char long).\n\n(Thanks!)\n\n/\n Leif\n"},{"id":"362594","messageId":"CACsJy8C1bcsd88vHM5WFHAAHd0=+OLp32QnSfA2WB5Hfqm=OMA@mail.gmail.com","threadId":"49767","inReplyTo":"20181106172505.nw2ugm4yb3gmrnk3@bivouac.eciton.net","subject":"Re: [PATCH] format-patch: respect --stat when explicitly specified","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-06T17:27:53Z","receivedAt":"2018-11-06T17:28:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Nov 6, 2018 at 6:25 PM Leif Lindholm wrote:\n> > > Sure. Only today was the first time I had a look at the git sources,\n> > > so some guidance would be most appreciated.\n> >\n> > No problem (and if you don't have time to do it, just say the word and\n> > I will continue; this is my bug after all)\n>\n> Weeeell, if you're offering, I would certainly appreciate not having\n> to dig deeper into this. I've got a patch review backlog the length of\n> my arm in another project...\n\nYour loss. Your name is not going to be in git.git then (j/k). Thanks\nfor the report. I'll double, triple check, add tests and resubmit\nsoon.\n-- \nDuy\n"},{"id":"362666","messageId":"20181107164953.24965-1-pclouds@gmail.com","threadId":"49767","inReplyTo":"20181106104811.14625-1-leif.lindholm@linaro.org","subject":"[PATCH v2] format-patch: respect --stat in cover letter's diffstat","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-07T16:49:53Z","receivedAt":"2018-11-07T16:50:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit 43662b23ab (format-patch: keep cover-letter diffstat wrapped in\n72 columns - 2018-01-24) uncondtionally sets stat width to 72 when\ngenerating diffstat for the cover letter, ignoring --stat from command\nline. But it should only do so when stat width is still default\n(i.e. stat_width == 0).\n\nIn order to fix this, we should only set stat_width if stat_width is\nzero. But it will never be. Commit 071dd0ba43 (format-patch: reduce\npatch diffstat width to 72 - 2018-02-01) makes sure that default stat\nwidth will be 72 (ignoring $COLUMNS, but could still be overriden by\n--stat). So all we need to do here is drop the assignment.\n\nReported-by: Laszlo Ersek \nHelped-by: Leif Lindholm \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/log.c | 2 --\n t/t4052-stat-output.sh | 48 +++++++++++++++++++++++++++++-------------\n 2 files changed, 33 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 061d4fd864..1a39c6e52a 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1009,8 +1009,6 @@ static void show_diffstat(struct rev_info *rev,\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 \tdiff_tree_oid(get_commit_tree_oid(origin),\ndiff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\nindex 6e2cf933f7..b1ce0d9b97 100755\n--- a/t/t4052-stat-output.sh\n+++ b/t/t4052-stat-output.sh\n@@ -44,42 +44,50 @@ show --stat\n log -1 --stat\n EOF\n \n-while read cmd args\n+cat >expect.40 <<-'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+EOF\n+cat >expect.6030 <<-'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+EOF\n+cat >expect2.40 <<-'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+EOF\n+cat >expect2.6030 <<-'EOF'\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n+EOF\n+while read expect cmd args\n do\n-\tcat >expect <<-'EOF'\n-\t ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n-\tEOF\n \ttest_expect_success \"$cmd --stat=width: a long name is given more room when the bar is short\" '\n \t\tgit $cmd $args --stat=40 >output &&\n \t\tgrep \" | \" output >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_cmp $expect.40 actual\n \t'\n \n \ttest_expect_success \"$cmd --stat-width=width with long name\" '\n \t\tgit $cmd $args --stat-width=40 >output &&\n \t\tgrep \" | \" output >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_cmp $expect.40 actual\n \t'\n \n-\tcat >expect <<-'EOF'\n-\t ...aaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +\n-\tEOF\n \ttest_expect_success \"$cmd --stat=...,name-width with long name\" '\n \t\tgit $cmd $args --stat=60,30 >output &&\n \t\tgrep \" | \" output >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_cmp $expect.6030 actual\n \t'\n \n \ttest_expect_success \"$cmd --stat-name-width with long name\" '\n \t\tgit $cmd $args --stat-name-width=30 >output &&\n \t\tgrep \" | \" output >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_cmp $expect.6030 actual\n \t'\n done <<\\EOF\n-format-patch -1 --stdout\n-diff HEAD^ HEAD --stat\n-show --stat\n-log -1 --stat\n+expect2 format-patch --cover-letter -1 --stdout\n+expect diff HEAD^ HEAD --stat\n+expect show --stat\n+expect log -1 --stat\n EOF\n \n \n@@ -95,6 +103,16 @@ test_expect_success 'preparation for big change tests' '\n \tgit commit -m message abcd\n '\n \n+cat >expect72 <<'EOF'\n+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n+EOF\n+test_expect_success \"format-patch --cover-letter ignores COLUMNS (big change)\" '\n+\tCOLUMNS=200 git format-patch -1 --stdout --cover-letter >output &&\n+\tgrep \" | \" output >actual &&\n+\ttest_cmp expect72 actual\n+'\n+\n cat >expect72 <<'EOF'\n abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n EOF\n-- \n2.19.1.1005.gac84295441\n\n"},{"id":"362676","messageId":"f0f95dd0-1a9e-01d0-70f4-3c6d5450df70@redhat.com","threadId":"49767","inReplyTo":"20181107164953.24965-1-pclouds@gmail.com","subject":"Re: [PATCH v2] format-patch: respect --stat in cover letter's diffstat","fromName":"Laszlo Ersek","fromEmail":"lersek@redhat.com","sentAt":"2018-11-07T22:21:38Z","receivedAt":"2018-11-07T22:21:42Z","isPatch":true,"sender":{"key":"lersek@redhat.com","avatar":null},"body":"On 11/07/18 17:49, Nguyễn Thái Ngọc Duy wrote:\n> Commit 43662b23ab (format-patch: keep cover-letter diffstat wrapped in\n> 72 columns - 2018-01-24) uncondtionally sets stat width to 72 when\n> generating diffstat for the cover letter, ignoring --stat from command\n> line. But it should only do so when stat width is still default\n> (i.e. stat_width == 0).\n> \n> In order to fix this, we should only set stat_width if stat_width is\n> zero. But it will never be. Commit 071dd0ba43 (format-patch: reduce\n> patch diffstat width to 72 - 2018-02-01) makes sure that default stat\n> width will be 72 (ignoring $COLUMNS, but could still be overriden by\n> --stat). So all we need to do here is drop the assignment.\n> \n> Reported-by: Laszlo Ersek \n> Helped-by: Leif Lindholm \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/log.c | 2 --\n> t/t4052-stat-output.sh | 48 +++++++++++++++++++++++++++++-------------\n> 2 files changed, 33 insertions(+), 17 deletions(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index 061d4fd864..1a39c6e52a 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1009,8 +1009,6 @@ static void show_diffstat(struct rev_info *rev,\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> \tdiff_tree_oid(get_commit_tree_oid(origin),\n\nBecause I plan to use the patch on top of v2.19.1 (until the next major\nrelease, v2.20, is made), that's also where I applied and tested the patch.\n\nWith master @ a4b8ab5363a3, this patch targets show_diffstat(). At\nv2.19.1, commit fa5b7ea670f4 (\"format-patch: allow additional generated\ncontent in make_cover_letter()\", 2018-07-23) had not occurred yet, so\nthere the subject code still lived in make_cover_letter(). On my end,\ngit-am has applied the hunk to make_cover_letter() seamlessly.\n\nI tested the patch with \"--stat=1000 --stat-graph-width=20\", formatting\nan edk2 series that contained commit 1ed6498c4a02\n(\"UefiCpuPkg/CommonFeature: Skip locking when the feature is disabled\",\n2018-11-07). The long pathname\n\"UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c\" is no longer\ntruncated in the cumulative diffstat, in the cover letter.\n\nTested-by: Laszlo Ersek \n\n> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh\n> [...]\n\nI didn't try to run the test suite (I wasn't conscious of it anyway); I\nbuilt & installed git with\n\n nice make -j4 prefix=... all doc info\n nice make prefix=... install install-doc install-html install-info\n\nI also wasn't watching the make log. So if those make targets don't\ninclude the test suite, then I didn't exercise the new test case.\n\nThank you!\nLaszlo\n"}]}