[PATCH v2 0/4] git log --count

[PATCH v2 0/4] git log --count

[Lawrence Siebert]

This should fix the whitespace issues this had previously.  

Lawrence Siebert (4):
  list-object: add get_commit_count function
  log: add --count option to git log
  log --count: added test
  git-log: update man documentation for --count

 Documentation/git-log.txt          |  2 ++
 Documentation/rev-list-options.txt |  2 +-
 builtin/log.c                      | 29 +++++++++++++++++++++++++++++
 builtin/rev-list.c                 | 12 ++----------
 list-objects.c                     | 14 ++++++++++++++
 list-objects.h                     |  1 +
 t/t4202-log.sh                     |  7 +++++++
 7 files changed, 56 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] list-object: add get_commit_count function

[Lawrence Siebert]

Moving commit counting from rev-list into list-object which is a step
toward letting git log do counting as well.

Signed-off-by: Lawrence Siebert <lawrencesiebert@gmail.com>
---
 builtin/rev-list.c | 12 ++----------
 list-objects.c     | 14 ++++++++++++++
 list-objects.h     |  1 +
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ff84a82..07f522b 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -388,16 +388,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	traverse_commit_list(&revs, show_commit, show_object, &info);
 
-	if (revs.count) {
-		if (revs.left_right && revs.cherry_mark)
-			printf("%d\t%d\t%d\n", revs.count_left, revs.count_right, revs.count_same);
-		else if (revs.left_right)
-			printf("%d\t%d\n", revs.count_left, revs.count_right);
-		else if (revs.cherry_mark)
-			printf("%d\t%d\n", revs.count_left + revs.count_right, revs.count_same);
-		else
-			printf("%d\n", revs.count_left + revs.count_right);
-	}
+	if (revs.count)
+		get_commit_count(&revs);
 
 	return 0;
 }
diff --git a/list-objects.c b/list-objects.c
index 41736d2..6f76301 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -234,3 +234,17 @@ void traverse_commit_list(struct rev_info *revs,
 	object_array_clear(&revs->pending);
 	strbuf_release(&base);
 }
+
+void get_commit_count(struct rev_info * revs) {
+	if (revs->count) {
+		if (revs->left_right && revs->cherry_mark)
+			printf("%d\t%d\t%d\n", revs->count_left, revs->count_right, revs->count_same);
+		else if (revs->left_right)
+			printf("%d\t%d\n", revs->count_left, revs->count_right);
+		else if (revs->cherry_mark)
+			printf("%d\t%d\n", revs->count_left + revs->count_right, revs->count_same);
+		else
+			printf("%d\n", revs->count_left + revs->count_right);
+	}
+	return;
+}
diff --git a/list-objects.h b/list-objects.h
index 136a1da..d28c1f3 100644
--- a/list-objects.h
+++ b/list-objects.h
@@ -7,5 +7,6 @@ void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, voi
 
 typedef void (*show_edge_fn)(struct commit *);
 void mark_edges_uninteresting(struct rev_info *, show_edge_fn);
+void get_commit_count(struct rev_info * revs);
 
 #endif
-- 
1.9.1

Re: [PATCH v2 1/4] list-object: add get_commit_count function

[Matthieu Moy]

Lawrence Siebert <lawrencesiebert@gmail.com> writes:

> +void get_commit_count(struct rev_info * revs) {

Please, write "struct rev_info *revs" (stick * to revs).

> +void get_commit_count(struct rev_info * revs);

Likewise.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 1/4] list-object: add get_commit_count function

[Lawrence Siebert]

Mattieu,

Understood. I don't suppose there is any commonly code formatting tool
to automate formatting in the git style, is there?

Thanks,
Lawrence

On Fri, Jul 3, 2015 at 12:24 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Lawrence Siebert <lawrencesiebert@gmail.com> writes:
>
>> +void get_commit_count(struct rev_info * revs) {
>
> Please, write "struct rev_info *revs" (stick * to revs).
>
>> +void get_commit_count(struct rev_info * revs);
>
> Likewise.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/



-- 
About Me: http://about.me/lawrencesiebert
Constantly Coding: http://constantcoding.blogspot.com

Re: [PATCH v2 1/4] list-object: add get_commit_count function

[Matthieu Moy]

Lawrence Siebert <lawrencesiebert@gmail.com> writes:

> Mattieu,
>
> Understood. I don't suppose there is any commonly code formatting tool
> to automate formatting in the git style, is there?

IIRC, someone posted a configuration file for clang-format that
essentially matches the Git coding style.

You can read Documentation/CodingGuidelines. Unsurprisingly, Git uses a
coding style similar to the Linux kernel, so anything you find about the
kernel essentially applies here too.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v2 2/4] log: add --count option to git log

[Lawrence Siebert]

adds --count from git rev-list to git log, without --use-bitmap-index
for the moment.

Signed-off-by: Lawrence Siebert <lawrencesiebert@gmail.com>
---
 builtin/log.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index 8781049..4aaff3a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -25,6 +25,7 @@
 #include "version.h"
 #include "mailmap.h"
 #include "gpg-interface.h"
+#include "list-objects.h"
 
 /* Set a default date-time format for git log ("log.date" config variable) */
 static const char *default_date_mode = NULL;
@@ -317,12 +318,40 @@ static void finish_early_output(struct rev_info *rev)
 	show_early_header(rev, "done", n);
 }
 
+static void show_object(struct object *obj,
+			const struct name_path *path, const char *component,
+			void *cb_data)
+{
+	return;
+}
+
+static void show_commit(struct commit *commit, void *data)
+{
+	struct rev_info *revs = (struct rev_info *)data;
+	if (commit->object.flags & PATCHSAME)
+		revs->count_same++;
+	else if (commit->object.flags & SYMMETRIC_LEFT)
+		revs->count_left++;
+	else
+		revs->count_right++;
+	if (commit->parents) {
+		free_commit_list(commit->parents);
+		commit->parents = NULL;
+	}
+	free_commit_buffer(commit);
+}
+
 static int cmd_log_walk(struct rev_info *rev)
 {
 	struct commit *commit;
 	int saved_nrl = 0;
 	int saved_dcctc = 0;
 
+	if (rev->count) {
+		prepare_revision_walk(rev);
+		traverse_commit_list(rev, show_commit, show_object, rev);
+		get_commit_count(rev);
+	}
 	if (rev->early_output)
 		setup_early_output(rev);
 
-- 
1.9.1

Re: [PATCH v2 2/4] log: add --count option to git log

[Matthieu Moy]

Lawrence Siebert <lawrencesiebert@gmail.com> writes:

> +static void show_object(struct object *obj,
> +			const struct name_path *path, const char *component,
> +			void *cb_data)
> +{
> +	return;
> +}

It seems streange to me to have a function named show_object when it
does not show anything. Maybe name it null_travers_cb to make it clear
it's a callback and it does nothing?

Not a strong objection though, it's only a static function.

> +static void show_commit(struct commit *commit, void *data)
> +{
> +	struct rev_info *revs = (struct rev_info *)data;
> +	if (commit->object.flags & PATCHSAME)
> +		revs->count_same++;
> +	else if (commit->object.flags & SYMMETRIC_LEFT)
> +		revs->count_left++;
> +	else
> +		revs->count_right++;
> +	if (commit->parents) {
> +		free_commit_list(commit->parents);
> +		commit->parents = NULL;
> +	}
> +	free_commit_buffer(commit);
> +}
> +
>  static int cmd_log_walk(struct rev_info *rev)
>  {
>  	struct commit *commit;
>  	int saved_nrl = 0;
>  	int saved_dcctc = 0;
>  
> +	if (rev->count) {
> +		prepare_revision_walk(rev);
> +		traverse_commit_list(rev, show_commit, show_object, rev);
> +		get_commit_count(rev);
> +	}

I didn't test, but it seems this does a full graph traversal before
starting the output. A very important property of "git log" is that it
starts showing revisions immediately, even when ran on a very long
history (it shows the first screen immediately and continues working in
the background while the first page is displayed in the pager).

Is it the case? If so, it should be changed. If not, perhaps explain why
in the commit message.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 2/4] log: add --count option to git log

[Lawrence Siebert]

traverse_commit_list requires a function to be passed to it.  That
said, after further review it can probably pass NULL and have it work
fine.  If not, I'll use your naming convention.

`git rev-list --count` (or actually `git rev-list --count HEAD`, which
this borrows the code from, produces a single value, a numeric count.
I think walking the entire list is necessary to get the final value,
which is what we want with --count.

Thanks,
Lawrence Siebert





On Fri, Jul 3, 2015 at 12:29 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Lawrence Siebert <lawrencesiebert@gmail.com> writes:
>
>> +static void show_object(struct object *obj,
>> +                     const struct name_path *path, const char *component,
>> +                     void *cb_data)
>> +{
>> +     return;
>> +}
>
> It seems streange to me to have a function named show_object when it
> does not show anything. Maybe name it null_travers_cb to make it clear
> it's a callback and it does nothing?
>
> Not a strong objection though, it's only a static function.
>
>> +static void show_commit(struct commit *commit, void *data)
>> +{
>> +     struct rev_info *revs = (struct rev_info *)data;
>> +     if (commit->object.flags & PATCHSAME)
>> +             revs->count_same++;
>> +     else if (commit->object.flags & SYMMETRIC_LEFT)
>> +             revs->count_left++;
>> +     else
>> +             revs->count_right++;
>> +     if (commit->parents) {
>> +             free_commit_list(commit->parents);
>> +             commit->parents = NULL;
>> +     }
>> +     free_commit_buffer(commit);
>> +}
>> +
>>  static int cmd_log_walk(struct rev_info *rev)
>>  {
>>       struct commit *commit;
>>       int saved_nrl = 0;
>>       int saved_dcctc = 0;
>>
>> +     if (rev->count) {
>> +             prepare_revision_walk(rev);
>> +             traverse_commit_list(rev, show_commit, show_object, rev);
>> +             get_commit_count(rev);
>> +     }
>
> I didn't test, but it seems this does a full graph traversal before
> starting the output. A very important property of "git log" is that it
> starts showing revisions immediately, even when ran on a very long
> history (it shows the first screen immediately and continues working in
> the background while the first page is displayed in the pager).
>
> Is it the case? If so, it should be changed. If not, perhaps explain why
> in the commit message.
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/



-- 
About Me: http://about.me/lawrencesiebert
Constantly Coding: http://constantcoding.blogspot.com

Re: [PATCH v2 2/4] log: add --count option to git log

[Matthieu Moy]

Hi,

Please, don't top-post on this list. Quote the parts you're replying to
and reply below.

Lawrence Siebert <lawrencesiebert@gmail.com> writes:

> traverse_commit_list requires a function to be passed to it.  That
> said, after further review it can probably pass NULL and have it work
> fine.  If not, I'll use your naming convention.

If possible, passing NULL would be best.

> `git rev-list --count` (or actually `git rev-list --count HEAD`, which
> this borrows the code from, produces a single value, a numeric count.
> I think walking the entire list is necessary to get the final value,
> which is what we want with --count.

OK, that was me answering emails before coffee. I thought --count was
producing the count _in addition_ to the normal output. But then I don't
understand by looking at the code how you prevent the normal output. I
just tested, and indeed it does work (I guess all the magic is already
there from "rev-list --count", and it was more or less a bug that "log
--count" was not using it properly), but you may want to explain better
what's going on in the commit message.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH v2 3/4] log --count: added test

[Lawrence Siebert]

added test comparing output between git log --count HEAD and
git rev-list --count HEAD

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

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1b2e981..35f8d82 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -871,4 +871,11 @@ test_expect_success 'log --graph --no-walk is forbidden' '
 	test_must_fail git log --graph --no-walk
 '
 
+test_expect_success 'log --count' '
+	git log --count HEAD > actual &&
+	git	rev-list --count HEAD > expect &&
+	test_cmp expect actual
+'
+
+
 test_done
-- 
1.9.1

Re: [PATCH v2 3/4] log --count: added test

[Matthieu Moy]

Lawrence Siebert <lawrencesiebert@gmail.com> writes:

> added test comparing output between git log --count HEAD and
> git rev-list --count HEAD

Unless there is a very long list of tests, I'd rather see this squashed
with PATCH 2/4. As a reviewer I prefer having code and tests in the same
place.

> Signed-off-by: Lawrence Siebert <lawrencesiebert@gmail.com>
> ---
>  t/t4202-log.sh | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 1b2e981..35f8d82 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -871,4 +871,11 @@ test_expect_success 'log --graph --no-walk is forbidden' '
>  	test_must_fail git log --graph --no-walk
>  '
>  
> +test_expect_success 'log --count' '
> +	git log --count HEAD > actual &&
> +	git	rev-list --count HEAD > expect &&

The weird space is still there.

Also, we write ">actual", not "> actual" in the Git coding style.

That is actually a rather weak test. rev-list --count interacts with
--left-right, so I guess you want to test --count --left-right.

Also, some revision-limiting options can reduce the count like

git log --grep whatever

and you should check that you actually count the right number here.

(I don't know this part of the code enough, but I'm not sure you
actually deal with this properly)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 3/4] log --count: added test

[Lawrence Siebert]

Matthieu,

Ok, I'll fix that.  I think I can also add tests, I can look at the
tests for rev-list --count, with the understanding that I saw somebody
else had made changes for the  --use-bitmap-index option, and I am
basing off of master for this, and thus don't feel comfortable with
--use-bitmap-index at this time.

If it's acceptable practice, I'll just squash everything I do on this
feature and it's tests into one commit with a more detailed comment,
and send the patch for that.  I wasn't sure about how much history I
should save, and how much I should split stuff up, so I appreciate
your clarification.

Thank you for your time,
Lawrence Siebert

On Fri, Jul 3, 2015 at 12:34 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Lawrence Siebert <lawrencesiebert@gmail.com> writes:
>
>> added test comparing output between git log --count HEAD and
>> git rev-list --count HEAD
>
> Unless there is a very long list of tests, I'd rather see this squashed
> with PATCH 2/4. As a reviewer I prefer having code and tests in the same
> place.
>
>> Signed-off-by: Lawrence Siebert <lawrencesiebert@gmail.com>
>> ---
>>  t/t4202-log.sh | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 1b2e981..35f8d82 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -871,4 +871,11 @@ test_expect_success 'log --graph --no-walk is forbidden' '
>>       test_must_fail git log --graph --no-walk
>>  '
>>
>> +test_expect_success 'log --count' '
>> +     git log --count HEAD > actual &&
>> +     git     rev-list --count HEAD > expect &&
>
> The weird space is still there.
>
> Also, we write ">actual", not "> actual" in the Git coding style.
>
> That is actually a rather weak test. rev-list --count interacts with
> --left-right, so I guess you want to test --count --left-right.
>
> Also, some revision-limiting options can reduce the count like
>
> git log --grep whatever
>
> and you should check that you actually count the right number here.
>
> (I don't know this part of the code enough, but I'm not sure you
> actually deal with this properly)
>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/



-- 
About Me: http://about.me/lawrencesiebert
Constantly Coding: http://constantcoding.blogspot.com

Re: [PATCH v2 3/4] log --count: added test

[Matthieu Moy]

Lawrence Siebert <lawrencesiebert@gmail.com> writes:

> Ok, I'll fix that.

(Note: this is a typical example of why we don't top-post here. I made
several remarks and I can't know what "that" refers to)

(Meta-note: don't take the note as agressive, I know that top-posting is
the norm in many other places, I'm just giving you a glimpse of the
local culture ;-) ).

> If it's acceptable practice, I'll just squash everything I do on this
> feature and it's tests into one commit with a more detailed comment,
> and send the patch for that.

I think at least two patches are better: your PATCH 1 is a typical
preparation step, best reviewed alone in its own patch.

Splitting history into several patches is good, but each patch should
correspond to one logical step. Splitting code Vs doc Vs tests is
usually not the right way.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 3/4] log --count: added test

[Matthieu Moy]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Also, some revision-limiting options can reduce the count like
>
> git log --grep whatever

OK, --grep seems to work, but -S and -G do not:

$ ./bin-wrappers/git log -Sfoo --count
40012
$ ./bin-wrappers/git log -Sfoo --oneline | wc -l
925
$ ./bin-wrappers/git log --count                 
40012

See 251df09 (log: fix --max-count when used together with -S or -G,
2011-03-09) for a start of an explanation.

If implementing a proper count is too hard, one option is to forbid
--count -S and --count -G to avoid confusion.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 3/4] log --count: added test

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> If implementing a proper count is too hard, one option is to forbid
> --count -S and --count -G to avoid confusion.

Let's not go there.  Letting people to use "--oneline | wc -l" is
far better unless we can get --count that behaves the same as that,
only faster.

Re: [PATCH v2 3/4] log --count: added test

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Also, some revision-limiting options can reduce the count like
>
> git log --grep whatever
>
> and you should check that you actually count the right number here.
>
> (I don't know this part of the code enough, but I'm not sure you
> actually deal with this properly)

Yes, "rev-list", when the revision range is "limited" (with or
without pathspec), can give "--count" once limit_list() finishes,
but "log" filters the result of limit_list() further with at least
three separate phases.

 - options in the "grep" family (--grep/--author/etc.) lets you skip
   commits based on the contents of the commit object;

 - options in the "diff" family (-w/-b/etc.) may let "git log"
   consider a commit because the pathspec limit thought two blobs
   were different at byte-by-byte level, but after running "diff"
   with these "looser" comparison, "git log" may realize that there
   weren't any interesting change introduced by the commit [*1*];

 - and finally, of course "log --max-count=20" may further limit the
   maximum number of commits that are shown.  This of course is not
   interesting in the context of "--count" in the sense that "git
   log --count -20 --grep=foo maint..master" may not be immediately
   a sensible thing to do (but we never know.  Perhaps your user may
   be asking "do we have 20 or more commits that say 'foo' in the
   range?")

An implementation of "--count" to take the first and the third ones
in account may not be too hard, but I am fairly familiar with the
codepath for the second one and I think it would be very tricky.

Note that these additional things "log" does over "rev-list" *DO*
justify addition of "--count" to "log" (because "rev-list --count"
cannot emulate these); I am however not sure if it is worth the
additional complexity we need to add to the codepath (especially for
the second phase).  I'd need to take another look at the codepaths
involved myself to be sure, but I suspect the damage to the codepath
for the second may end up to be extensive when we do decide to fix
the possible bug in it.


[Footnote]

*1* They may still show the log message in such a case where "-b/-w"
was asked and commit had only whitespace changes, but I think we
should consider that a bug.

[PATCH v2 4/4] git-log: update man documentation for --count

[Lawrence Siebert]

I'm not altogether sure the best way to update the internal usage
from git-log -h, but this at least updates the man page.

Signed-off-by: Lawrence Siebert <lawrencesiebert@gmail.com>
---
 Documentation/git-log.txt          | 2 ++
 Documentation/rev-list-options.txt | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 5692945..0e10e44 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -90,6 +90,8 @@ include::line-range-format.txt[]
 Paths may need to be prefixed with ``\-- '' to separate them from
 options or the revision range, when confusion arises.
 
+
+:git-log: 1
 include::rev-list-options.txt[]
 
 include::pretty-formats.txt[]
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 77ac439..f4e15fb 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -797,7 +797,7 @@ This implies the `--topo-order` option by default, but the
 	in between them in that case. If `<barrier>` is specified, it
 	is the string that will be shown instead of the default one.
 
-ifdef::git-rev-list[]
+ifdef::git-log,git-rev-list[]
 --count::
 	Print a number stating how many commits would have been
 	listed, and suppress all other output.  When used together
-- 
1.9.1