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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 19, 2018 at 5:48 AM, Jeff King <peff@xxxxxxxx> wrote:
> 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...

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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux