Re: [PATCH v2] diff: add --compact-summary option to complement --stat

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 18, 2018, 18:57 UTC
Message-ID: <CAPig+cQLgs59JYxcmK30qY307ArwqJx6pNOo95Z39_jJ9+D6+g@mail.gmail.com>
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:

Show 8 quoted lines

> [...]
> 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/

Show 10 quoted lines

> 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.)

Show 6 quoted lines

> +               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.)

Show 9 quoted lines

> +               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)