From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.6 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 28CC21F404 for ; Mon, 22 Jan 2018 23:52:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751145AbeAVXwF (ORCPT ); Mon, 22 Jan 2018 18:52:05 -0500 Received: from cloud.peff.net ([104.130.231.41]:53876 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751059AbeAVXwE (ORCPT ); Mon, 22 Jan 2018 18:52:04 -0500 Received: (qmail 10954 invoked by uid 109); 22 Jan 2018 23:52:05 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Mon, 22 Jan 2018 23:52:05 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 12458 invoked by uid 111); 22 Jan 2018 23:52:42 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.94) with (ECDHE-RSA-AES256-GCM-SHA384 encrypted) SMTP; Mon, 22 Jan 2018 18:52:42 -0500 Authentication-Results: peff.net; auth=none Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Mon, 22 Jan 2018 18:52:02 -0500 Date: Mon, 22 Jan 2018 18:52:02 -0500 From: Jeff King To: =?utf-8?B?Tmd1eeG7hW4gVGjDoWkgTmfhu41j?= Duy Cc: Junio C Hamano , git@vger.kernel.org Subject: Re: [PATCH] format-patch: set diffstat width to 70 instead of default 80 Message-ID: <20180122235202.GA26357@sigill.intra.peff.net> References: <20180122123154.8301-1-pclouds@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20180122123154.8301-1-pclouds@gmail.com> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org 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@coredump.intra.peff.net/