Re: [PATCH] Add test cases for git-am

[Stephan Beyer <s-beyer@gmx.net>]

Hi,

>     Mail-Followup-To: git@vger.kernel.org, gitster@pobox.com
> 
> Please don't do this "Mail-Followup-To:"; it is rude to others, and it is
> rude to me.

Oh, it was not meant to be rude.
In fact, I didn't even knew that my mutt generates it. ;)
So I deactivated it and I hope it helps you to enjoy your Caipirinha.

Btw, I'm confused whether I should use "List reply" (which only
replies to git@vger.kernel.org) or "Reply to all" (which I used now).
While on some other mailinglists it is discouraged to "reply to all",
it seems to be the common case here.

> > Perhaps it is also useful to swap t4150 and t4151 or to merge them.
> 
> Pehaps.  A single test t4150-am.sh might be enough.

Ok.

> > +test_expect_success setup '
> > +	echo hello >file &&
> > +	git add file &&
> > +	test_tick &&
> > +	git commit -m first &&
> > +	git tag first &&
> > +	echo world >>file &&
> > +	git add file &&
> > +	test_tick &&
> > +	git commit -s -F msg &&
> > +	git tag second &&
> > +	git format-patch --stdout first >patch1 &&
> > +	tail -n +3 msg >file &&
> 
> "tail -n 3" you mean?

No :-)
"tail -n 3" or "tail -n -3" results in the last three lines, but
"tail -n +3" results in the last lines beginning from the third line.

All three cases are POSIX-conform according to the online specs of 
IEEE or OpenGroup, e.g. see
	http://www.opengroup.org/onlinepubs/009695399/utilities/tail.html

> Same for "head -n <n>" in other places.

Here you are right that "head -n 3" equals "head -n +3", since both
result in the first three lines. ("head -n -3" results in all lines
except the last three.)
I kept the '+'-prefix for consistency, but I can remove it if you like.

> > +test_expect_success 'am changes committer and keeps author' '
> > +	test_tick &&
> > +	git checkout first &&
> > +	git am patch2 &&
> > +	! test -d .dotest &&
> > +	test "$(git rev-parse master)" != "$(git rev-parse HEAD)" &&
> > +	test "$(git rev-parse master^)" != "$(git rev-parse HEAD^)" &&
> > +	test "$(git rev-parse master^^)" = "$(git rev-parse HEAD^^)" &&
> > +	test -z "$(git diff master..HEAD)" &&
> > +	test -z "$(git diff master^..HEAD^)" &&
> > +	compare author master HEAD &&
> > +	compare author master^ HEAD^ &&
> > +	! compare committer master HEAD &&
> > +	! compare committer master^ HEAD^
> > +'
> 
> Hmmm.  Checking for inequality does not feel so robust.  You will allow
> "am" to record garbage and will not be able to detect a breakage.

Oh, right.

Does this feel better?
--- a/t/t4151-am.sh
+++ b/t/t4151-am.sh
@@ -123,15 +123,13 @@ test_expect_success 'am changes committer and keeps author' '
 	git checkout first &&
 	git am patch2 &&
 	! test -d .dotest &&
-	test "$(git rev-parse master)" != "$(git rev-parse HEAD)" &&
-	test "$(git rev-parse master^)" != "$(git rev-parse HEAD^)" &&
 	test "$(git rev-parse master^^)" = "$(git rev-parse HEAD^^)" &&
 	test -z "$(git diff master..HEAD)" &&
 	test -z "$(git diff master^..HEAD^)" &&
 	compare author master HEAD &&
 	compare author master^ HEAD^ &&
-	! compare committer master HEAD &&
-	! compare committer master^ HEAD^
+	test "Co M Miter <c.miter@example.com>" = \
+	     "$(git log -1 --pretty=format:"%cn <%ce>" HEAD)"
 '
 
 test_expect_success 'am --signoff adds Signed-off-by: line' '
---
Or better ideas? ;)

> > +test_expect_success 'am --signoff adds Signed-off-by: line' '
> > +	git checkout -b master2 first &&
> > +	git am --signoff <patch2 &&
> > +	test "$(git cat-file commit HEAD | grep -c "^Signed-off-by:")" -eq 1 &&
> > +	test "$(git cat-file commit HEAD^ | grep -c "^Signed-off-by:")" -eq 2
> > +'
> 
> Again, don't you want to check not just "It added something", but "It
> added what we expected it to add"?

Like this?
---
 test_expect_success 'am --signoff adds Signed-off-by: line' '
 	git checkout -b master2 first &&
 	git am --signoff <patch2 &&
 	test "$(git cat-file commit HEAD | grep -c "^Signed-off-by:")" -eq 1 &&
-	test "$(git cat-file commit HEAD^ | grep -c "^Signed-off-by:")" -eq 2
+	test "$(git cat-file commit HEAD^ | grep -c "^Signed-off-by:")" -eq 2 &&
+	git cat-file commit HEAD | grep -q "^Signed-off-by: Co M Miter <c.miter@example.com>$" &&
+	git cat-file commit HEAD^ | grep -q "^Signed-off-by: Co M Miter <c.miter@example.com>$" &&
+	git cat-file commit HEAD^ | grep -q "^Signed-off-by: C O Mitter <committer@example.com>$"
 '
---

Mh, I thought it is not bad to keep the -eq checks just to go sure nothing
is added twice by whatever reason.
 

Thank you!
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F