Re: [PATCH v2 11/27] blame tests: simplify userdiff driver test

On Mon, Feb 15 2021, Johannes Sixt wrote:

Am 15.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:

quoted

Simplify the test added in 9466e3809d (blame: enable funcname blaming
with userdiff driver, 2020-11-01) to use the --author support recently
added in 999cfc4f45 (test-lib functions: add --author support to
test_commit, 2021-01-12).
We also did not need the full fortran-external-function content,
let's
cut it down to just the important parts, and further modify it to
demonstrate that the fortran-specific userdiff function is in effect
by adding "WRONG" lines surrounding the "RIGHT" one.
The test also left behind a .gitattributes files, let's clean it up
with "test_when_finished".
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
  t/annotate-tests.sh | 36 +++++++++++++++---------------------
  1 file changed, 15 insertions(+), 21 deletions(-)

diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
index 04a2c58594..4a86e0f349 100644
--- a/t/annotate-tests.sh
+++ b/t/annotate-tests.sh

@@ -479,32 +479,26 @@ test_expect_success 'blame -L ^:RE (absolute: end-of-file)' '
  	check_count -f hello.c -L$n -L^:ma.. F 4 G 1 H 1
  '
  -test_expect_success 'setup -L :funcname with userdiff driver' '
-	echo "fortran-* diff=fortran" >.gitattributes &&
-	fortran_file=fortran-external-function &&
-	cat >$fortran_file <<-\EOF &&
+test_expect_success 'blame -L :funcname with userdiff driver' '
+	cat >file.template <<-\EOF &&
+	def WRONG begin end
  	function RIGHT(a, b) result(c)
+	int WRONG(void) {}
    	integer, intent(in) :: ChangeMe
-	integer, intent(in) :: b
-	integer, intent(out) :: c
-
-	c = a+b
-
-	end function RIGHT
  	EOF
-	git add "$fortran_file" &&
-	GIT_AUTHOR_NAME="A" GIT_AUTHOR_EMAIL="A@test.git" \
-	git commit -m "add fortran file" &&
-	sed -e "s/ChangeMe/IWasChanged/" <"$fortran_file" >"$fortran_file".tmp &&
-	mv "$fortran_file".tmp "$fortran_file" &&
-	git add "$fortran_file" &&
-	GIT_AUTHOR_NAME="B" GIT_AUTHOR_EMAIL="B@test.git" \
-	git commit -m "change fortran file"
-'
  -test_expect_success 'blame -L :funcname with userdiff driver' '
-	check_count -f fortran-external-function -L:RIGHT A 7 B 1
+	fortran_file=file.f03 &&
+	test_when_finished "rm .gitattributes" &&
+	echo "$fortran_file diff=fortran" >.gitattributes &&
+
+	test_commit --author "A <A@test.git>" \
+		"add" $fortran_file \
+		"$(cat file.template)" &&
+	test_commit --author "B <B@test.git>" \
+		"change" $fortran_file \
+		"$(cat file.template | sed -e s/ChangeMe/IWasChanged/)" &&
+	check_count -f $fortran_file -L:RIGHT A 3 B 1
  '
    test_expect_success 'setup incremental' '

I don't get the point. What do you need the tokens "WRONG" for when
they are not checked anywhere? Instead of adding unrelated lines (that
do not even look like Fortran), couldn't you just not remove some of
the others? In particular, the last one that contains "RIGHT" as well
may be useful to keep in order to show that the code is not confused
by it.

Isn't the point of the test to assert that we're using a userdiff driver
over the built-in xdiff rules here?

We can imagine that a change to its default heuristics would be to find
the first non-whitespace line, but it jumping over non-whitespace lines
in search of a fortran-looking line doesn't seem like it would ever
happen. Hence the WRONG lines.

Please place "$fortran_file" in dquotes on the check_count line.

Why do we need to dquote a convenience variable defined in the test
itself that'll never contain spaces or other funny things we'd get if we
had $(pwd) or whatever in there? It wouldn't hurt, but maybe I'm missing
some reason for why it's necessary or desired here.