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  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

[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();
> >
> > 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

[Æ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();
>
> 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

[Æ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(, >diffopt, sizeof(opts));
>   opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> + opts.diffopt.stat_width = 70;
>  
>   diff_setup_done();

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

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

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(, >diffopt, sizeof(opts));
>   opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> + opts.diffopt.stat_width = 70;
>  
>   diff_setup_done();

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.ga23...@coredump.intra.peff.net/

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   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 
> ---
>  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(, >diffopt, sizeof(opts));
>   opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
> + opts.diffopt.stat_width = 70;
>  
>   diff_setup_done();
>  
> @@ -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;

[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 
---
 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(, >diffopt, sizeof(opts));
opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
+   opts.diffopt.stat_width = 70;
 
diff_setup_done();
 
@@ -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