{"thread":{"id":"26675","subject":"BUG? git log -Sfoo --max-count=N","startedAt":"2011-03-06T21:37:37Z","lastAt":"2011-03-10T22:39:34Z","messageCount":8,"participants":["Óscar Fuentes","Matthieu Moy","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"162859","messageId":"87hbbgvske.fsf@wanadoo.es","threadId":"26675","inReplyTo":null,"subject":"BUG? git log -Sfoo --max-count=N","fromName":"Óscar Fuentes","fromEmail":"ofv@wanadoo.es","sentAt":"2011-03-06T21:37:37Z","receivedAt":"2011-03-06T21:37:37Z","isPatch":false,"sender":{"key":"ofv@wanadoo.es","avatar":null},"body":"The documentation says\n\n--max-count= \n Limit the number of commits output\n\nbut when used with -S as in\n\ngit log -Sfoo --max-count=N\n\nit acts as \"inspect only the N first commits\", i.e. if `foo' is not\npresent on any of the first N commits no output is shown.\n\nUsing other filtering options (such as `--grep=' or `-- somepath')\ntogether with --max-count=N will output at most N commits regardless of\nthe position on the history of those commits, as expected.\n\nTested with git 1.7.1 and 1.7.4.\n"},{"id":"162917","messageId":"vpqpqq3qern.fsf@bauges.imag.fr","threadId":"26675","inReplyTo":"87hbbgvske.fsf@wanadoo.es","subject":"[RFC/PATCH] Re: BUG? git log -Sfoo --max-count=N","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-03-07T12:46:52Z","receivedAt":"2011-03-07T12:46:52Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Óscar Fuentes writes:\n\n> when [--max-count is] used with -S as in\n>\n> git log -Sfoo --max-count=N\n>\n> it acts as \"inspect only the N first commits\", i.e. if `foo' is not\n> present on any of the first N commits no output is shown.\n\nI'd call this a bug.\n\nThe following patch seems to fix it, but I'm not terribly happy with the\nway it works. Any better idea?\n\nFrom 3b962e004790c36c426efff64ad34043045e4aca Mon Sep 17 00:00:00 2001\nFrom: Matthieu Moy \nDate: Mon, 7 Mar 2011 13:41:05 +0100\nSubject: [PATCH] log: fix --max-count when used together with -S or -G\n\n--max-count is implemented by counting revisions in get_revision(), but\nthe -S and -G take effect later (after running diff), hence,\n--max-count=10 -Sfoo meant \"examine the 10 first revisions, and out of\nthem, show only those changing the occurences of foo\", not \"show 10\nrevisions changing the occurences of foo\".\n\nIn case the commit isn't actually shown, cancel the decrement of\nmax_count.\n---\n builtin/log.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f5ed690..b83900b 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -263,7 +263,12 @@ static int cmd_log_walk(struct rev_info *rev)\n * retain that state information if replacing rev->diffopt in this loop\n */\n while ((commit = get_revision(rev)) != NULL) {\n- log_tree_commit(rev, commit);\n+ if (!log_tree_commit(rev, commit))\n+ /*\n+ * We decremented max_count in get_revision,\n+ * but we didn't actually show the commit.\n+ */\n+ rev->max_count++;\n if (!rev->reflog_info) {\n /* we allow cycles in reflog ancestry */\n free(commit->buffer);\n-- \n1.7.4.1.176.g6b069.dirty\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"163003","messageId":"7vvczte7tw.fsf@alter.siamese.dyndns.org","threadId":"26675","inReplyTo":"vpqpqq3qern.fsf@bauges.imag.fr","subject":"Re: [RFC/PATCH] Re: BUG? git log -Sfoo --max-count=N","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-08T19:22:03Z","receivedAt":"2011-03-08T19:22:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> In case the commit isn't actually shown, cancel the decrement of\n> max_count.\n\nI briefly wondered if this change is enough to deal with the way boundary\ncommits are handled in revision.c::get_revision_internal(). Once the\ncount drops to zero, the function switches to \"boundary commits output\nmode\", and incrementing the variable after that happens wouldn't have any\neffect, but that happens only upon the next call to get_revision() that\nhas zero in max_count upon entry to the function, so incrementing here\nwould be compatible with the precondition of that function.\n\nI agree that this codepath is subtle, but it doesn't feel a particularly\ngood change to move the call to log_tree_commit() to get_revision() as a\ngeneral callback, either. The function does way too many things other\nthan \"determine if this commit is shown and return 0/1\".\n\n> ---\n> builtin/log.c | 7 ++++++-\n> 1 files changed, 6 insertions(+), 1 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index f5ed690..b83900b 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -263,7 +263,12 @@ static int cmd_log_walk(struct rev_info *rev)\n> * retain that state information if replacing rev->diffopt in this loop\n> */\n> while ((commit = get_revision(rev)) != NULL) {\n> - log_tree_commit(rev, commit);\n> + if (!log_tree_commit(rev, commit))\n> + /*\n> + * We decremented max_count in get_revision,\n> + * but we didn't actually show the commit.\n> + */\n> + rev->max_count++;\n> if (!rev->reflog_info) {\n> /* we allow cycles in reflog ancestry */\n> free(commit->buffer);\n> -- \n> 1.7.4.1.176.g6b069.dirty\n"},{"id":"163083","messageId":"1299703935-639-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"26675","inReplyTo":"7vvczte7tw.fsf@alter.siamese.dyndns.org","subject":"[PATCH] log: fix --max-count when used together with -S or -G","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2011-03-09T20:52:15Z","receivedAt":"2011-03-09T20:52:15Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"--max-count is implemented by counting revisions in get_revision(), but\nthe -S and -G take effect later (after running diff), hence,\n--max-count=10 -Sfoo meant \"examine the 10 first revisions, and out of\nthem, show only those changing the occurences of foo\", not \"show 10\nrevisions changing the occurences of foo\".\n\nIn case the commit isn't actually shown, cancel the decrement of\nmax_count.\n\nSigned-off-by: Matthieu Moy \n---\n\nAlthough I don't find the patch really elegant, it seems correct\n(well, there was an obvious bug: I didn't check that max_count was !=\n-1, but that's repaired), and nobody came up with a better idea.\n\nSince the RFC, I also added tests.\n\n builtin/log.c | 8 +++++++-\n t/t4013-diff-various.sh | 3 +++\n t/t4013/diff.log_-SF_master_--max-count=0 | 2 ++\n t/t4013/diff.log_-SF_master_--max-count=1 | 7 +++++++\n t/t4013/diff.log_-SF_master_--max-count=2 | 7 +++++++\n 5 files changed, 26 insertions(+), 1 deletions(-)\n create mode 100644 t/t4013/diff.log_-SF_master_--max-count=0\n create mode 100644 t/t4013/diff.log_-SF_master_--max-count=1\n create mode 100644 t/t4013/diff.log_-SF_master_--max-count=2\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex f5ed690..167d710 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -263,7 +263,13 @@ static int cmd_log_walk(struct rev_info *rev)\n \t * retain that state information if replacing rev->diffopt in this loop\n \t */\n \twhile ((commit = get_revision(rev)) != NULL) {\n-\t\tlog_tree_commit(rev, commit);\n+\t\tif (!log_tree_commit(rev, commit) &&\n+\t\t rev->max_count >= 0)\n+\t\t\t/*\n+\t\t\t * We decremented max_count in get_revision,\n+\t\t\t * but we didn't actually show the commit.\n+\t\t\t */\n+\t\t\trev->max_count++;\n \t\tif (!rev->reflog_info) {\n \t\t\t/* we allow cycles in reflog ancestry */\n \t\t\tfree(commit->buffer);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex b8f81d0..5daa0f2 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -210,6 +210,9 @@ log -m -p master\n log -SF master\n log -S F master\n log -SF -p master\n+log -SF master --max-count=0\n+log -SF master --max-count=1\n+log -SF master --max-count=2\n log -GF master\n log -GF -p master\n log -GF -p --pickaxe-all master\ndiff --git a/t/t4013/diff.log_-SF_master_--max-count=0 b/t/t4013/diff.log_-SF_master_--max-count=0\nnew file mode 100644\nindex 0000000..c1fc6c8\n--- /dev/null\n+++ b/t/t4013/diff.log_-SF_master_--max-count=0\n@@ -0,0 +1,2 @@\n+$ git log -SF master --max-count=0\n+$\ndiff --git a/t/t4013/diff.log_-SF_master_--max-count=1 b/t/t4013/diff.log_-SF_master_--max-count=1\nnew file mode 100644\nindex 0000000..c981a03\n--- /dev/null\n+++ b/t/t4013/diff.log_-SF_master_--max-count=1\n@@ -0,0 +1,7 @@\n+$ git log -SF master --max-count=1\n+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n+Author: A U Thor \n+Date: Mon Jun 26 00:02:00 2006 +0000\n+\n+ Third\n+$\ndiff --git a/t/t4013/diff.log_-SF_master_--max-count=2 b/t/t4013/diff.log_-SF_master_--max-count=2\nnew file mode 100644\nindex 0000000..a6c55fd\n--- /dev/null\n+++ b/t/t4013/diff.log_-SF_master_--max-count=2\n@@ -0,0 +1,7 @@\n+$ git log -SF master --max-count=2\n+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0\n+Author: A U Thor \n+Date: Mon Jun 26 00:02:00 2006 +0000\n+\n+ Third\n+$\n-- \n1.7.4.1.211.gda9d9.dirty\n"},{"id":"163089","messageId":"20110309213824.GA4400@sigill.intra.peff.net","threadId":"26675","inReplyTo":"1299703935-639-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] log: fix --max-count when used together with -S or -G","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-09T21:38:27Z","receivedAt":"2011-03-09T21:38:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 09, 2011 at 09:52:15PM +0100, Matthieu Moy wrote:\n\n> --max-count is implemented by counting revisions in get_revision(), but\n> the -S and -G take effect later (after running diff), hence,\n> --max-count=10 -Sfoo meant \"examine the 10 first revisions, and out of\n> them, show only those changing the occurences of foo\", not \"show 10\n> revisions changing the occurences of foo\".\n> \n> In case the commit isn't actually shown, cancel the decrement of\n> max_count.\n\nHmm. Is this papering over a bigger problem, which is that we are\nthrowing out commits at the time of diff rather than finding out early\nwhether they are TREESAME?\n\nThat is, you fixed this:\n\n git log -100 -Sfoo\n\nbut this is still broken:\n\n git log --parents -Sfoo\n\nin that parent rewriting doesn't happen. You can see the results with\n\"gitk -Sfoo\" (compare to \"gitk -- path\", which properly shows a\nsimplified history).\n\nThis is also a problem with --follow. Maybe others.\n\nOne solution is to hoist the diffcore_std stuff up to rev_compare_tree,\nso we get pickaxe and rename-detection at that level. But there may be\nsome performance implications, especially with respect to saving the\nintermediate result to be used by the actual diff generation later on.\n\nSo it's definitely a much deeper topic than your small patch. Which\nmaybe means we should apply your patch now as a band-aid and hope for a\nbetter solution in the long term. I dunno.\n\n-Peff\n"},{"id":"163092","messageId":"vpqk4g8hsmf.fsf@bauges.imag.fr","threadId":"26675","inReplyTo":"20110309213824.GA4400@sigill.intra.peff.net","subject":"Re: [PATCH] log: fix --max-count when used together with -S or -G","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2011-03-09T21:49:12Z","receivedAt":"2011-03-09T21:49:12Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> So it's definitely a much deeper topic than your small patch. Which\n> maybe means we should apply your patch now as a band-aid and hope for a\n> better solution in the long term. I dunno.\n\nI'm too lazy/don't have time to do an in-depth fix. It doesn't seem\ncrazy to apply the patch, since it fixes the common case, and adds tests\nfor it, but I don't care personnaly about the feature/bug, so I won't\nfight if it's rejected. I can resend with a more explicit comment in the\ncode saying it would deserve a better fix if someone cares.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"163096","messageId":"7vk4g87wvf.fsf@alter.siamese.dyndns.org","threadId":"26675","inReplyTo":"20110309213824.GA4400@sigill.intra.peff.net","subject":"Re: [PATCH] log: fix --max-count when used together with -S or -G","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-09T22:27:32Z","receivedAt":"2011-03-09T22:27:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hmm. Is this papering over a bigger problem,...\n\nIt is not very obvious to me if redefining the semantics of filtering done\nby diff (the current definition is it is purely an output phase thing) is\nnecessarily a good thing. I agree that the interaction between the output\nphase filtering and pruning done by the revision walker machinery is a\nfine topic to discuss.\n\nBut Matthieu's patch is not papering over anything but is a real fix\nwithin the context of the current architecture.\n"},{"id":"163182","messageId":"20110310223934.GF15828@sigill.intra.peff.net","threadId":"26675","inReplyTo":"7vk4g87wvf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] log: fix --max-count when used together with -S or -G","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-03-10T22:39:34Z","receivedAt":"2011-03-10T22:39:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 09, 2011 at 02:27:32PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Hmm. Is this papering over a bigger problem,...\n> \n> It is not very obvious to me if redefining the semantics of filtering done\n> by diff (the current definition is it is purely an output phase thing) is\n> necessarily a good thing. I agree that the interaction between the output\n> phase filtering and pruning done by the revision walker machinery is a\n> fine topic to discuss.\n> \n> But Matthieu's patch is not papering over anything but is a real fix\n> within the context of the current architecture.\n\nI consider the current state to be \"buggy but we live with it\" and not\nan architectural decision. But that is simply a matter of perspective. :)\n\nCertainly Matthieu's patch fixes a real problem, and it does not make it\nany harder to address the other problems in the future, so I think there\nis no reason not to apply it.\n\n-Peff\n"}]}