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

From: Jeff King <peff@peff.net>
Date: Jan 22, 2018, 23:52 UTC
Message-ID: <20180122235202.GA26357@sigill.intra.peff.net>
In-Reply-To: <20180122123154.8301-1-pclouds@gmail.com>

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/