git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH v2] diff: add --compact-summary option to complement --stat
Date: Thu, 18 Jan 2018 13:57:16 -0500	[thread overview]
Message-ID: <CAPig+cQLgs59JYxcmK30qY307ArwqJx6pNOo95Z39_jJ9+D6+g@mail.gmail.com> (raw)
In-Reply-To: <20180118100546.32251-1-pclouds@gmail.com>

On Thu, Jan 18, 2018 at 5:05 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> [...]
> The new option --compact-summary implements this with a tweak to support
> mode change, which is shown in --summary too.
>
> For mode changes, executable bit is denoted as "M+x" or "M-x" when it's
> added or removed respectively. The same for when a regular file is
> replaced with a symlink "M+l" or the other way "M-l". This also applies
> to new files. New regulare files are "A", while new executable files or

s/regulare/regular/

> symlinks are "A+x" or "A+l".
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> @@ -188,6 +188,17 @@ and accumulating child directory counts in the parent directories:
> +--compact-summary::
> +       Output a condensed summary of extended header information in
> +       front of the file name part of diffstat. This option is
> +       ignored if --stat is not specified.

Rather than ignoring this option if --stat is not specified, a
different approach would be for --compact-summary to imply --stat.

Also, per documentation:

    --stat[=<width>[,<name-width>[,<count>]]]::

    These parameters can also be set individually with `--stat-width=<width>`,
    `--stat-name-width=<name-width>` and `--stat-count=<count>`.

One wonders if "compact" could be another modifier recognized by --stat.

(Genuine questions/observations; I haven't formed strong opinions either way.)

> diff --git a/diff.c b/diff.c
> @@ -2287,6 +2289,18 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
> +       for (i = 0; (i < count) && (i < data->nr); i++) {

Noisy extra parentheses...

    for (i = 0; i < count && i < data->nr; i++) {

perhaps? (Not at all worth a re-roll.)

> +               const struct diffstat_file *file = data->files[i];
> +               int len;
> +
> +               if (!file->status_code)
> +                       continue;
> +               len = strlen(file->status_code) + 1;

The +1 is for the space following the status code? (Reading ahead,
that seems to be the case.)

    len = strlen(file->status_code) + strlen(" ");

perhaps? (Probably not worth a re-roll.)

> +               if (len > max_status_len)
> +                       max_status_len = len;
> +       }
> +
> @@ -2383,6 +2397,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>                       options->stat_name_width < max_len) ?
>                 options->stat_name_width : max_len;
>
> +       name_width += max_status_len;

I wonder if it would be clearer to account for the space after the the
status code here rather than above when it was not obvious what +1 was
for.

    name_width += max_status_len + strlen(" ");

(and drop the earlier +1)

  reply	other threads:[~2018-01-18 18:57 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-13 13:22 [PATCH/RFC] diff: add --compact-summary option to complement --stat Nguyễn Thái Ngọc Duy
2018-01-13 18:37 ` Philip Oakley
2018-01-14  9:37 ` Simon Ruderich
2018-01-14 10:24   ` Duy Nguyen
2018-01-18 10:05 ` [PATCH v2] " Nguyễn Thái Ngọc Duy
2018-01-18 18:57   ` Eric Sunshine [this message]
2018-01-19  0:01     ` Duy Nguyen
2018-01-18 21:23   ` Ævar Arnfjörð Bjarmason
2018-01-19  0:06     ` Duy Nguyen
2018-01-18 22:48   ` Jeff King
2018-01-19  0:26     ` Duy Nguyen
2018-01-19 21:52       ` Jeff King
2018-01-19 21:20   ` Junio C Hamano
2018-01-19 21:53     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPig+cQLgs59JYxcmK30qY307ArwqJx6pNOo95Z39_jJ9+D6+g@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).