[PATCH] format-patch: set diffstat width to 70 instead of default 80

[PATCH] format-patch: set diffstat width to 70 instead of default 80

[Nguyễn Thái Ngọc Duy]

Patches or cover letters generated by format-patch are meant to be
exchanged as emails, most of the time. And since it's generally agreed
that text in mails should be wrapped around 70 columns or so, make sure
these diffstat follow the convention.

I noticed this when I quoted a diffstat line [1]. Should we do something
like this? diffstat is rarely quoted though so perhaps the stat width
should be something like 75.

t4052 fails but I don't think it's worth fixing until it's clear if it's
worth doing this.

[1] https://public-inbox.org/git/20180122121426.GD5980@ash/T/#u

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 14fdf39165..6be79656c5 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	memcpy(&opts, &rev->diffopt, sizeof(opts));
 	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
+	opts.diffopt.stat_width = 70;
 
 	diff_setup_done(&opts);
 
@@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		die(_("--check does not make sense"));
 
 	if (!use_patch_format &&
-		(!rev.diffopt.output_format ||
-		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
+	    (!rev.diffopt.output_format ||
+	     rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
+		if (!rev.diffopt.stat_width)
+			rev.diffopt.stat_width = 70;
+	}
 
 	/* Always generate a patch */
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
-- 
2.16.0.47.g3d9b0fac3a

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Patches or cover letters generated by format-patch are meant to be
> exchanged as emails, most of the time. And since it's generally agreed
> that text in mails should be wrapped around 70 columns or so, make sure
> these diffstat follow the convention.
>
> I noticed this when I quoted a diffstat line [1]. Should we do something
> like this? diffstat is rarely quoted though so perhaps the stat width
> should be something like 75.
>
> t4052 fails but I don't think it's worth fixing until it's clear if it's
> worth doing this.

I guess you meant this as an RFC/PATCH; FWIW, I personally am in
favor of this change.

>
> [1] https://public-inbox.org/git/20180122121426.GD5980@ash/T/#u
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/log.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 14fdf39165..6be79656c5 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  
>  	memcpy(&opts, &rev->diffopt, sizeof(opts));
>  	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> +	opts.diffopt.stat_width = 70;
>  
>  	diff_setup_done(&opts);
>  
> @@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		die(_("--check does not make sense"));
>  
>  	if (!use_patch_format &&
> -		(!rev.diffopt.output_format ||
> -		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> +	    (!rev.diffopt.output_format ||
> +	     rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {
>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
> +		if (!rev.diffopt.stat_width)
> +			rev.diffopt.stat_width = 70;
> +	}
>  
>  	/* Always generate a patch */
>  	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Jeff King]

On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Patches or cover letters generated by format-patch are meant to be
> exchanged as emails, most of the time. And since it's generally agreed
> that text in mails should be wrapped around 70 columns or so, make sure
> these diffstat follow the convention.
> 
> I noticed this when I quoted a diffstat line [1]. Should we do something
> like this? diffstat is rarely quoted though so perhaps the stat width
> should be something like 75.

I think the general idea is sensible. Somewhere I picked up "72" as the
right size for email lines to accommodate quoting, but I'm pretty sure
you could justify any number between 70 and 75. :)

A few thoughts looking at the patch:

> diff --git a/builtin/log.c b/builtin/log.c
> index 14fdf39165..6be79656c5 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  
>  	memcpy(&opts, &rev->diffopt, sizeof(opts));
>  	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> +	opts.diffopt.stat_width = 70;
>  
>  	diff_setup_done(&opts);

I wondered how this should interact with any config, but I don't think
you can actually configure the stat-width. You _can_ configure
diff.statgraphwidth, though, which seems like a funny inconsistency.

Anyway, I'm not it would make sense to prefer any kind of generic
diff.statwidth to this value. The point is that the context here has to
do with emails, not just terminals, and the rules are different. So I
think you'd need format.statwidth or something. I'm perfectly willing to
punt on that until somebody actually cares.

> @@ -1611,9 +1612,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		die(_("--check does not make sense"));
>  
>  	if (!use_patch_format &&
> -		(!rev.diffopt.output_format ||
> -		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
> +	    (!rev.diffopt.output_format ||
> +	     rev.diffopt.output_format == DIFF_FORMAT_PATCH)) {
>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
> +		if (!rev.diffopt.stat_width)
> +			rev.diffopt.stat_width = 70;
> +	}

Hmm, so if I say:

  git format-patch --stat --patch

I'd get the larger default? That seems kind of funny. Should this
stat_width setting be outside of this conditional (and if the user
asks for a non-stat format, it would just be ignored)?

-Peff

PS I had a funny feeling that this had come up before not due to
   quoting, but just due to people with enormous terminals generating
   too-long lines. But I couldn't find any discussion, and my
   (admittedly brief) reading of the code is that we'd actually respect
   the terminal size by default.

   While digging, I did find this discussion, though:

     https://public-inbox.org/git/20080403102214.GA23121@coredump.intra.peff.net/

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Jeff King jotted:

> On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> +	opts.diffopt.stat_width = 70;
>>
>>  	diff_setup_done(&opts);
>
> I wondered how this should interact with any config, but I don't think
> you can actually configure the stat-width. You _can_ configure
> diff.statgraphwidth, though, which seems like a funny inconsistency.

Isn't the numeric argument to --stat (this works with/without this
patch):

    $ git format-patch -10 --stdout --stat=30 -- t|grep -m 5 ' | '
     ...submodule-update.sh | 1 +
     ...ule-update.sh | 14 ++++++
     ...-addresses.sh | 27 ---
     t/t9000/test.pl  | 67 ------
     ...send-email.sh | 19 ++++++
    $ git format-patch -10 --stdout --stat=90 -- t|grep -m 5 ' | '
     t/lib-submodule-update.sh | 1 +
     t/lib-submodule-update.sh | 14 ++++++++++++++
     t/t9000-addresses.sh | 27 -------------------------
     t/t9000/test.pl      | 67 --------------------------------------------------------------
     t/t9001-send-email.sh | 19 +++++++++++++++++++

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Jeff King]

On Tue, Jan 23, 2018 at 01:10:43AM +0100, Ævar Arnfjörð Bjarmason wrote:

> 
> On Mon, Jan 22 2018, Jeff King jotted:
> 
> > On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:
> >> +	opts.diffopt.stat_width = 70;
> >>
> >>  	diff_setup_done(&opts);
> >
> > I wondered how this should interact with any config, but I don't think
> > you can actually configure the stat-width. You _can_ configure
> > diff.statgraphwidth, though, which seems like a funny inconsistency.
> 
> Isn't the numeric argument to --stat (this works with/without this
> patch):
> 
>     $ git format-patch -10 --stdout --stat=30 -- t|grep -m 5 ' | '
>      ...submodule-update.sh | 1 +
>      ...ule-update.sh | 14 ++++++
>      ...-addresses.sh | 27 ---
>      t/t9000/test.pl  | 67 ------
>      ...send-email.sh | 19 ++++++
>     $ git format-patch -10 --stdout --stat=90 -- t|grep -m 5 ' | '
>      t/lib-submodule-update.sh | 1 +
>      t/lib-submodule-update.sh | 14 ++++++++++++++
>      t/t9000-addresses.sh | 27 -------------------------
>      t/t9000/test.pl      | 67 --------------------------------------------------------------
>      t/t9001-send-email.sh | 19 +++++++++++++++++++

Yeah, I meant by actual on-disk config. I didn't actually look at the
patch closely, but I assumed that "format-patch --stat=90" would still
override this (if not, then I think that would be a bug).

-Peff

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Duy Nguyen]

On Tue, Jan 23, 2018 at 6:52 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> Patches or cover letters generated by format-patch are meant to be
>> exchanged as emails, most of the time. And since it's generally agreed
>> that text in mails should be wrapped around 70 columns or so, make sure
>> these diffstat follow the convention.
>>
>> I noticed this when I quoted a diffstat line [1]. Should we do something
>> like this? diffstat is rarely quoted though so perhaps the stat width
>> should be something like 75.
>
> I think the general idea is sensible. Somewhere I picked up "72" as the
> right size for email lines to accommodate quoting, but I'm pretty sure
> you could justify any number between 70 and 75. :)

I think it's easy to settle on 72 because cover letter's shortlog
already wraps at 72 columns. No point in introducing another number
here.

> PS I had a funny feeling that this had come up before not due to
>    quoting, but just due to people with enormous terminals generating
>    too-long lines. But I couldn't find any discussion, and my
>    (admittedly brief) reading of the code is that we'd actually respect
>    the terminal size by default.

Yeah, there are tests to check that we do ignore terminal size too.
-- 
Duy

Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 22 2018, Nguyễn Thái Ngọc Duy jotted:

> diff --git a/builtin/log.c b/builtin/log.c
> index 14fdf39165..6be79656c5 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1061,6 +1061,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
>  
>  	memcpy(&opts, &rev->diffopt, sizeof(opts));
>  	opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> +	opts.diffopt.stat_width = 70;
>  
>  	diff_setup_done(&opts);

There's no opts.diffopt. Presumably this should be squashed on top:

    -       opts.diffopt.stat_width = 70;
    +       rev->diffopt.stat_width = 70;

[PATCH v2 0/2] wrap format-patch diffstats around 72 columns

[Nguyễn Thái Ngọc Duy]

Like v1, these changes keep diffstat generated by format-patch in 72
columns. This constant is already used in the code, so it's a bit
better than my random "70 or 75" value.

Granted these hard coded values (both 80 and 72) are not really nice.
But I would wait for somebody to say "I need or want this" before I
add code to make the default configurable.

Nguyễn Thái Ngọc Duy (2):
  format-patch: keep cover-letter diffstat wrapped in 72 columns
  format-patch: reduce patch diffstat width to 72

 builtin/log.c          |  7 ++++++-
 t/t4052-stat-output.sh | 28 ++++++++++++++--------------
 2 files changed, 20 insertions(+), 15 deletions(-)

-- 
2.16.1.200.g71ee9f6994

[PATCH v2 1/2] format-patch: keep cover-letter diffstat wrapped in 72 columns

[Nguyễn Thái Ngọc Duy]

We already wrap shortlog around 72 columns in cover letters. Do the same
for diffstat (also in cover letters).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 46b4ca13e5..96af897403 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -29,6 +29,8 @@
 #include "gpg-interface.h"
 #include "progress.h"
 
+#define MAIL_DEFAULT_WRAP 72
+
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
 
@@ -1044,7 +1046,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	shortlog_init(&log);
 	log.wrap_lines = 1;
-	log.wrap = 72;
+	log.wrap = MAIL_DEFAULT_WRAP;
 	log.in1 = 2;
 	log.in2 = 4;
 	log.file = rev->diffopt.file;
@@ -1061,6 +1063,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	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);
 
-- 
2.16.1.200.g71ee9f6994

[PATCH v2 2/2] format-patch: reduce patch diffstat width to 72

[Nguyễn Thái Ngọc Duy]

Patches generated by format-patch are meant to be exchanged as emails,
most of the time. And since it's generally agreed that text in mails
should be wrapped around 70 columns or so, make sure these diffstat
follow the convention (especially when used with --cover-letter since we
already defaults to wrapping 72 columns). The default can still be
overriden with command line options.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c          |  2 ++
 t/t4052-stat-output.sh | 28 ++++++++++++++--------------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 96af897403..94ee177d56 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1617,6 +1617,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		(!rev.diffopt.output_format ||
 		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
+	if (!rev.diffopt.stat_width)
+		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
 
 	/* Always generate a patch */
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 9f563db20a..1e62333b46 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -60,7 +60,7 @@ do
 		test_cmp expect actual
 	'
 done <<\EOF
-format-patch -1 --stdout
+format-patch --stat=80 -1 --stdout
 diff HEAD^ HEAD --stat
 show --stat
 log -1 --stat
@@ -79,11 +79,11 @@ test_expect_success 'preparation for big change tests' '
 	git commit -m message abcd
 '
 
-cat >expect80 <<'EOF'
- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+cat >expect72 <<'EOF'
+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
-cat >expect80-graph <<'EOF'
-|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+cat >expect72-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 cat >expect200 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
@@ -107,7 +107,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
 respects expect200 show --stat
 respects expect200 log -1 --stat
@@ -135,7 +135,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect40 diff HEAD^ HEAD --stat
 respects expect40 show --stat
 respects expect40 log -1 --stat
@@ -163,7 +163,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect40 diff HEAD^ HEAD --stat
 respects expect40 show --stat
 respects expect40 log -1 --stat
@@ -250,11 +250,11 @@ show --stat
 log -1 --stat
 EOF
 
-cat >expect80 <<'EOF'
- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
+cat >expect72 <<'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++
 EOF
-cat >expect80-graph <<'EOF'
-|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
+cat >expect72-graph <<'EOF'
+|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++
 EOF
 cat >expect200 <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
@@ -278,7 +278,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
 respects expect200 show --stat
 respects expect200 log -1 --stat
@@ -308,7 +308,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect1 diff HEAD^ HEAD --stat
 respects expect1 show --stat
 respects expect1 log -1 --stat
-- 
2.16.1.200.g71ee9f6994

Re: [PATCH v2 2/2] format-patch: reduce patch diffstat width to 72

[Jeff King]

On Thu, Jan 25, 2018 at 06:59:27PM +0700, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> index 9f563db20a..1e62333b46 100755
> --- a/t/t4052-stat-output.sh
> +++ b/t/t4052-stat-output.sh
> @@ -60,7 +60,7 @@ do
>  		test_cmp expect actual
>  	'
>  done <<\EOF
> -format-patch -1 --stdout
> +format-patch --stat=80 -1 --stdout
>  diff HEAD^ HEAD --stat
>  show --stat
>  log -1 --stat

This hunk confused me. I think what is going on is this:

  - we have a loop that runs the same test on several commands

  - that loop expects format-patch, diff, etc, to have the same output

  - now that format-patch differs from the other commands in its default
    length, we need to use a manual --stat-width to get identical output

It seems like that kind of nullifies the point of some of the tests in
the loop, though, since they are meant to check the behavior without
--stat.

OTOH, I think that case is tested later (in the other tests you
adjusted). So I guess these tests are just covering the "name vs bar
length" part?

-Peff

Re: [PATCH v2 2/2] format-patch: reduce patch diffstat width to 72

[Duy Nguyen]

On Sat, Jan 27, 2018 at 11:47 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 25, 2018 at 06:59:27PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
>> index 9f563db20a..1e62333b46 100755
>> --- a/t/t4052-stat-output.sh
>> +++ b/t/t4052-stat-output.sh
>> @@ -60,7 +60,7 @@ do
>>               test_cmp expect actual
>>       '
>>  done <<\EOF
>> -format-patch -1 --stdout
>> +format-patch --stat=80 -1 --stdout
>>  diff HEAD^ HEAD --stat
>>  show --stat
>>  log -1 --stat
>
> This hunk confused me. I think what is going on is this:
>
>   - we have a loop that runs the same test on several commands
>
>   - that loop expects format-patch, diff, etc, to have the same output
>
>   - now that format-patch differs from the other commands in its default
>     length, we need to use a manual --stat-width to get identical output
>
> It seems like that kind of nullifies the point of some of the tests in
> the loop, though, since they are meant to check the behavior without
> --stat.
>
> OTOH, I think that case is tested later (in the other tests you
> adjusted). So I guess these tests are just covering the "name vs bar
> length" part?

My bad. My thought was.. "Hmm.. I would need to take format-patch out
out the loop since it won't match exactly 80 columns anymore. That's a
lot of work. Hey how about using this opportunity to test that --stat=
can still override default settings?" I didn't realize the follow
tests in that loop set stat width. I'll take format-patch out of the
loop and deal with it separately.
-- 
Duy

Re: [PATCH v2 0/2] wrap format-patch diffstats around 72 columns

[Jeff King]

On Thu, Jan 25, 2018 at 06:59:25PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Like v1, these changes keep diffstat generated by format-patch in 72
> columns. This constant is already used in the code, so it's a bit
> better than my random "70 or 75" value.
> 
> Granted these hard coded values (both 80 and 72) are not really nice.
> But I would wait for somebody to say "I need or want this" before I
> add code to make the default configurable.
> 
> Nguyễn Thái Ngọc Duy (2):
>   format-patch: keep cover-letter diffstat wrapped in 72 columns
>   format-patch: reduce patch diffstat width to 72

This looks OK to me. There was one head-scratcher in the second patch
which I mentioned, but I think it's probably OK.

I also notice that we have no tests for diffstat on a cover letter,
which might be worth addressing.

-Peff

[PATCH v3 0/2] wrap format-patch diffstats around 72 columns

[Nguyễn Thái Ngọc Duy]

v3 fixes tests in 2/2 that I overlooked (but Jeff didn't). Interdiff:

diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 1e62333b46..6e2cf933f7 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -19,17 +19,33 @@ test_expect_success 'preparation' '
 	git commit -m message "$name"
 '
 
+cat >expect72 <<-'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
+EOF
+test_expect_success "format-patch: small change with long name gives more space to the name" '
+	git format-patch -1 --stdout >output &&
+	grep " | " output >actual &&
+	test_cmp expect72 actual
+'
+
 while read cmd args
 do
-	cat >expect <<-'EOF'
+	cat >expect80 <<-'EOF'
 	 ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
 	EOF
 	test_expect_success "$cmd: small change with long name gives more space to the name" '
 		git $cmd $args >output &&
 		grep " | " output >actual &&
-		test_cmp expect actual
+		test_cmp expect80 actual
 	'
+done <<\EOF
+diff HEAD^ HEAD --stat
+show --stat
+log -1 --stat
+EOF
 
+while read cmd args
+do
 	cat >expect <<-'EOF'
 	 ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
 	EOF
@@ -60,7 +76,7 @@ do
 		test_cmp expect actual
 	'
 done <<\EOF
-format-patch --stat=80 -1 --stdout
+format-patch -1 --stdout
 diff HEAD^ HEAD --stat
 show --stat
 log -1 --stat


Nguyễn Thái Ngọc Duy (2):
  format-patch: keep cover-letter diffstat wrapped in 72 columns
  format-patch: reduce patch diffstat width to 72

 builtin/log.c          |  7 ++++++-
 t/t4052-stat-output.sh | 46 ++++++++++++++++++++++++++++--------------
 2 files changed, 37 insertions(+), 16 deletions(-)

-- 
2.16.1.205.g271f633410

[PATCH v3 1/2] format-patch: keep cover-letter diffstat wrapped in 72 columns

[Nguyễn Thái Ngọc Duy]

We already wrap shortlog around 72 columns in cover letters. Do the same
for diffstat (also in cover letters).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 46b4ca13e5..96af897403 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -29,6 +29,8 @@
 #include "gpg-interface.h"
 #include "progress.h"
 
+#define MAIL_DEFAULT_WRAP 72
+
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
 
@@ -1044,7 +1046,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	shortlog_init(&log);
 	log.wrap_lines = 1;
-	log.wrap = 72;
+	log.wrap = MAIL_DEFAULT_WRAP;
 	log.in1 = 2;
 	log.in2 = 4;
 	log.file = rev->diffopt.file;
@@ -1061,6 +1063,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 
 	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);
 
-- 
2.16.1.205.g271f633410

[PATCH v3 2/2] format-patch: reduce patch diffstat width to 72

[Nguyễn Thái Ngọc Duy]

Patches generated by format-patch are meant to be exchanged as emails,
most of the time. And since it's generally agreed that text in mails
should be wrapped around 70 columns or so, make sure these diffstat
follow the convention (especially when used with --cover-letter since we
already defaults to wrapping 72 columns). The default can still be
overriden with command line options.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/log.c          |  2 ++
 t/t4052-stat-output.sh | 46 ++++++++++++++++++++++++++++--------------
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 96af897403..94ee177d56 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1617,6 +1617,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		(!rev.diffopt.output_format ||
 		 rev.diffopt.output_format == DIFF_FORMAT_PATCH))
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
+	if (!rev.diffopt.stat_width)
+		rev.diffopt.stat_width = MAIL_DEFAULT_WRAP;
 
 	/* Always generate a patch */
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index 9f563db20a..6e2cf933f7 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -19,17 +19,33 @@ test_expect_success 'preparation' '
 	git commit -m message "$name"
 '
 
+cat >expect72 <<-'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
+EOF
+test_expect_success "format-patch: small change with long name gives more space to the name" '
+	git format-patch -1 --stdout >output &&
+	grep " | " output >actual &&
+	test_cmp expect72 actual
+'
+
 while read cmd args
 do
-	cat >expect <<-'EOF'
+	cat >expect80 <<-'EOF'
 	 ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
 	EOF
 	test_expect_success "$cmd: small change with long name gives more space to the name" '
 		git $cmd $args >output &&
 		grep " | " output >actual &&
-		test_cmp expect actual
+		test_cmp expect80 actual
 	'
+done <<\EOF
+diff HEAD^ HEAD --stat
+show --stat
+log -1 --stat
+EOF
 
+while read cmd args
+do
 	cat >expect <<-'EOF'
 	 ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1 +
 	EOF
@@ -79,11 +95,11 @@ test_expect_success 'preparation for big change tests' '
 	git commit -m message abcd
 '
 
-cat >expect80 <<'EOF'
- abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+cat >expect72 <<'EOF'
+ abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
-cat >expect80-graph <<'EOF'
-|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+cat >expect72-graph <<'EOF'
+|  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 EOF
 cat >expect200 <<'EOF'
  abcd | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
@@ -107,7 +123,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
 respects expect200 show --stat
 respects expect200 log -1 --stat
@@ -135,7 +151,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect40 diff HEAD^ HEAD --stat
 respects expect40 show --stat
 respects expect40 log -1 --stat
@@ -163,7 +179,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect40 diff HEAD^ HEAD --stat
 respects expect40 show --stat
 respects expect40 log -1 --stat
@@ -250,11 +266,11 @@ show --stat
 log -1 --stat
 EOF
 
-cat >expect80 <<'EOF'
- ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
+cat >expect72 <<'EOF'
+ ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++
 EOF
-cat >expect80-graph <<'EOF'
-|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 ++++++++++++++++++++
+cat >expect72-graph <<'EOF'
+|  ...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++
 EOF
 cat >expect200 <<'EOF'
  aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | 1000 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
@@ -278,7 +294,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect200 diff HEAD^ HEAD --stat
 respects expect200 show --stat
 respects expect200 log -1 --stat
@@ -308,7 +324,7 @@ do
 		test_cmp "$expect-graph" actual
 	'
 done <<\EOF
-ignores expect80 format-patch -1 --stdout
+ignores expect72 format-patch -1 --stdout
 respects expect1 diff HEAD^ HEAD --stat
 respects expect1 show --stat
 respects expect1 log -1 --stat
-- 
2.16.1.205.g271f633410

Re: [PATCH v3 0/2] wrap format-patch diffstats around 72 columns

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> v3 fixes tests in 2/2 that I overlooked (but Jeff didn't). Interdiff:
> ...
> Nguyễn Thái Ngọc Duy (2):
>   format-patch: keep cover-letter diffstat wrapped in 72 columns
>   format-patch: reduce patch diffstat width to 72

Thanks, will replace.  I think we are pretty in good shape with
this change now.