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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2018, 17:22 UTC
Message-ID: <xmqqwou0xtar.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <86a7qwpt9g.knu@iDaemons.org>

"Akinori MUSHA" <knu@iDaemons.org> writes:

> It looks like write_author_script() intends to write out a file in
> Bourne shell syntax, but it doesn't put a closing single quote on the
> last line.

s/closing single quote/& and the terminating newline/?

>
> This patch makes .git/rebase-merge/author-script actually parsable by
> sh(1) by adding a single quote and a linefeed to terminate the line
> properly.

Sounds good.

I wonder why this breakage was left unnoticed for a long time, though. It's not like writing and reading the author-script from C code was done first in the "rebase -i" and friends that are users of the sequencer machinery (I think we had code to do so in "git am" that was rewritten in C first). Do we have a similar issue over there as well? If not, perhaps if we reused the existing code that was not broken, we wouldn't have seen this breakage on the sequencer side?

Thanks.

Show 45 quoted lines

>
> Signed-off-by: Akinori MUSHA <knu@idaemons.org>
> ---
>  sequencer.c                   |  1 +
>  t/t3404-rebase-interactive.sh | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 4034c0461..5f32b6df1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -651,6 +651,7 @@ static int write_author_script(const char *message)
>  			strbuf_addch(&buf, *(message++));
>  		else
>  			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +	strbuf_addstr(&buf, "'\n");
>  	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
>  	strbuf_release(&buf);
>  	return res;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 352a52e59..345b103eb 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -75,6 +75,19 @@ 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 ||:" &&
> +	git checkout master &&
> +	set_fake_editor &&
> +	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
> +	test -f .git/rebase-merge/author-script &&
> +	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"
> +'
> +
>  test_expect_success 'rebase -i with the exec command' '
>  	git checkout master &&
>  	(
> -- 
> 2.18.0