BUG? git log -Sfoo --max-count=N

BUG? git log -Sfoo --max-count=N

[Óscar Fuentes]

The documentation says

--max-count=<number> 
  Limit the number of commits output

but when used with -S as in

git log -Sfoo --max-count=N

it acts as "inspect only the N first commits", i.e. if `foo' is not
present on any of the first N commits no output is shown.

Using other filtering options (such as `--grep=' or `-- somepath')
together with --max-count=N will output at most N commits regardless of
the position on the history of those commits, as expected.

Tested with git 1.7.1 and 1.7.4.

[RFC/PATCH] Re: BUG? git log -Sfoo --max-count=N

[Matthieu Moy]

Óscar Fuentes <ofv@wanadoo.es> writes:

> when [--max-count is] used with -S as in
>
> git log -Sfoo --max-count=N
>
> it acts as "inspect only the N first commits", i.e. if `foo' is not
> present on any of the first N commits no output is shown.

I'd call this a bug.

The following patch seems to fix it, but I'm not terribly happy with the
way it works. Any better idea?

From 3b962e004790c36c426efff64ad34043045e4aca Mon Sep 17 00:00:00 2001
From: Matthieu Moy <Matthieu.Moy@imag.fr>
Date: Mon, 7 Mar 2011 13:41:05 +0100
Subject: [PATCH] log: fix --max-count when used together with -S or -G

--max-count is implemented by counting revisions in get_revision(), but
the -S and -G take effect later (after running diff), hence,
--max-count=10 -Sfoo meant "examine the 10 first revisions, and out of
them, show only those changing the occurences of foo", not "show 10
revisions changing the occurences of foo".

In case the commit isn't actually shown, cancel the decrement of
max_count.
---
 builtin/log.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index f5ed690..b83900b 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -263,7 +263,12 @@ static int cmd_log_walk(struct rev_info *rev)
         * retain that state information if replacing rev->diffopt in this loop
         */
        while ((commit = get_revision(rev)) != NULL) {
-               log_tree_commit(rev, commit);
+               if (!log_tree_commit(rev, commit))
+                       /*
+                        * We decremented max_count in get_revision,
+                        * but we didn't actually show the commit.
+                        */
+                       rev->max_count++;
                if (!rev->reflog_info) {
                        /* we allow cycles in reflog ancestry */
                        free(commit->buffer);
-- 
1.7.4.1.176.g6b069.dirty

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

Re: [RFC/PATCH] Re: BUG? git log -Sfoo --max-count=N

[Junio C Hamano]

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

> In case the commit isn't actually shown, cancel the decrement of
> max_count.

I briefly wondered if this change is enough to deal with the way boundary
commits are handled in revision.c::get_revision_internal().  Once the
count drops to zero, the function switches to "boundary commits output
mode", and incrementing the variable after that happens wouldn't have any
effect, but that happens only upon the next call to get_revision() that
has zero in max_count upon entry to the function, so incrementing here
would be compatible with the precondition of that function.

I agree that this codepath is subtle, but it doesn't feel a particularly
good change to move the call to log_tree_commit() to get_revision() as a
general callback, either.  The function does way too many things other
than "determine if this commit is shown and return 0/1".

> ---
>  builtin/log.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index f5ed690..b83900b 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -263,7 +263,12 @@ static int cmd_log_walk(struct rev_info *rev)
>          * retain that state information if replacing rev->diffopt in this loop
>          */
>         while ((commit = get_revision(rev)) != NULL) {
> -               log_tree_commit(rev, commit);
> +               if (!log_tree_commit(rev, commit))
> +                       /*
> +                        * We decremented max_count in get_revision,
> +                        * but we didn't actually show the commit.
> +                        */
> +                       rev->max_count++;
>                 if (!rev->reflog_info) {
>                         /* we allow cycles in reflog ancestry */
>                         free(commit->buffer);
> -- 
> 1.7.4.1.176.g6b069.dirty

[PATCH] log: fix --max-count when used together with -S or -G

[Matthieu Moy]

--max-count is implemented by counting revisions in get_revision(), but
the -S and -G take effect later (after running diff), hence,
--max-count=10 -Sfoo meant "examine the 10 first revisions, and out of
them, show only those changing the occurences of foo", not "show 10
revisions changing the occurences of foo".

In case the commit isn't actually shown, cancel the decrement of
max_count.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

Although I don't find the patch really elegant, it seems correct
(well, there was an obvious bug: I didn't check that max_count was !=
-1, but that's repaired), and nobody came up with a better idea.

Since the RFC, I also added tests.

 builtin/log.c                             |    8 +++++++-
 t/t4013-diff-various.sh                   |    3 +++
 t/t4013/diff.log_-SF_master_--max-count=0 |    2 ++
 t/t4013/diff.log_-SF_master_--max-count=1 |    7 +++++++
 t/t4013/diff.log_-SF_master_--max-count=2 |    7 +++++++
 5 files changed, 26 insertions(+), 1 deletions(-)
 create mode 100644 t/t4013/diff.log_-SF_master_--max-count=0
 create mode 100644 t/t4013/diff.log_-SF_master_--max-count=1
 create mode 100644 t/t4013/diff.log_-SF_master_--max-count=2

diff --git a/builtin/log.c b/builtin/log.c
index f5ed690..167d710 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -263,7 +263,13 @@ static int cmd_log_walk(struct rev_info *rev)
 	 * retain that state information if replacing rev->diffopt in this loop
 	 */
 	while ((commit = get_revision(rev)) != NULL) {
-		log_tree_commit(rev, commit);
+		if (!log_tree_commit(rev, commit) &&
+		    rev->max_count >= 0)
+			/*
+			 * We decremented max_count in get_revision,
+			 * but we didn't actually show the commit.
+			 */
+			rev->max_count++;
 		if (!rev->reflog_info) {
 			/* we allow cycles in reflog ancestry */
 			free(commit->buffer);
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index b8f81d0..5daa0f2 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -210,6 +210,9 @@ log -m -p master
 log -SF master
 log -S F master
 log -SF -p master
+log -SF master --max-count=0
+log -SF master --max-count=1
+log -SF master --max-count=2
 log -GF master
 log -GF -p master
 log -GF -p --pickaxe-all master
diff --git a/t/t4013/diff.log_-SF_master_--max-count=0 b/t/t4013/diff.log_-SF_master_--max-count=0
new file mode 100644
index 0000000..c1fc6c8
--- /dev/null
+++ b/t/t4013/diff.log_-SF_master_--max-count=0
@@ -0,0 +1,2 @@
+$ git log -SF master --max-count=0
+$
diff --git a/t/t4013/diff.log_-SF_master_--max-count=1 b/t/t4013/diff.log_-SF_master_--max-count=1
new file mode 100644
index 0000000..c981a03
--- /dev/null
+++ b/t/t4013/diff.log_-SF_master_--max-count=1
@@ -0,0 +1,7 @@
+$ git log -SF master --max-count=1
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+$
diff --git a/t/t4013/diff.log_-SF_master_--max-count=2 b/t/t4013/diff.log_-SF_master_--max-count=2
new file mode 100644
index 0000000..a6c55fd
--- /dev/null
+++ b/t/t4013/diff.log_-SF_master_--max-count=2
@@ -0,0 +1,7 @@
+$ git log -SF master --max-count=2
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+$
-- 
1.7.4.1.211.gda9d9.dirty

Re: [PATCH] log: fix --max-count when used together with -S or -G

[Jeff King]

On Wed, Mar 09, 2011 at 09:52:15PM +0100, Matthieu Moy wrote:

> --max-count is implemented by counting revisions in get_revision(), but
> the -S and -G take effect later (after running diff), hence,
> --max-count=10 -Sfoo meant "examine the 10 first revisions, and out of
> them, show only those changing the occurences of foo", not "show 10
> revisions changing the occurences of foo".
> 
> In case the commit isn't actually shown, cancel the decrement of
> max_count.

Hmm. Is this papering over a bigger problem, which is that we are
throwing out commits at the time of diff rather than finding out early
whether they are TREESAME?

That is, you fixed this:

  git log -100 -Sfoo

but this is still broken:

  git log --parents -Sfoo

in that parent rewriting doesn't happen. You can see the results with
"gitk -Sfoo" (compare to "gitk -- path", which properly shows a
simplified history).

This is also a problem with --follow. Maybe others.

One solution is to hoist the diffcore_std stuff up to rev_compare_tree,
so we get pickaxe and rename-detection at that level. But there may be
some performance implications, especially with respect to saving the
intermediate result to be used by the actual diff generation later on.

So it's definitely a much deeper topic than your small patch. Which
maybe means we should apply your patch now as a band-aid and hope for a
better solution in the long term. I dunno.

-Peff

Re: [PATCH] log: fix --max-count when used together with -S or -G

[Matthieu Moy]

Jeff King <peff@peff.net> writes:

> So it's definitely a much deeper topic than your small patch. Which
> maybe means we should apply your patch now as a band-aid and hope for a
> better solution in the long term. I dunno.

I'm too lazy/don't have time to do an in-depth fix. It doesn't seem
crazy to apply the patch, since it fixes the common case, and adds tests
for it, but I don't care personnaly about the feature/bug, so I won't
fight if it's rejected. I can resend with a more explicit comment in the
code saying it would deserve a better fix if someone cares.

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

Re: [PATCH] log: fix --max-count when used together with -S or -G

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hmm. Is this papering over a bigger problem,...

It is not very obvious to me if redefining the semantics of filtering done
by diff (the current definition is it is purely an output phase thing) is
necessarily a good thing. I agree that the interaction between the output
phase filtering and pruning done by the revision walker machinery is a
fine topic to discuss.

But Matthieu's patch is not papering over anything but is a real fix
within the context of the current architecture.

Re: [PATCH] log: fix --max-count when used together with -S or -G

[Jeff King]

On Wed, Mar 09, 2011 at 02:27:32PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Hmm. Is this papering over a bigger problem,...
> 
> It is not very obvious to me if redefining the semantics of filtering done
> by diff (the current definition is it is purely an output phase thing) is
> necessarily a good thing. I agree that the interaction between the output
> phase filtering and pruning done by the revision walker machinery is a
> fine topic to discuss.
> 
> But Matthieu's patch is not papering over anything but is a real fix
> within the context of the current architecture.

I consider the current state to be "buggy but we live with it" and not
an architectural decision. But that is simply a matter of perspective. :)

Certainly Matthieu's patch fixes a real problem, and it does not make it
any harder to address the other problems in the future, so I think there
is no reason not to apply it.

-Peff