threads / patch / 47667

patchformat-patch: set diffstat width to 70 instead of default 80

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

## tl;dr

17 messages between Jan 22, 2018 and Feb 2, 2018. Diffs are folded; open one to read it.

replies: 16people: 4as markdown or json

Nguyễn Thái Ngọc Duy· Jan 22, 2018, 12:31 UTC · lore

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(-)
Show changes to builtin/log.c +6 −2
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
Junio C Hamano· Jan 22, 2018, 23:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 11 quoted lines
> 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.

Show 35 quoted lines
>
> [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;
Jeff King· Jan 22, 2018, 23:52 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Mon, Jan 22, 2018 at 07:31:54PM +0700, Nguyễn Thái Ngọc Duy wrote:
Show 8 quoted lines
> 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:
Show 11 quoted lines
> 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.

Show 12 quoted lines
> @@ -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/
Ævar Arnfjörð Bjarmason· Jan 23, 2018, 00:10 UTC · re: Jeff King · lore

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

On Mon, Jan 22 2018, Jeff King jotted:
Show 8 quoted lines
> 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 +++++++++++++++++++
Jeff King· Jan 23, 2018, 00:12 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Tue, Jan 23, 2018 at 01:10:43AM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 27 quoted lines
> 
> 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
Duy Nguyen· Jan 23, 2018, 02:42 UTC · re: Jeff King · lore

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

On Tue, Jan 23, 2018 at 6:52 AM, Jeff King <peff@peff.net> wrote:
Show 14 quoted lines
> 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.

Show 5 quoted lines
> 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
Ævar Arnfjörð Bjarmason· Jan 23, 2018, 00:08 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Mon, Jan 22 2018, Nguyễn Thái Ngọc Duy jotted:
Show 11 quoted lines
> 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;
Nguyễn Thái Ngọc Duy· Jan 25, 2018, 11:59 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Nguyễn Thái Ngọc Duy· Jan 25, 2018, 11:59 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to builtin/log.c +4 −1
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
Nguyễn Thái Ngọc Duy· Jan 25, 2018, 11:59 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +16 −14

builtin/log.c, t/t4052-stat-output.sh

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
Jeff King· Jan 27, 2018, 16:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Thu, Jan 25, 2018 at 06:59:27PM +0700, Nguyễn Thái Ngọc Duy wrote:
Show 13 quoted lines
> 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
Duy Nguyen· Jan 30, 2018, 10:22 UTC · re: Jeff King · lore

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

On Sat, Jan 27, 2018 at 11:47 PM, Jeff King <peff@peff.net> wrote:
Show 32 quoted lines
> 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
Jeff King· Jan 27, 2018, 16:48 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

On Thu, Jan 25, 2018 at 06:59:25PM +0700, Nguyễn Thái Ngọc Duy wrote:
Show 11 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Feb 1, 2018, 12:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

v3 fixes tests in 2/2 that I overlooked (but Jeff didn't). Interdiff:
Show changes to t/t4052-stat-output.sh +19 −3
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
Nguyễn Thái Ngọc Duy· Feb 1, 2018, 12:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to builtin/log.c +4 −1
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
Nguyễn Thái Ngọc Duy· Feb 1, 2018, 12:47 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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(-)
Show changes to 2 files +33 −15

builtin/log.c, t/t4052-stat-output.sh

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
Junio C Hamano· Feb 2, 2018, 18:42 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 5 quoted lines
> 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.

← back to recent threads