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@xxxxxxxxxxxxxxxxxxxxxxx/
