Re: [PATCH] sequencer: detect author name errors in read_author_script()

[Jeff King <peff@peff.net>]

On Mon, Oct 03, 2022 at 11:40:52AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > +check_broken_author 'unknown key in author-script' '
> > +	echo "GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}" \
> > +		>>.git/rebase-merge/author-script'
> > +
> > +
> > +test_done
> 
> Maybe I just have too much PTSD from dealing with shell quoting issues
> with this '"$script"" pattern when you need to pass arguments with
> spaces in it, or even quotes. Although it probably won't ever be an
> issue here.

Quoting-wise, it's the exact same as passing the snippet to
test_expect_success that we already do. Which, granted, is full of
horrors, but is well understood within our code base. The "$script" is
evaluated when we pass the snippet to test_expect_success, which sees
the expansion along with the rest of the script. The double-quotes
around it ensure that whitespace is retained.

There is one extra quirk here, which is that it must not end with a
newline, since we stick "&&" on it.

> +	# Avoid quoting issues
> +	write_script script.sh &&
> +
> +	test_expect_success "$title of '$script'" '
>  		# create conflicted state
>  		test_when_finished "git rebase --abort" &&
>  		git checkout -B tmp branch2 &&
>  		test_must_fail git rebase branch1 &&
>  
> -		# break author-script
> -		'"$script"' &&
> +		./script.sh >tmp &&
> +		mv tmp .git/rebase-merge/author-script &&

One of my goals was that the script be expanded inline so that
verbose-mode showed what the test was actually doing. But this hides it
behind a vague "script.sh". Maybe that's not important enough to merit
the admitted ugliness of the loop+function we have.

If we give that up, then a better option is probably to put the
before/after parts in functions, like:

diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh
index e68aac4b36..b92a3ce46b 100755
--- a/t/t3438-rebase-broken-files.sh
+++ b/t/t3438-rebase-broken-files.sh
@@ -11,43 +11,49 @@ test_expect_success 'set up conflicting branches' '
 	test_commit two file
 '
 
-check_broken_author () {
-	title=$1; shift
-	script=$1; shift
-
-	test_expect_success "$title" '
-		# create conflicted state
-		test_when_finished "git rebase --abort" &&
-		git checkout -B tmp branch2 &&
-		test_must_fail git rebase branch1 &&
-
-		# break author-script
-		'"$script"' &&
-
-		# resolving notices broken author-script
-		echo resolved >file &&
-		git add file &&
-		test_must_fail git rebase --continue
-	'
+create_conflict () {
+	test_when_finished "git rebase --abort" &&
+	git checkout -B tmp branch2 &&
+	test_must_fail git rebase branch1
+}
+
+check_resolve_fails () {
+	echo resolved >file &&
+	git add file &&
+	test_must_fail git rebase --continue
 }
 
 for item in NAME EMAIL DATE
 do
-	check_broken_author "detect missing GIT_AUTHOR_$item" '
+	test_expect_success "detect missing GIT_AUTHOR_$item" '
+		create_conflict &&
+
 		grep -v $item .git/rebase-merge/author-script >tmp &&
-		mv tmp .git/rebase-merge/author-script'
+		mv tmp .git/rebase-merge/author-script &&
+
+		check_resolve_fails
+	'
 done
 
 for item in NAME EMAIL DATE
 do
-	check_broken_author "detect duplicate GIT_AUTHOR_$item" '
+	test_expect_success "detect duplicate GIT_AUTHOR_$item" '
+		create_conflict &&
+
 		grep -i $item .git/rebase-merge/author-script >tmp &&
-		cat tmp >>.git/rebase-merge/author-script'
+		cat tmp >>.git/rebase-merge/author-script &&
+
+		check_resolve_fails
+	'
 done
 
-check_broken_author 'unknown key in author-script' '
+test_expect_success 'unknown key in author-script' '
+	create_conflict &&
+
 	echo "GIT_AUTHOR_BOGUS=${SQ}whatever${SQ}" \
-		>>.git/rebase-merge/author-script'
+		>>.git/rebase-merge/author-script &&
 
+	check_resolve_fails
+'
 
 test_done

That makes the boilerplate shorter in the "-v" output but focuses on the
actual modification that breaks the author-script.

-Peff