Re: [PATCH] sequencer.c: terminate the last line of author-script properly

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 17 Jul 2018, Junio C Hamano wrote:

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 2d189da2f1..b0cef509ab 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -81,11 +81,13 @@ test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "ed

You missed a very long line here.

>  	set_fake_editor &&
>  	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
>  	test -f .git/rebase-merge/author-script &&

Why do we need this, if we already have an `eval` later on?

> -	unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> -	eval "$(cat .git/rebase-merge/author-script)" &&
> -	test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
> -	test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
> -	test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"
> +	(
> +		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> +		eval "$(cat .git/rebase-merge/author-script)" &&

Why not

	. .git/rebase-merge/author-script

instead? Less roundabout, easier to read, I think.

> +		test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&

How is this even working without `-s`?

*clicketyclick*

Ah, --quiet does this. Wait. `git show --quiet` is not even documented.

All of those lines are too long, though. I am surprised you did not catch
that.

Besides, this would be more compact, less repetitive, *and* more readable
as

	test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \
		"$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE"

t3404-rebase-interactive.sh already takes 8 minutes (last I checked,
anyway) to run on a *fast* machine. There is absolutely no need to
introduce even more spawning, not when it is so easily avoided.

> +		test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
> +		test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"

It is a shame that we cannot use %at directly here.

> +	)
>  '
>  
>  test_expect_success 'rebase -i with the exec command' '

Note: this is not a criticism of the original patch. It is a criticism of
the review which could really have been better.

I also saw that the test_when_finished uses a shell construct that shell
script aficionados might like, but these days it is a lot better to use
`test_might_fail` instead. Let's do this, then.

So here goes, the clean-up patch on top of your 843654e435e (why does it
have to be so darned tedious to get from a mail to the corresponding
commit in `pu`), in all its glory:

-- snipsnap --
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index b0cef509ab7..97f0b4bf881 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -75,18 +75,16 @@ test_expect_success 'rebase --keep-empty' '
 	test_line_count = 6 actual
 '
 
-test_expect_success 'rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse' '
-	test_when_finished "git rebase --abort ||:" &&
+test_expect_success 'rebase -i writes correct author-script' '
+	test_when_finished "test_might_fail git rebase --abort" &&
 	git checkout master &&
 	set_fake_editor &&
 	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
-	test -f .git/rebase-merge/author-script &&
 	(
 		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
-		eval "$(cat .git/rebase-merge/author-script)" &&
-		test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
-		test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
-		test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"
+		. .git/rebase-merge/author-script &&
+		test "$(git show -s --date=raw --format=%an,%ae,@%ad)" = \
+			"$GIT_AUTHOR_NAME,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE"
 	)
 '