patch--count feature for git shortlog

This is a new feature for short log, which lets you count commits on a per file or repository basis easily.

Currently if you want a total count of commits with shortlog, you would need to add up each authors commits after using --summary. This adds a -N / --count option to shortlog for this purpose. I also created a test. It's not a great test, but it works.

I'm sure I've make a beginner mistake in some part of the process; I've done my best to follow documentation, so please be explicit when you tell me I've done something wrong.

Thanks, Lawrence Siebert

Lawrence Siebert (2):
  shortlog: -N/--count option
  shortlog: added --count test

 builtin/shortlog.c  | 14 +++++++++-----
 shortlog.h          |  1 +
 t/t4201-shortlog.sh |  5 +++++
 3 files changed, 15 insertions(+), 5 deletions(-)

--summary is per author --count counts all

Signed-off-by: Lawrence Siebert <lawrencesiebert@gmail.com>
---
 builtin/shortlog.c | 14 +++++++++-----
 shortlog.h         |  1 +
 2 files changed, 10 insertions(+), 5 deletions(-)

Signed-off-by: Lawrence Siebert <lawrencesiebert@gmail.com>
---
 t/t4201-shortlog.sh | 5 +++++
 1 file changed, 5 insertions(+)

Lawrence Siebert <lawrencesiebert@gmail.com> writes:

  
The standard way to do that is

    git log --oneline ... whatever other args ... | wc -l

or more kosher from a script

    git rev-list ... whatever other args ... | wc -l

Adding an option to ignore per-author breakdown smells backwards. The whole point of shortlog is to give you the commit stats broken out for each author. I guess it is not so far-fetched to add an option to "git log" to only show the number of commits that would be output, if you really wanted to avoid "| wc -l", but this option does not belong to shortlog.

And it certainly does not deserve a short-and-sweet single letter option "-N", I think.

Junio,

I appreciate your help. Okay, That all makes sense.

I would note that something like:
 git shortlog -s "$FILENAME:  | cut -f 1 | paste -sd+ - | bc

seems like it run much faster then:

 git log --oneline "$FILENAME" | wc -l

Which was why I was looking at shortlog. I was using it to sort files by commit count when provided a list of files, which git rev-list doesn't really work for.

Anyway I can try and put it in log proper, if you think that's the best place.

Thank you, Lawrence Siebert

Lawrence Siebert <lawrencesiebert@gmail.com> writes:

What makes you say rev-list does not work (perhaps 'really' is the key word there?)

    git rev-list --no-merges maint..master -- Makefile
    git shortlog --no-merges maint..master -- Makefile
    git log --oneline --no-merges maint..master -- Makefile

all gives me results that are consistent among them.

My apologies, I misunderstood and thought rev-list didn't take filenames.

Lawrence Siebert

On Mon, Jun 29, 2015 at 10:04 AM, Junio C Hamano <gitster@pobox.com> wrote:

Hi Lawrence,

On 2015-06-29 18:46, Lawrence Siebert wrote:

How does it compare to `git rev-list -- "$FILENAME" | wc -l`?

Ciao, Johannes

On Tue, Jun 30, 2015 at 02:10:49PM +0200, Johannes Schindelin wrote:

Or even `git rev-list --count HEAD -- "$FILENAME"`.

John Keeping <john@keeping.me.uk> writes:

Ahh, OK.  I didn't know we already had "rev-list --count".

Then please disregard the suggestion to add the option to "log"; it still holds true that the option does not belong to "shortlog", but I do think "how many changes were made to this path" statistics driven by a script should use "rev-list" plumbing, and if it already has "--count" option, that is perfect ;-)

Thanks.

On Fri, Jul 3, 2015 at 10:31 AM, Junio C Hamano <gitster@pobox.com> wrote:

Junio,

I think, respectfully, there is still a benefit to adding it as a feature to "log", in that more Git users know of and use "log" than "rev-list". I hadn't heard of "rev-list" before joining this mailing list.

That means "log --count" will get used more. That also means that more eyeballs will hit --count with bug reports and better tests; I've already seen 2-3 suggestions for "log --count" tests that "rev-list --count" also doesn't have tests for.

I would like to keep working on implementing "log --count", sharing code with rev-list where possible so they both are improved, unless you are saying you won't merge.

Thanks, Lawrence

Lawrence Siebert wrote:

Lawrence,

As git-rev-list is (mainly) plumbing for git-log porcelain, I think what you would need to do to add "--count" support to "git log" is just parse option, exclude nonsense combinations, and pass down to the revision parsing machinery.

HTH

Vincent, I'm ccing you because of --use-bitmap-index

John, Johannes,

I really appreciate both your thoughts.

`git rev-list --count HEAD -- "$FILENAME"` runs noticeably faster then
my patch code for `git shortlog --count`, git shortlog -s "$FILENAME"
| cut -f 1 | paste -sd+ -|bc, and faster than any use of piping to wc
-l mentioned so far.  Anyway I think Junio is quite right that my code
doesn't belong in shortlog, at least as it currently is.

I have some ideas for future work for myself, both code and documentation changes. I can detail it and get comments first, or just submit patches and get comments after, whichever is the preferred practice. One in particular is worth mentioning.

The following doesn't currently run: `git rev-list --count --use-bitmap-index HEAD`

This is an optional parameter for rev-list from commit aa32939fea9c8934b41efce56015732fa12b8247 which can't currently be used because of changes in parameter parsing, but which modifies `--count` and which may be faster. I've gotten it working again, both by changing the current repo code to make it work, and also by building from that commit, and when I tested it on the whole repo, it seems like it's less variable in speed then `git rev-list --count HEAD`. but not necessarily consistently faster like tests suggested it was when it was committed. Obviously I'm not testing on the same system as the original committer, or with the same compiler, or even using the same version of the linux kernel repo, so those may be a factor. It may also work better in a circumstance that I haven't accounted for, like an older repo, on a per file basis when getting per file commit counts for all files, or something like that.

I'm thinking I could submit a patch that makes it work again, and leave it to the user to decide whether to use it or not. There is also a --test-bitmap option which compares the regular count with the bitmap count. I'm not sure if the implication there was regression testing or that --use-bitmap-index might give the wrong results in certain circumstances. Vincent, could you clarify?

Thanks, Lawrence Siebert http://www.github.com/gryftir

On Tue, Jun 30, 2015 at 5:23 AM, John Keeping <john@keeping.me.uk> wrote:

On Tue, Jun 30, 2015 at 08:00:53PM -0700, Lawrence Siebert wrote:

Can you give more details?

In a copy of linux.git with bitmaps:

  $ git log -1 --oneline
  64fb1d0 Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc

  $ ls -l .git/objects/pack/
  total 892792
  -r--r--r-- 1 peff peff  24498374 May 21 13:39 pack-182149ca37c3f2d8fa190e4add772ae08af0e9d2.bitmap
  -r--r--r-- 1 peff peff 115283036 May 21 13:39 pack-182149ca37c3f2d8fa190e4add772ae08af0e9d2.idx
  -r--r--r-- 1 peff peff 774420808 May 21 13:39 pack-182149ca37c3f2d8fa190e4add772ae08af0e9d2.pack

The packfiles were created with "git repack -adb". It shows big speedups for this exact operation:

  $ git version
  git version 2.5.0.rc0

  $ time git rev-list --count HEAD
  515406

  real    0m9.500s
  user    0m9.424s
  sys     0m0.092s

  $ time git rev-list --use-bitmap-index --count HEAD
  515406

  real    0m0.392s
  user    0m0.328s
  sys     0m0.064s

Note that this would not work with, say:

  git rev-list --use-bitmap-index --count HEAD -- Makefile

as the bitmap index does not have enough information to do path limiting (we should probably disallow this or fall back to the non-bitmap code path, but right now we just ignore the path limiter).

Yes, `--test-bitmap` is just for internal testing; you should always get the same results.

-Peff

Jeff King <peff@peff.net> writes:

Yeah, at least the former but preferrably the latter.

Thanks.