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.
Matthieu Moy[RFC/PATCH] Re: BUG? git log -Sfoo --max-count=N
Ó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/
Junio C HamanoRe: [RFC/PATCH] Re: BUG? git log -Sfoo --max-count=N
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
Matthieu Moy[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.
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
Jeff KingRe: [PATCH] log: fix --max-count when used together with -S or -G
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
Matthieu MoyRe: [PATCH] log: fix --max-count when used together with -S or -G
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/
Junio C HamanoRe: [PATCH] log: fix --max-count when used together with -S or -G
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.
Jeff KingRe: [PATCH] log: fix --max-count when used together with -S or -G
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