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

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Matthieu Moy <hidden>
Date: 2016-06-15 23:05:38

Lawrence Siebert [off-list ref] 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.
quoted hunk
Signed-off-by: Lawrence Siebert <redacted>
---
 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

From: Lawrence Siebert <hidden>
Date: 2016-06-15 23:05:38

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
[off-list ref] wrote:
Lawrence Siebert [off-list ref] writes:
quoted
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.
quoted
Signed-off-by: Lawrence Siebert <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help