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

From: Duy Nguyen <pclouds@gmail.com>
Date: Jan 19, 2018, 00:26 UTC
Message-ID: <CACsJy8CPHk+aXHr-mkHZi27s=c3+ny8D9CSuzOSO8PweviBcqQ@mail.gmail.com>
In-Reply-To: <20180118224814.GA8473@sigill.intra.peff.net>

On Fri, Jan 19, 2018 at 5:48 AM, Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

> On Thu, Jan 18, 2018 at 05:05:46PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> This is partly inspired by gerrit web interface which shows diffstat
>> like this, e.g. with commit 0433d533f1 (notice the "A" column on the
>> third line):
>>
>>      Documentation/merge-config.txt     |  4 +
>>      builtin/merge.c                    |  2 +
>>    A t/t5573-pull-verify-signatures.sh  | 81 ++++++++++++++++++
>>      t/t7612-merge-verify-signatures.sh | 45 ++++++++++
>>    4 files changed, 132 insertions(+)
>
> I like the general concept. Perusing "git log" output, though, it felt
> like the summary column was very close to the filenames. What do you
> think of putting it after the "|", where it is only close to a number?
>
> Something like the patch below (on top of yours, but it probably needs
> tweaked further for graph_width), which gives:
>
>    t/t5573-pull-verify-signatures.sh | A+x  78 ++++++++++++++++++++++++++++
>
> (I know this is a bikeshed, so I'm perfectly willing to take "yuck, I
> don't like that as well" as a response).

The position of A+x column is exactly where gerrit put it. Though web pages have more flexibility than our terminal console so its position does not have to be the same. I'm just throwing options out there

For many years I have this instead

 t/t5573-pull-verify-signatures.sh (new +x) | 81 ++++++++++++++++++++

Another option is just wrap the code in [] to make it look like check boxes. But that wastes two more columns

       builtin/merge.c                    |  2 +
 [A+x] t/t5573-pull-verify-signatures.sh  | 81 ++++++++++++++++++++++++
       t/t7612-merge-verify-signatures.sh | 45 +++++++++++++

Back to your suggestion, I kinda like the closeness between the +/- count and "|" though. The output on 10c78a162f is like this, which makes "A" looks a bit umm.. disconnected from the path name?

  Documentation/RelNotes/2.14.0.txt | A  97 +++++++++++++++++++++++++++
  GIT-VERSION-GEN                   |     2 +-
  RelNotes                          |     2 +-

Another way is just separate the status code from file name

 A | Documentation/RelNotes/2.14.0.txt | 97 +++++++++++++++++++++++++++
   | GIT-VERSION-GEN                   |  2 +-
   | RelNotes                          |  2 +-

Last note. With colored diffstat, we should be able to use a separate color (or something in the +/- part) to denote new/deleted files. I didn't think about this...

Show 5 quoted lines

>> The new option --compact-summary implements this with a tweak to support
>> mode change, which is shown in --summary too.
>
> One thing I noticed is that --compact-summary by itself does nothing.
> Should it imply --stat?

It might go with --numstat or --dirstat in future too. Didn't really think hard about this yet. But I probably will go with Eric suggestion and keep this in --stat=.... unless it really makes sense to have something like this in --numstat or --dirstat.

-- 
Duy