Re: [PATCH v2 3/4] log --count: added test
Matthieu Moy 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] log --count: added test
Matthieu Moy 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] log --count: added test
Matthieu Moy 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] log --count: added test
Lawrence Siebert 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] log --count: added test
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 wrote: > Lawrence Siebert 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 >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] log --count: added test
Lawrence Siebert 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 > --- > 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
