[PATCH] format-patch: set diffstat width to 70 instead of default 80
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(&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
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(&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
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.ga23...@coredump.intra.peff.net/
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: > 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;
Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80
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
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
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