Re: [PATCH v2] format-patch: respect --stat in cover letter's diffstat
On 11/10/18 06:46, Nguyễn Thái Ngọc Duy wrote: > Commit 43662b23ab (format-patch: keep cover-letter diffstat wrapped in > 72 columns - 2018-01-24) uncondtionally sets stat width to 72 when > generating diffstat for the cover letter, ignoring --stat from command > line. But it should only do so when stat width is still default > (i.e. stat_width == 0). > > In order to fix this, we should only set stat_width if stat_width is > zero. But it will never be. Commit 071dd0ba43 (format-patch: reduce > patch diffstat width to 72 - 2018-02-01) makes sure that default stat > width will be 72 (ignoring $COLUMNS, but could still be overriden by > --stat). So all we need to do here is drop the assignment. > > Reported-by: Laszlo Ersek > Helped-by: Leif Lindholm > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > builtin/log.c | 2 -- > t/t4052-stat-output.sh | 48 +- > 2 files changed, 33 insertions(+), 17 deletions(-) * This submission should have been posted as v3, not v2. V2 was posted at https://public-inbox.org/git/20181107164953.24965-1-pclo...@gmail.com/ * Comparing the patch emails, the only difference is that this version renames "expect.40" to "expect.60". This should have been mentioned in a cover letter, or in the Notes section of the current submission. * In my response to the (original) v2 posting, at https://public-inbox.org/git/f0f95dd0-1a9e-01d0-70f4-3c6d5450d...@redhat.com/ I stated that I didn't try to run the test suite, and gave my T-b (under the circumstances described there). Given that the change in v3 (= this submission) is limited to the test case, I think my T-b should have been carried forward. Thanks Laszlo > diff --git a/builtin/log.c b/builtin/log.c > index 061d4fd864..1a39c6e52a 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1009,8 +1009,6 @@ static void show_diffstat(struct rev_info *rev, > > memcpy(&opts, &rev->diffopt, sizeof(opts)); > opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT; > - opts.stat_width = MAIL_DEFAULT_WRAP; > - > diff_setup_done(&opts); > > diff_tree_oid(get_commit_tree_oid(origin), > diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh > index 6e2cf933f7..28c053849a 100755 > --- a/t/t4052-stat-output.sh > +++ b/t/t4052-stat-output.sh > @@ -44,42 +44,50 @@ show --stat > log -1 --stat > EOF > > -while read cmd args > +cat >expect.60 <<-'EOF' > + ...a | 1 + > +EOF > +cat >expect.6030 <<-'EOF' > + ...aaa | 1 + > +EOF > +cat >expect2.60 <<-'EOF' > + ...a | 1 + > + ...a | 1 + > +EOF > +cat >expect2.6030 <<-'EOF' > + ...aaa | 1 + > + ...aaa | 1 + > +EOF > +while read expect cmd args > do > - cat >expect <<-'EOF' > - ...a | 1 + > - EOF > test_expect_success "$cmd --stat=width: a long name is given more room > when the bar is short" ' > git $cmd $args --stat=40 >output && > grep " | " output >actual && > - test_cmp expect actual > + test_cmp $expect.60 actual > ' > > test_expect_success "$cmd --stat-width=width with long name" ' > git $cmd $args --stat-width=40 >output && > grep " | " output >actual && > - test_cmp expect actual > + test_cmp $expect.60 actual > ' > > - cat >expect <<-'EOF' > - ...aaa | 1 + > - EOF > test_expect_success "$cmd --stat=...,name-width with long name" ' > git $cmd $args --stat=60,30 >output && > grep " | " output >actual && > - test_cmp expect actual > + test_cmp $expect.6030 actual > ' > > test_expect_success "$cmd --stat-name-width with long name" ' > git $cmd $args --stat-name-width=30 >output && > grep " | " output >actual && > - test_cmp expect actual > + test_cmp $expect.6030 actual > ' > done <<\EOF > -format-patch -1 --stdout > -diff HEAD^ HEAD --stat > -show --stat > -log -1 --stat > +expect2 format-patch --cover-letter -1 --stdout > +expect diff HEAD^ HEAD --stat > +expect show --stat > +expect log -1 --stat > EOF > > > @@ -95,6 +103,16 @@ test_expect_success 'preparation for big change tests' ' > git commit -m message abcd > ' > > +cat >expect72 <<'EOF' > + abcd | 1000 ++ > + abcd | 1000 ++ > +EOF > +test_expect_success "format-patch --cover-letter ignores COLUMNS (big > change)" ' > + COLUMNS=200 git format-patch -1 --stdout --cover-letter >output && > + grep " | " output >actual && > + test_cmp expect72 actual > +' > + > cat >expect72 <<'EOF' > abcd | 1000
[PATCH v2] format-patch: respect --stat in cover letter's diffstat
Commit 43662b23ab (format-patch: keep cover-letter diffstat wrapped in 72 columns - 2018-01-24) uncondtionally sets stat width to 72 when generating diffstat for the cover letter, ignoring --stat from command line. But it should only do so when stat width is still default (i.e. stat_width == 0). In order to fix this, we should only set stat_width if stat_width is zero. But it will never be. Commit 071dd0ba43 (format-patch: reduce patch diffstat width to 72 - 2018-02-01) makes sure that default stat width will be 72 (ignoring $COLUMNS, but could still be overriden by --stat). So all we need to do here is drop the assignment. Reported-by: Laszlo Ersek Helped-by: Leif Lindholm Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/log.c | 2 -- t/t4052-stat-output.sh | 48 +- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 061d4fd864..1a39c6e52a 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1009,8 +1009,6 @@ static void show_diffstat(struct rev_info *rev, memcpy(&opts, &rev->diffopt, sizeof(opts)); opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT; - opts.stat_width = MAIL_DEFAULT_WRAP; - diff_setup_done(&opts); diff_tree_oid(get_commit_tree_oid(origin), diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh index 6e2cf933f7..28c053849a 100755 --- a/t/t4052-stat-output.sh +++ b/t/t4052-stat-output.sh @@ -44,42 +44,50 @@ show --stat log -1 --stat EOF -while read cmd args +cat >expect.60 <<-'EOF' + ...a | 1 + +EOF +cat >expect.6030 <<-'EOF' + ...aaa | 1 + +EOF +cat >expect2.60 <<-'EOF' + ...a | 1 + + ...a | 1 + +EOF +cat >expect2.6030 <<-'EOF' + ...aaa | 1 + + ...aaa | 1 + +EOF +while read expect cmd args do - cat >expect <<-'EOF' -...a | 1 + - EOF test_expect_success "$cmd --stat=width: a long name is given more room when the bar is short" ' git $cmd $args --stat=40 >output && grep " | " output >actual && - test_cmp expect actual + test_cmp $expect.60 actual ' test_expect_success "$cmd --stat-width=width with long name" ' git $cmd $args --stat-width=40 >output && grep " | " output >actual && - test_cmp expect actual + test_cmp $expect.60 actual ' - cat >expect <<-'EOF' -...aaa | 1 + - EOF test_expect_success "$cmd --stat=...,name-width with long name" ' git $cmd $args --stat=60,30 >output && grep " | " output >actual && - test_cmp expect actual + test_cmp $expect.6030 actual ' test_expect_success "$cmd --stat-name-width with long name" ' git $cmd $args --stat-name-width=30 >output && grep " | " output >actual && - test_cmp expect actual + test_cmp $expect.6030 actual ' done <<\EOF -format-patch -1 --stdout -diff HEAD^ HEAD --stat -show --stat -log -1 --stat +expect2 format-patch --cover-letter -1 --stdout +expect diff HEAD^ HEAD --stat +expect show --stat +expect log -1 --stat EOF @@ -95,6 +103,16 @@ test_expect_success 'preparation for big change tests' ' git commit -m message abcd ' +cat >expect72 <<'EOF' + abcd | 1000 ++ + abcd | 1000 ++ +EOF +test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" ' + COLUMNS=200 git format-patch -1 --stdout --cover-letter >output && + grep " | " output >actual && + test_cmp expect72 actual +' + cat >expect72 <<'EOF' abcd | 1000 ++ EOF -- 2.19.1.1005.gac84295441
Re: [PATCH v2] format-patch: respect --stat in cover letter's diffstat
On 11/07/18 17:49, Nguyễn Thái Ngọc Duy wrote:
> Commit 43662b23ab (format-patch: keep cover-letter diffstat wrapped in
> 72 columns - 2018-01-24) uncondtionally sets stat width to 72 when
> generating diffstat for the cover letter, ignoring --stat from command
> line. But it should only do so when stat width is still default
> (i.e. stat_width == 0).
>
> In order to fix this, we should only set stat_width if stat_width is
> zero. But it will never be. Commit 071dd0ba43 (format-patch: reduce
> patch diffstat width to 72 - 2018-02-01) makes sure that default stat
> width will be 72 (ignoring $COLUMNS, but could still be overriden by
> --stat). So all we need to do here is drop the assignment.
>
> Reported-by: Laszlo Ersek
> Helped-by: Leif Lindholm
> Signed-off-by: Nguyễn Thái Ngọc Duy
> ---
> builtin/log.c | 2 --
> t/t4052-stat-output.sh | 48 +-
> 2 files changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 061d4fd864..1a39c6e52a 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1009,8 +1009,6 @@ static void show_diffstat(struct rev_info *rev,
>
> memcpy(&opts, &rev->diffopt, sizeof(opts));
> opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> - opts.stat_width = MAIL_DEFAULT_WRAP;
> -
> diff_setup_done(&opts);
>
> diff_tree_oid(get_commit_tree_oid(origin),
Because I plan to use the patch on top of v2.19.1 (until the next major
release, v2.20, is made), that's also where I applied and tested the patch.
With master @ a4b8ab5363a3, this patch targets show_diffstat(). At
v2.19.1, commit fa5b7ea670f4 ("format-patch: allow additional generated
content in make_cover_letter()", 2018-07-23) had not occurred yet, so
there the subject code still lived in make_cover_letter(). On my end,
git-am has applied the hunk to make_cover_letter() seamlessly.
I tested the patch with "--stat=1000 --stat-graph-width=20", formatting
an edk2 series that contained commit 1ed6498c4a02
("UefiCpuPkg/CommonFeature: Skip locking when the feature is disabled",
2018-11-07). The long pathname
"UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c" is no longer
truncated in the cumulative diffstat, in the cover letter.
Tested-by: Laszlo Ersek
> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> [...]
I didn't try to run the test suite (I wasn't conscious of it anyway); I
built & installed git with
nice make -j4 prefix=... all doc info
nice make prefix=... install install-doc install-html install-info
I also wasn't watching the make log. So if those make targets don't
include the test suite, then I didn't exercise the new test case.
Thank you!
Laszlo
[PATCH v2] format-patch: respect --stat in cover letter's diffstat
Commit 43662b23ab (format-patch: keep cover-letter diffstat wrapped in 72 columns - 2018-01-24) uncondtionally sets stat width to 72 when generating diffstat for the cover letter, ignoring --stat from command line. But it should only do so when stat width is still default (i.e. stat_width == 0). In order to fix this, we should only set stat_width if stat_width is zero. But it will never be. Commit 071dd0ba43 (format-patch: reduce patch diffstat width to 72 - 2018-02-01) makes sure that default stat width will be 72 (ignoring $COLUMNS, but could still be overriden by --stat). So all we need to do here is drop the assignment. Reported-by: Laszlo Ersek Helped-by: Leif Lindholm Signed-off-by: Nguyễn Thái Ngọc Duy --- builtin/log.c | 2 -- t/t4052-stat-output.sh | 48 +- 2 files changed, 33 insertions(+), 17 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 061d4fd864..1a39c6e52a 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1009,8 +1009,6 @@ static void show_diffstat(struct rev_info *rev, memcpy(&opts, &rev->diffopt, sizeof(opts)); opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT; - opts.stat_width = MAIL_DEFAULT_WRAP; - diff_setup_done(&opts); diff_tree_oid(get_commit_tree_oid(origin), diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh index 6e2cf933f7..b1ce0d9b97 100755 --- a/t/t4052-stat-output.sh +++ b/t/t4052-stat-output.sh @@ -44,42 +44,50 @@ show --stat log -1 --stat EOF -while read cmd args +cat >expect.40 <<-'EOF' + ...a | 1 + +EOF +cat >expect.6030 <<-'EOF' + ...aaa | 1 + +EOF +cat >expect2.40 <<-'EOF' + ...a | 1 + + ...a | 1 + +EOF +cat >expect2.6030 <<-'EOF' + ...aaa | 1 + + ...aaa | 1 + +EOF +while read expect cmd args do - cat >expect <<-'EOF' -...a | 1 + - EOF test_expect_success "$cmd --stat=width: a long name is given more room when the bar is short" ' git $cmd $args --stat=40 >output && grep " | " output >actual && - test_cmp expect actual + test_cmp $expect.40 actual ' test_expect_success "$cmd --stat-width=width with long name" ' git $cmd $args --stat-width=40 >output && grep " | " output >actual && - test_cmp expect actual + test_cmp $expect.40 actual ' - cat >expect <<-'EOF' -...aaa | 1 + - EOF test_expect_success "$cmd --stat=...,name-width with long name" ' git $cmd $args --stat=60,30 >output && grep " | " output >actual && - test_cmp expect actual + test_cmp $expect.6030 actual ' test_expect_success "$cmd --stat-name-width with long name" ' git $cmd $args --stat-name-width=30 >output && grep " | " output >actual && - test_cmp expect actual + test_cmp $expect.6030 actual ' done <<\EOF -format-patch -1 --stdout -diff HEAD^ HEAD --stat -show --stat -log -1 --stat +expect2 format-patch --cover-letter -1 --stdout +expect diff HEAD^ HEAD --stat +expect show --stat +expect log -1 --stat EOF @@ -95,6 +103,16 @@ test_expect_success 'preparation for big change tests' ' git commit -m message abcd ' +cat >expect72 <<'EOF' + abcd | 1000 ++ + abcd | 1000 ++ +EOF +test_expect_success "format-patch --cover-letter ignores COLUMNS (big change)" ' + COLUMNS=200 git format-patch -1 --stdout --cover-letter >output && + grep " | " output >actual && + test_cmp expect72 actual +' + cat >expect72 <<'EOF' abcd | 1000 ++ EOF -- 2.19.1.1005.gac84295441
