git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Duy Nguyen <pclouds@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v2] diff: add --compact-summary option to complement --stat
Date: Fri, 19 Jan 2018 07:26:28 +0700	[thread overview]
Message-ID: <CACsJy8CPHk+aXHr-mkHZi27s=c3+ny8D9CSuzOSO8PweviBcqQ@mail.gmail.com> (raw)
In-Reply-To: <20180118224814.GA8473@sigill.intra.peff.net>

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

  reply	other threads:[~2018-01-19  0:27 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
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 [this message]
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='CACsJy8CPHk+aXHr-mkHZi27s=c3+ny8D9CSuzOSO8PweviBcqQ@mail.gmail.com' \
    --to=pclouds@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /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).