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

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

[Akinori MUSHA]

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.

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.

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


-- 
Akinori MUSHA / https://akinori.org/

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

[Junio C Hamano]

"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.

>
> 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

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

[Phillip Wood]

Hi Junio

On 12/07/18 18:22, Junio C Hamano wrote:
> 
> "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 

The only consumer of a faulty author script written by the sequencer is 
read_env_script() in sequencer.c which doesn't worry about checking that 
quotes are paired. The shell versions of rebase use the code in 
git-sh-setup.sh to create the author script which does write valid posix 
shell.

> (I think we had code to do so in "git am"
> that was rewritten in C first).

The code in builtin/am.c doesn't try to write valid posix shell (if one 
assumes it is the only consumer of the author script then it doesn't 
need to) which results in simpler code, but external scripts cannot 
safely eval it anymore.

>  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?

Best Wishes

Phillip

> Thanks.
> 
>>
>> 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

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

[Phillip Wood]

On 18/07/18 10:45, Phillip Wood wrote:
> 
> Hi Junio
> 
> On 12/07/18 18:22, Junio C Hamano wrote:
>>
>> "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 
> 
> The only consumer of a faulty author script written by the sequencer is 
> read_env_script() in sequencer.c which doesn't worry about checking that 
> quotes are paired. 

That's not quite true anymore, recently another consumer 
read_author_ident() was added which uses sq_dequote() instead of custom 
code. Looking more closely at write_author_script() the quoting of 
single quotes is buggy they are escaped as \\' instead of \'. The code 
in read_env_script() expects the buggy quoting so it works. I just tried 
the code path that uses sq_dequote() by doing rebase --root with an 
author containing a single quote and that worked but I'm not sure why.

The shell versions of rebase use the code in
> git-sh-setup.sh to create the author script which does write valid posix 
> shell.
> 
>> (I think we had code to do so in "git am"
>> that was rewritten in C first).
> 
> The code in builtin/am.c doesn't try to write valid posix shell (if one 
> assumes it is the only consumer of the author script then it doesn't 
> need to) 

Though it will break if git gets upgraded while am is stopped for a 
conflict resolution. Given that the code has been around for a while 
that may not be much of a concern now

which results in simpler code, but external scripts cannot
> safely eval it anymore.
> 
>>  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?
> 
> Best Wishes
> 
> Phillip
> 
>> Thanks.
>>
>>>
>>> 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
> 

[RFC PATCH] sequencer: fix quoting in write_author_script

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Single quotes should be escaped as \' not \\'. Note that this only
affects authors that contain a single quote and then only external
scripts that read the author script and users whose git is upgraded from
the shell version of rebase -i while rebase was stopped. This is because
the parsing in read_env_script() expected the broken version and for
some reason sq_dequote() called by read_author_ident() seems to handle
the broken quoting correctly.

Ideally write_author_script() would be rewritten to use
split_ident_line() and sq_quote_buf() but this commit just fixes the
immediate bug.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

This is untested, unfortuantely I don't have really have time to write a test or
follow this up at the moment, if someone else want to run with it then please
do.

sequencer.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..0b78d1f100 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -638,21 +638,21 @@ static int write_author_script(const char *message)
 		else if (*message != '\'')
 			strbuf_addch(&buf, *(message++));
 		else
-			strbuf_addf(&buf, "'\\\\%c'", *(message++));
+			strbuf_addf(&buf, "'\\%c'", *(message++));
 	strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");
 	while (*message && *message != '\n' && *message != '\r')
 		if (skip_prefix(message, "> ", &message))
 			break;
 		else if (*message != '\'')
 			strbuf_addch(&buf, *(message++));
 		else
-			strbuf_addf(&buf, "'\\\\%c'", *(message++));
+			strbuf_addf(&buf, "'\\%c'", *(message++));
 	strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");
 	while (*message && *message != '\n' && *message != '\r')
 		if (*message != '\'')
 			strbuf_addch(&buf, *(message++));
 		else
-			strbuf_addf(&buf, "'\\\\%c'", *(message++));
+			strbuf_addf(&buf, "'\\%c'", *(message++));
 	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
 	strbuf_release(&buf);
 	return res;
@@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
 {
 	struct strbuf script = STRBUF_INIT;
 	int i, count = 0;
-	char *p, *p2;
+	const char *p2;
+	char *p;
 
 	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
 		return -1;
 
 	for (p = script.buf; *p; p++)
-		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
+		/*
+		 * write_author_script() used to escape "'" incorrectly as
+		 * "'\\\\''" rather than "'\\''" so we check for the correct
+		 * version the incorrect version in case git was upgraded while
+		 * rebase was stopped.
+		 */
+		if (skip_prefix(p, "'\\''", &p2) ||
+		    skip_prefix(p, "'\\\\''", &p2))
 			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
 		else if (*p == '\'')
 			strbuf_splice(&script, p-- - script.buf, 1, "", 0);
-- 
2.18.0

Re: [RFC PATCH] sequencer: fix quoting in write_author_script

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Single quotes should be escaped as \' not \\'. Note that this only
> affects authors that contain a single quote and then only external
> scripts that read the author script and users whose git is upgraded from
> the shell version of rebase -i while rebase was stopped. This is because
> the parsing in read_env_script() expected the broken version and for
> some reason sq_dequote() called by read_author_ident() seems to handle
> the broken quoting correctly.
>
> Ideally write_author_script() would be rewritten to use
> split_ident_line() and sq_quote_buf() but this commit just fixes the
> immediate bug.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>
> This is untested, unfortuantely I don't have really have time to write a test or
> follow this up at the moment, if someone else want to run with it then please
> do.

Any follow-up on this?

Dscho, I do agree with Phillip that the author-script this code
produces does not quote single quotes correctly (if we consider that
the author-script is meant to be compatible with shell script
version and what "git am" uses, that is), and Phillip's fix looks
like a reasonable first step going forward (the next step would
remove the transitional band-aid from the reading code, also added
by this patch).

Thanks.

>
> sequencer.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..0b78d1f100 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)
>  		else if (*message != '\'')
>  			strbuf_addch(&buf, *(message++));
>  		else
> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");
>  	while (*message && *message != '\n' && *message != '\r')
>  		if (skip_prefix(message, "> ", &message))
>  			break;
>  		else if (*message != '\'')
>  			strbuf_addch(&buf, *(message++));
>  		else
> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");
>  	while (*message && *message != '\n' && *message != '\r')
>  		if (*message != '\'')
>  			strbuf_addch(&buf, *(message++));
>  		else
> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>  	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
>  	strbuf_release(&buf);
>  	return res;
> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
>  {
>  	struct strbuf script = STRBUF_INIT;
>  	int i, count = 0;
> -	char *p, *p2;
> +	const char *p2;
> +	char *p;
>  
>  	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>  		return -1;
>  
>  	for (p = script.buf; *p; p++)
> -		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> +		/*
> +		 * write_author_script() used to escape "'" incorrectly as
> +		 * "'\\\\''" rather than "'\\''" so we check for the correct
> +		 * version the incorrect version in case git was upgraded while
> +		 * rebase was stopped.
> +		 */
> +		if (skip_prefix(p, "'\\''", &p2) ||
> +		    skip_prefix(p, "'\\\\''", &p2))
>  			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
>  		else if (*p == '\'')
>  			strbuf_splice(&script, p-- - script.buf, 1, "", 0);

Re: [RFC PATCH] sequencer: fix quoting in write_author_script

[Johannes Schindelin]

Hi Phillip,

On Wed, 18 Jul 2018, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> Single quotes should be escaped as \' not \\'. Note that this only
> affects authors that contain a single quote and then only external
> scripts that read the author script and users whose git is upgraded from
> the shell version of rebase -i while rebase was stopped. This is because
> the parsing in read_env_script() expected the broken version and for
> some reason sq_dequote() called by read_author_ident() seems to handle
> the broken quoting correctly.
> 
> Ideally write_author_script() would be rewritten to use
> split_ident_line() and sq_quote_buf() but this commit just fixes the
> immediate bug.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---

Good catch.

> This is untested, unfortuantely I don't have really have time to write a test or
> follow this up at the moment, if someone else want to run with it then please
> do.

I modified the test that was added by Akinori. As it was added very early,
and as there is still a test case *after* Akinori's that compares a
hard-coded SHA-1, I refrained from using `test_commit` (which would change
that SHA-1). See below.

> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..0b78d1f100 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)
>  		else if (*message != '\'')
>  			strbuf_addch(&buf, *(message++));
>  		else
> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");
>  	while (*message && *message != '\n' && *message != '\r')
>  		if (skip_prefix(message, "> ", &message))
>  			break;
>  		else if (*message != '\'')
>  			strbuf_addch(&buf, *(message++));
>  		else
> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");
>  	while (*message && *message != '\n' && *message != '\r')
>  		if (*message != '\'')
>  			strbuf_addch(&buf, *(message++));
>  		else
> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>  	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);

I resolved the merge conflict with Akinori's patch. FWIW I pushed all of
this, including the fixup to Junio's fixup to the
`fix-t3404-author-script-test` branch at https://github.com/dscho/git.

>  	strbuf_release(&buf);
>  	return res;
> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
>  {
>  	struct strbuf script = STRBUF_INIT;
>  	int i, count = 0;
> -	char *p, *p2;
> +	const char *p2;
> +	char *p;
>  
>  	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>  		return -1;
>  
>  	for (p = script.buf; *p; p++)
> -		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> +		/*
> +		 * write_author_script() used to escape "'" incorrectly as
> +		 * "'\\\\''" rather than "'\\''" so we check for the correct
> +		 * version the incorrect version in case git was upgraded while
> +		 * rebase was stopped.
> +		 */
> +		if (skip_prefix(p, "'\\''", &p2) ||
> +		    skip_prefix(p, "'\\\\''", &p2))

I think in this form, it is possibly unsafe because it assumes that the
new code cannot generate output that would trigger that same code path.
Although I have to admit that I did not give this a great deal of thought.

In any case, if you have to think long and hard about some fix, it might
be better to go with something that is easier to reason about. So how
about this: we already know that the code is buggy, Akinori fixed the bug,
where the author-script missed its trailing single-quote. We can use this
as a tell-tale for *this* bug. Assuming that Junio will advance both your
and Akinori's fix in close proximity.

Again, this is pushed to the `fix-t3404-author-script-test` branch at
https://github.com/dscho/git; My fixup on top of your patch looks like
this (feel free to drop the sq_bug part and only keep the test part):

-- snipsnap --
diff --git a/sequencer.c b/sequencer.c
index 46c0b3e720f..7abe78dc78e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -573,13 +573,14 @@ static int write_author_script(const char *message)
 static int read_env_script(struct argv_array *env)
 {
 	struct strbuf script = STRBUF_INIT;
-	int i, count = 0;
+	int i, count = 0, sq_bug;
 	const char *p2;
 	char *p;
 
 	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
 		return -1;
 
+	sq_bug = script.len && script.buf[script.len - 1] != '\'';
 	for (p = script.buf; *p; p++)
 		/*
 		 * write_author_script() used to escape "'" incorrectly as
@@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)
 		 * version the incorrect version in case git was upgraded while
 		 * rebase was stopped.
 		 */
-		if (skip_prefix(p, "'\\''", &p2) ||
-		    skip_prefix(p, "'\\\\''", &p2))
+		if (sq_bug && skip_prefix(p, "'\\\\''", &p2))
+			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
+		else if (skip_prefix(p, "'\\''", &p2))
 			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
 		else if (*p == '\'')
 			strbuf_splice(&script, p-- - script.buf, 1, "", 0);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 97f0b4bf881..dd726ff4dc4 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -75,16 +75,18 @@ test_expect_success 'rebase --keep-empty' '
 	test_line_count = 6 actual
 '
 
+SQ="'"
 test_expect_success 'rebase -i writes correct author-script' '
 	test_when_finished "test_might_fail git rebase --abort" &&
-	git checkout master &&
+	git checkout -b author-with-sq master &&
+	GIT_AUTHOR_NAME="Auth O$SQ R" git commit --allow-empty -m with-sq &&
 	set_fake_editor &&
-	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
+	FAKE_LINES="edit 1" git rebase -ki HEAD^ &&
 	(
 		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL 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"
+			"Auth O$SQ R,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE"
 	)
 '
 
@@ -1347,7 +1349,6 @@ test_expect_success 'editor saves as CR/LF' '
 	)
 '
 
-SQ="'"
 test_expect_success 'rebase -i --gpg-sign=<key-id>' '
 	set_fake_editor &&
 	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \

Re: [RFC PATCH] sequencer: fix quoting in write_author_script

[Phillip Wood]

Hi Johannes
On 26/07/18 13:33, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Wed, 18 Jul 2018, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Single quotes should be escaped as \' not \\'. Note that this only
>> affects authors that contain a single quote and then only external
>> scripts that read the author script and users whose git is upgraded from
>> the shell version of rebase -i while rebase was stopped. This is because
>> the parsing in read_env_script() expected the broken version and for
>> some reason sq_dequote() called by read_author_ident() seems to handle
>> the broken quoting correctly.
>>
>> Ideally write_author_script() would be rewritten to use
>> split_ident_line() and sq_quote_buf() but this commit just fixes the
>> immediate bug.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
> 
> Good catch.
> 
>> This is untested, unfortuantely I don't have really have time to write a test or
>> follow this up at the moment, if someone else want to run with it then please
>> do.
> 
> I modified the test that was added by Akinori. As it was added very early,
> and as there is still a test case *after* Akinori's that compares a
> hard-coded SHA-1, I refrained from using `test_commit` (which would change
> that SHA-1). See below.

Thanks for adding a test, that sounds like sensible approach, however
having thought about it I wonder if we should just be writing a plain
text file (e.g rebase-merge/author-data) and fixing the reader to read
that if it exists and only then fall back to reading the legacy
rebase-merge/author-script with a fix to correctly handle the script
written by the shell version - what do you think? The author-script
really should be just an implementation detail. If anyone really wants
to read it they can still do 'read -r l' and split the lines with
${l%%=*} and ${l#*=}

>> diff --git a/sequencer.c b/sequencer.c
>> index 5354d4d51e..0b78d1f100 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)
>>  		else if (*message != '\'')
>>  			strbuf_addch(&buf, *(message++));
>>  		else
>> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
>> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");
>>  	while (*message && *message != '\n' && *message != '\r')
>>  		if (skip_prefix(message, "> ", &message))
>>  			break;
>>  		else if (*message != '\'')
>>  			strbuf_addch(&buf, *(message++));
>>  		else
>> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
>> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");
>>  	while (*message && *message != '\n' && *message != '\r')
>>  		if (*message != '\'')
>>  			strbuf_addch(&buf, *(message++));
>>  		else
>> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
>> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>>  	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
> 
> I resolved the merge conflict with Akinori's patch. FWIW I pushed all of
> this, including the fixup to Junio's fixup to the
> `fix-t3404-author-script-test` branch at https://github.com/dscho/git.
> 
>>  	strbuf_release(&buf);
>>  	return res;
>> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
>>  {
>>  	struct strbuf script = STRBUF_INIT;
>>  	int i, count = 0;
>> -	char *p, *p2;
>> +	const char *p2;
>> +	char *p;
>>  
>>  	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>>  		return -1;
>>  
>>  	for (p = script.buf; *p; p++)
>> -		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
>> +		/*
>> +		 * write_author_script() used to escape "'" incorrectly as
>> +		 * "'\\\\''" rather than "'\\''" so we check for the correct
>> +		 * version the incorrect version in case git was upgraded while
>> +		 * rebase was stopped.
>> +		 */
>> +		if (skip_prefix(p, "'\\''", &p2) ||
>> +		    skip_prefix(p, "'\\\\''", &p2))
> 
> I think in this form, it is possibly unsafe because it assumes that the
> new code cannot generate output that would trigger that same code path.
> Although I have to admit that I did not give this a great deal of thought.

Hm, I not sure that it can. If the Author begins \\' then this will be
written as the C string "'\\\\'\\''...". If \\' comes at the end then I
think this will be written as "\\\\'\\'''", in the middle of the name it
will be "\\\\'\\''..."

> In any case, if you have to think long and hard about some fix, it might
> be better to go with something that is easier to reason about. So how
> about this: we already know that the code is buggy, Akinori fixed the bug,
> where the author-script missed its trailing single-quote. We can use this
> as a tell-tale for *this* bug. Assuming that Junio will advance both your
> and Akinori's fix in close proximity.

That sounds like a good approach
> Again, this is pushed to the `fix-t3404-author-script-test` branch at
> https://github.com/dscho/git; My fixup on top of your patch looks like
> this (feel free to drop the sq_bug part and only keep the test part):
> 
> -- snipsnap --
> diff --git a/sequencer.c b/sequencer.c
> index 46c0b3e720f..7abe78dc78e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -573,13 +573,14 @@ static int write_author_script(const char *message)
>  static int read_env_script(struct argv_array *env)
>  {
>  	struct strbuf script = STRBUF_INIT;
> -	int i, count = 0;
> +	int i, count = 0, sq_bug;
>  	const char *p2;
>  	char *p;
>  
>  	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>  		return -1;
>  
> +	sq_bug = script.len && script.buf[script.len - 1] != '\'';
>  	for (p = script.buf; *p; p++)
>  		/*
>  		 * write_author_script() used to escape "'" incorrectly as
> @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)
>  		 * version the incorrect version in case git was upgraded while
>  		 * rebase was stopped.
>  		 */

We probably want the change the comment slightly to explain sq_bug

> -		if (skip_prefix(p, "'\\''", &p2) ||
> -		    skip_prefix(p, "'\\\\''", &p2))
> +		if (sq_bug && skip_prefix(p, "'\\\\''", &p2))
> +			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
> +		else if (skip_prefix(p, "'\\''", &p2))
>  			strbuf_splice(&script, p - script.buf, p2 - p, "'", 1);
>  		else if (*p == '\'')
>  			strbuf_splice(&script, p-- - script.buf, 1, "", 0);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 97f0b4bf881..dd726ff4dc4 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -75,16 +75,18 @@ test_expect_success 'rebase --keep-empty' '
>  	test_line_count = 6 actual
>  '
>  
> +SQ="'"
>  test_expect_success 'rebase -i writes correct author-script' '
>  	test_when_finished "test_might_fail git rebase --abort" &&
> -	git checkout master &&
> +	git checkout -b author-with-sq master &&
> +	GIT_AUTHOR_NAME="Auth O$SQ R" git commit --allow-empty -m with-sq &&
>  	set_fake_editor &&
> -	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
> +	FAKE_LINES="edit 1" git rebase -ki HEAD^ &&
>  	(
>  		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL 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"
> +			"Auth O$SQ R,$GIT_AUTHOR_EMAIL,$GIT_AUTHOR_DATE"
>  	)
>  '

Testing all three variables together is nice.

Best Wishes

Phillip

> @@ -1347,7 +1349,6 @@ test_expect_success 'editor saves as CR/LF' '
>  	)
>  '
>  
> -SQ="'"
>  test_expect_success 'rebase -i --gpg-sign=<key-id>' '
>  	set_fake_editor &&
>  	FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \
> 

Re: [RFC PATCH] sequencer: fix quoting in write_author_script

[Johannes Schindelin]

Hi Phillip, Junio and Akinori,

I just noticed that t3404 is broken without my patches (but with Junio's
fixup), on Windows, macOS and Linux. (See log at the end.)

On Fri, 27 Jul 2018, Phillip Wood wrote:

> On 26/07/18 13:33, Johannes Schindelin wrote:
> > 
> > On Wed, 18 Jul 2018, Phillip Wood wrote:
> > 
> >> Single quotes should be escaped as \' not \\'. Note that this only
> >> affects authors that contain a single quote and then only external
> >> scripts that read the author script and users whose git is upgraded from
> >> the shell version of rebase -i while rebase was stopped. This is because
> >> the parsing in read_env_script() expected the broken version and for
> >> some reason sq_dequote() called by read_author_ident() seems to handle
> >> the broken quoting correctly.
> >>
> >> Ideally write_author_script() would be rewritten to use
> >> split_ident_line() and sq_quote_buf() but this commit just fixes the
> >> immediate bug.
> > 
> >> This is untested, unfortuantely I don't have really have time to write a test or
> >> follow this up at the moment, if someone else want to run with it then please
> >> do.
> > 
> > I modified the test that was added by Akinori. As it was added very early,
> > and as there is still a test case *after* Akinori's that compares a
> > hard-coded SHA-1, I refrained from using `test_commit` (which would change
> > that SHA-1). See below.
> 
> Thanks for adding a test, that sounds like sensible approach, however
> having thought about it I wonder if we should just be writing a plain
> text file (e.g rebase-merge/author-data) and fixing the reader to read
> that if it exists and only then fall back to reading the legacy
> rebase-merge/author-script with a fix to correctly handle the script
> written by the shell version - what do you think? The author-script
> really should be just an implementation detail. If anyone really wants
> to read it they can still do 'read -r l' and split the lines with
> ${l%%=*} and ${l#*=}

In contrast to `git am`, there *is* a use case where power users might
have come to rely on the presence of the .git/rebase-merge/author-script
file *and* its nature as a shell script snippet: we purposefully allow
scripting `rebase -i`.

So I don't think that we can declare the file and its format as
implementation detail, even if the idea is very, very tempting.

> >> diff --git a/sequencer.c b/sequencer.c
> >> index 5354d4d51e..0b78d1f100 100644
> >> --- a/sequencer.c
> >> +++ b/sequencer.c
> >> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)
> >>  		else if (*message != '\'')
> >>  			strbuf_addch(&buf, *(message++));
> >>  		else
> >> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> >> +			strbuf_addf(&buf, "'\\%c'", *(message++));
> >>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");
> >>  	while (*message && *message != '\n' && *message != '\r')
> >>  		if (skip_prefix(message, "> ", &message))
> >>  			break;
> >>  		else if (*message != '\'')
> >>  			strbuf_addch(&buf, *(message++));
> >>  		else
> >> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> >> +			strbuf_addf(&buf, "'\\%c'", *(message++));
> >>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");
> >>  	while (*message && *message != '\n' && *message != '\r')
> >>  		if (*message != '\'')
> >>  			strbuf_addch(&buf, *(message++));
> >>  		else
> >> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> >> +			strbuf_addf(&buf, "'\\%c'", *(message++));
> >>  	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
> > 
> > I resolved the merge conflict with Akinori's patch. FWIW I pushed all of
> > this, including the fixup to Junio's fixup to the
> > `fix-t3404-author-script-test` branch at https://github.com/dscho/git.
> > 
> >>  	strbuf_release(&buf);
> >>  	return res;
> >> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
> >>  {
> >>  	struct strbuf script = STRBUF_INIT;
> >>  	int i, count = 0;
> >> -	char *p, *p2;
> >> +	const char *p2;
> >> +	char *p;
> >>  
> >>  	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> >>  		return -1;
> >>  
> >>  	for (p = script.buf; *p; p++)
> >> -		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
> >> +		/*
> >> +		 * write_author_script() used to escape "'" incorrectly as
> >> +		 * "'\\\\''" rather than "'\\''" so we check for the correct
> >> +		 * version the incorrect version in case git was upgraded while
> >> +		 * rebase was stopped.
> >> +		 */
> >> +		if (skip_prefix(p, "'\\''", &p2) ||
> >> +		    skip_prefix(p, "'\\\\''", &p2))
> > 
> > I think in this form, it is possibly unsafe because it assumes that the
> > new code cannot generate output that would trigger that same code path.
> > Although I have to admit that I did not give this a great deal of thought.
> 
> Hm, I not sure that it can. If the Author begins \\' then this will be
> written as the C string "'\\\\'\\''...". If \\' comes at the end then I
> think this will be written as "\\\\'\\'''", in the middle of the name it
> will be "\\\\'\\''..."

Yes, that matches my gut feeling... but...

> > In any case, if you have to think long and hard about some fix, it might
> > be better to go with something that is easier to reason about. So how
> > about this: we already know that the code is buggy, Akinori fixed the bug,
> > where the author-script missed its trailing single-quote. We can use this
> > as a tell-tale for *this* bug. Assuming that Junio will advance both your
> > and Akinori's fix in close proximity.
> 
> That sounds like a good approach

I am glad that you agree to this.

I am a big fan of this age-old wisdom
that goes somewhat like this: some code is so simple that there is no
space for obvious bugs, and some code is so complicated that there is no
space for obvious bugs. In this context, I would use the modified
version: some code is so easy to reason about that there is no obvious
flaw, and some other code is so difficult to reason about that there is
no obvious flaw.

Besides, using the sq_bug version can serve as a reminder to "pay down
the technical debt" in the future.

> > Again, this is pushed to the `fix-t3404-author-script-test` branch at
> > https://github.com/dscho/git; My fixup on top of your patch looks like
> > this (feel free to drop the sq_bug part and only keep the test part):
> > 
> > -- snipsnap --
> > diff --git a/sequencer.c b/sequencer.c
> > index 46c0b3e720f..7abe78dc78e 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -573,13 +573,14 @@ static int write_author_script(const char *message)
> >  static int read_env_script(struct argv_array *env)
> >  {
> >  	struct strbuf script = STRBUF_INIT;
> > -	int i, count = 0;
> > +	int i, count = 0, sq_bug;
> >  	const char *p2;
> >  	char *p;
> >  
> >  	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
> >  		return -1;
> >  
> > +	sq_bug = script.len && script.buf[script.len - 1] != '\'';
> >  	for (p = script.buf; *p; p++)
> >  		/*
> >  		 * write_author_script() used to escape "'" incorrectly as
> > @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)
> >  		 * version the incorrect version in case git was upgraded while
> >  		 * rebase was stopped.
> >  		 */
> 
> We probably want the change the comment slightly to explain sq_bug

True. Can you give it a shot? If not, I will try to remember some time mid
next week.

Here the promised log of t3404 with -i -v -x (on macOS, but Linux and
Windows shows equivalent failures, and the way I read it, the problem is
simply that the test was introduced in the middle of t3404 and
subsequent test cases' assumptions are no longer met):

-- snipsnap --
2018-07-27T11:53:04.2450890Z ok 2 - rebase --keep-empty
2018-07-27T11:53:04.2464690Z 
2018-07-27T11:53:04.2489730Z expecting success: 
2018-07-27T11:53:04.2513840Z 	test_when_finished "git rebase --abort ||:" &&
2018-07-27T11:53:04.2536980Z 	git checkout master &&
2018-07-27T11:53:04.2563050Z 	set_fake_editor &&
2018-07-27T11:53:04.2590300Z 	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
2018-07-27T11:53:04.2614900Z 	test -f .git/rebase-merge/author-script &&
2018-07-27T11:53:04.2639740Z 	(
2018-07-27T11:53:04.2665320Z 		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
2018-07-27T11:53:04.2688410Z 		eval "$(cat .git/rebase-merge/author-script)" &&
2018-07-27T11:53:04.2710930Z 		test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
2018-07-27T11:53:04.2734740Z 		test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
2018-07-27T11:53:04.2756700Z 		test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"
2018-07-27T11:53:04.2778060Z 	)
2018-07-27T11:53:04.2788910Z 
2018-07-27T11:53:04.2810410Z ++ test_when_finished 'git rebase --abort ||:'
2018-07-27T11:53:04.2831430Z ++ test 0 = 0
2018-07-27T11:53:04.2852830Z ++ test_cleanup='{ git rebase --abort ||:
2018-07-27T11:53:04.2874430Z 		} && (exit "$eval_ret"); eval_ret=$?; :'
2018-07-27T11:53:04.2892720Z ++ git checkout master
2018-07-27T11:53:04.2911560Z Switched to branch 'master'
2018-07-27T11:53:04.2929490Z ++ set_fake_editor
2018-07-27T11:53:04.2947690Z ++ write_script fake-editor.sh
2018-07-27T11:53:04.2966040Z ++ echo '#!/bin/sh'
2018-07-27T11:53:04.2983780Z ++ cat
2018-07-27T11:53:04.3001950Z ++ chmod +x fake-editor.sh
2018-07-27T11:53:04.3019770Z +++ pwd
2018-07-27T11:53:04.3038530Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
2018-07-27T11:53:04.3057950Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
2018-07-27T11:53:04.3076510Z ++ export FAKE_EDITOR
2018-07-27T11:53:04.3094750Z ++ EDITOR='"$FAKE_EDITOR"'
2018-07-27T11:53:04.3112610Z ++ export EDITOR
2018-07-27T11:53:04.3130670Z ++ FAKE_LINES='edit 1'
2018-07-27T11:53:04.3153990Z ++ git rebase -i 'HEAD^'
2018-07-27T11:53:04.4674460Z rebase -i script before editing:
2018-07-27T11:53:04.4698420Z pick 8f99a4f E
2018-07-27T11:53:04.4710290Z 
2018-07-27T11:53:04.4792780Z rebase -i script after editing:
2018-07-27T11:53:04.4817630Z edit 8f99a4f E
2018-07-27T11:53:04.5025190Z Rebasing (1/1)
2018-07-27T11:53:04.5046400Z Stopped at 8f99a4f...  E
2018-07-27T11:53:04.5065680Z You can amend the commit now, with
2018-07-27T11:53:04.5074810Z 
2018-07-27T11:53:04.5092850Z   git commit --amend 
2018-07-27T11:53:04.5101800Z 
2018-07-27T11:53:04.5119900Z Once you are satisfied with your changes, run
2018-07-27T11:53:04.5129030Z 
2018-07-27T11:53:04.5147390Z   git rebase --continue
2018-07-27T11:53:04.5165610Z ++ test -f .git/rebase-merge/author-script
2018-07-27T11:53:04.5183820Z ++ sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
2018-07-27T11:53:04.5201610Z ++ unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
2018-07-27T11:53:04.5220730Z ++ return 0
2018-07-27T11:53:04.5244590Z +++ cat .git/rebase-merge/author-script
2018-07-27T11:53:04.5269130Z ++ eval 'GIT_AUTHOR_NAME='\''A U Thor'\''
2018-07-27T11:53:04.5294440Z GIT_AUTHOR_EMAIL='\''author@example.com'\''
2018-07-27T11:53:04.5319550Z GIT_AUTHOR_DATE='\''@1112912233 -0700'\'''
2018-07-27T11:53:04.5345580Z +++ GIT_AUTHOR_NAME='A U Thor'
2018-07-27T11:53:04.5365960Z +++ GIT_AUTHOR_EMAIL=author@example.com
2018-07-27T11:53:04.5384150Z +++ GIT_AUTHOR_DATE='@1112912233 -0700'
2018-07-27T11:53:04.5402250Z +++ git show --quiet --pretty=format:%an
2018-07-27T11:53:04.5420400Z ++ test 'A U Thor' = 'A U Thor'
2018-07-27T11:53:04.5438410Z +++ git show --quiet --pretty=format:%ae
2018-07-27T11:53:04.5459570Z ++ test author@example.com = author@example.com
2018-07-27T11:53:04.5477950Z +++ git show --quiet --date=raw --pretty=format:@%ad
2018-07-27T11:53:04.5496180Z ++ test '@1112912233 -0700' = '@1112912233 -0700'
2018-07-27T11:53:04.5514390Z ++ git rebase --abort
2018-07-27T11:53:04.6924920Z ++ exit 0
2018-07-27T11:53:04.6950890Z ++ eval_ret=0
2018-07-27T11:53:04.6973900Z ++ :
2018-07-27T11:53:04.6998480Z ok 3 - rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse
2018-07-27T11:53:04.7009400Z 
2018-07-27T11:53:04.7030290Z expecting success: 
2018-07-27T11:53:04.7050880Z 	set_fake_editor &&
2018-07-27T11:53:04.7072520Z 	test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 &&
2018-07-27T11:53:04.7092910Z 	test_i18ncmp expect actual
2018-07-27T11:53:04.7102960Z 
2018-07-27T11:53:04.7123150Z ++ set_fake_editor
2018-07-27T11:53:04.7144080Z ++ write_script fake-editor.sh
2018-07-27T11:53:04.7164970Z ++ echo '#!/bin/sh'
2018-07-27T11:53:04.7185360Z ++ cat
2018-07-27T11:53:04.7212950Z ++ chmod +x fake-editor.sh
2018-07-27T11:53:04.7236720Z +++ pwd
2018-07-27T11:53:04.7263500Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
2018-07-27T11:53:04.7291390Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
2018-07-27T11:53:04.7314090Z ++ export FAKE_EDITOR
2018-07-27T11:53:04.7338290Z ++ EDITOR='"$FAKE_EDITOR"'
2018-07-27T11:53:04.7362710Z ++ export EDITOR
2018-07-27T11:53:04.7386370Z ++ test_must_fail env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^'
2018-07-27T11:53:04.7412410Z ++ case "$1" in
2018-07-27T11:53:04.7436170Z ++ _test_ok=
2018-07-27T11:53:04.7455340Z ++ env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^'
2018-07-27T11:53:04.9560670Z ++ exit_code=0
2018-07-27T11:53:04.9581700Z ++ test 0 -eq 0
2018-07-27T11:53:04.9603200Z ++ list_contains '' success
2018-07-27T11:53:04.9628730Z ++ case ",$1," in
2018-07-27T11:53:04.9652120Z ++ return 1
2018-07-27T11:53:04.9677740Z ++ echo 'test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^'
2018-07-27T11:53:04.9704590Z test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^
2018-07-27T11:53:04.9730750Z ++ return 1
2018-07-27T11:53:04.9754580Z error: last command exited with $?=1
2018-07-27T11:53:04.9779100Z not ok 4 - rebase -i with empty HEAD
2018-07-27T11:53:04.9802670Z #	
2018-07-27T11:53:04.9826330Z #		set_fake_editor &&
2018-07-27T11:53:04.9851320Z #		test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 &&
2018-07-27T11:53:04.9875680Z #		test_i18ncmp expect actual
2018-07-27T11:53:04.9899170Z #	
2018-07-27T11:53:04.9923310Z make[1]: *** [t3404-rebase-interactive.sh] Error 1

Re: [RFC PATCH] sequencer: fix quoting in write_author_script

[Phillip Wood]

On 27/07/18 13:37, Johannes Schindelin wrote:
> Hi Phillip, Junio and Akinori,
> 
> I just noticed that t3404 is broken without my patches (but with Junio's
> fixup), on Windows, macOS and Linux. (See log at the end.)
> 
> On Fri, 27 Jul 2018, Phillip Wood wrote:
> 
>> On 26/07/18 13:33, Johannes Schindelin wrote:
>>>
>>> On Wed, 18 Jul 2018, Phillip Wood wrote:
>>>
>>>> Single quotes should be escaped as \' not \\'. Note that this only
>>>> affects authors that contain a single quote and then only external
>>>> scripts that read the author script and users whose git is upgraded from
>>>> the shell version of rebase -i while rebase was stopped. This is because
>>>> the parsing in read_env_script() expected the broken version and for
>>>> some reason sq_dequote() called by read_author_ident() seems to handle
>>>> the broken quoting correctly.
>>>>
>>>> Ideally write_author_script() would be rewritten to use
>>>> split_ident_line() and sq_quote_buf() but this commit just fixes the
>>>> immediate bug.
>>>
>>>> This is untested, unfortuantely I don't have really have time to write a test or
>>>> follow this up at the moment, if someone else want to run with it then please
>>>> do.
>>>
>>> I modified the test that was added by Akinori. As it was added very early,
>>> and as there is still a test case *after* Akinori's that compares a
>>> hard-coded SHA-1, I refrained from using `test_commit` (which would change
>>> that SHA-1). See below.
>>
>> Thanks for adding a test, that sounds like sensible approach, however
>> having thought about it I wonder if we should just be writing a plain
>> text file (e.g rebase-merge/author-data) and fixing the reader to read
>> that if it exists and only then fall back to reading the legacy
>> rebase-merge/author-script with a fix to correctly handle the script
>> written by the shell version - what do you think? The author-script
>> really should be just an implementation detail. If anyone really wants
>> to read it they can still do 'read -r l' and split the lines with
>> ${l%%=*} and ${l#*=}
> 
> In contrast to `git am`, there *is* a use case where power users might
> have come to rely on the presence of the .git/rebase-merge/author-script
> file *and* its nature as a shell script snippet: we purposefully allow
> scripting `rebase -i`.
> 
> So I don't think that we can declare the file and its format as
> implementation detail, even if the idea is very, very tempting.
> 
>>>> diff --git a/sequencer.c b/sequencer.c
>>>> index 5354d4d51e..0b78d1f100 100644
>>>> --- a/sequencer.c
>>>> +++ b/sequencer.c
>>>> @@ -638,21 +638,21 @@ static int write_author_script(const char *message)
>>>>  		else if (*message != '\'')
>>>>  			strbuf_addch(&buf, *(message++));
>>>>  		else
>>>> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
>>>> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>>>>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_EMAIL='");
>>>>  	while (*message && *message != '\n' && *message != '\r')
>>>>  		if (skip_prefix(message, "> ", &message))
>>>>  			break;
>>>>  		else if (*message != '\'')
>>>>  			strbuf_addch(&buf, *(message++));
>>>>  		else
>>>> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
>>>> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>>>>  	strbuf_addstr(&buf, "'\nGIT_AUTHOR_DATE='@");
>>>>  	while (*message && *message != '\n' && *message != '\r')
>>>>  		if (*message != '\'')
>>>>  			strbuf_addch(&buf, *(message++));
>>>>  		else
>>>> -			strbuf_addf(&buf, "'\\\\%c'", *(message++));
>>>> +			strbuf_addf(&buf, "'\\%c'", *(message++));
>>>>  	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
>>>
>>> I resolved the merge conflict with Akinori's patch. FWIW I pushed all of
>>> this, including the fixup to Junio's fixup to the
>>> `fix-t3404-author-script-test` branch at https://github.com/dscho/git.
>>>
>>>>  	strbuf_release(&buf);
>>>>  	return res;
>>>> @@ -666,13 +666,21 @@ static int read_env_script(struct argv_array *env)
>>>>  {
>>>>  	struct strbuf script = STRBUF_INIT;
>>>>  	int i, count = 0;
>>>> -	char *p, *p2;
>>>> +	const char *p2;
>>>> +	char *p;
>>>>  
>>>>  	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>>>>  		return -1;
>>>>  
>>>>  	for (p = script.buf; *p; p++)
>>>> -		if (skip_prefix(p, "'\\\\''", (const char **)&p2))
>>>> +		/*
>>>> +		 * write_author_script() used to escape "'" incorrectly as
>>>> +		 * "'\\\\''" rather than "'\\''" so we check for the correct
>>>> +		 * version the incorrect version in case git was upgraded while
>>>> +		 * rebase was stopped.
>>>> +		 */
>>>> +		if (skip_prefix(p, "'\\''", &p2) ||
>>>> +		    skip_prefix(p, "'\\\\''", &p2))
>>>
>>> I think in this form, it is possibly unsafe because it assumes that the
>>> new code cannot generate output that would trigger that same code path.
>>> Although I have to admit that I did not give this a great deal of thought.
>>
>> Hm, I not sure that it can. If the Author begins \\' then this will be
>> written as the C string "'\\\\'\\''...". If \\' comes at the end then I
>> think this will be written as "\\\\'\\'''", in the middle of the name it
>> will be "\\\\'\\''..."
> 
> Yes, that matches my gut feeling... but...
> 
>>> In any case, if you have to think long and hard about some fix, it might
>>> be better to go with something that is easier to reason about. So how
>>> about this: we already know that the code is buggy, Akinori fixed the bug,
>>> where the author-script missed its trailing single-quote. We can use this
>>> as a tell-tale for *this* bug. Assuming that Junio will advance both your
>>> and Akinori's fix in close proximity.
>>
>> That sounds like a good approach
> 
> I am glad that you agree to this.
> 
> I am a big fan of this age-old wisdom
> that goes somewhat like this: some code is so simple that there is no
> space for obvious bugs, and some code is so complicated that there is no
> space for obvious bugs. In this context, I would use the modified
> version: some code is so easy to reason about that there is no obvious
> flaw, and some other code is so difficult to reason about that there is
> no obvious flaw.
> 
> Besides, using the sq_bug version can serve as a reminder to "pay down
> the technical debt" in the future.
> 
>>> Again, this is pushed to the `fix-t3404-author-script-test` branch at
>>> https://github.com/dscho/git; My fixup on top of your patch looks like
>>> this (feel free to drop the sq_bug part and only keep the test part):
>>>
>>> -- snipsnap --
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 46c0b3e720f..7abe78dc78e 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -573,13 +573,14 @@ static int write_author_script(const char *message)
>>>  static int read_env_script(struct argv_array *env)
>>>  {
>>>  	struct strbuf script = STRBUF_INIT;
>>> -	int i, count = 0;
>>> +	int i, count = 0, sq_bug;
>>>  	const char *p2;
>>>  	char *p;
>>>  
>>>  	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
>>>  		return -1;
>>>  
>>> +	sq_bug = script.len && script.buf[script.len - 1] != '\'';
>>>  	for (p = script.buf; *p; p++)
>>>  		/*
>>>  		 * write_author_script() used to escape "'" incorrectly as
>>> @@ -587,8 +588,9 @@ static int read_env_script(struct argv_array *env)
>>>  		 * version the incorrect version in case git was upgraded while
>>>  		 * rebase was stopped.
>>>  		 */
>>
>> We probably want the change the comment slightly to explain sq_bug
> 
> True. Can you give it a shot? If not, I will try to remember some time mid
> next week.

Sure, I'll send something out tomorrow or Wednesday. I think we should
alter the test to check that there is no empty line at the end of the
author-script file as that will break the test for the missing "'".

> 
> Here the promised log of t3404 with -i -v -x (on macOS, but Linux and
> Windows shows equivalent failures, and the way I read it, the problem is
> simply that the test was introduced in the middle of t3404 and
> subsequent test cases' assumptions are no longer met):
> 
> -- snipsnap --
> 2018-07-27T11:53:04.2450890Z ok 2 - rebase --keep-empty
> 2018-07-27T11:53:04.2464690Z 
> 2018-07-27T11:53:04.2489730Z expecting success: 
> 2018-07-27T11:53:04.2513840Z 	test_when_finished "git rebase --abort ||:" &&
> 2018-07-27T11:53:04.2536980Z 	git checkout master &&
> 2018-07-27T11:53:04.2563050Z 	set_fake_editor &&
> 2018-07-27T11:53:04.2590300Z 	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
> 2018-07-27T11:53:04.2614900Z 	test -f .git/rebase-merge/author-script &&
> 2018-07-27T11:53:04.2639740Z 	(
> 2018-07-27T11:53:04.2665320Z 		sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
> 2018-07-27T11:53:04.2688410Z 		eval "$(cat .git/rebase-merge/author-script)" &&
> 2018-07-27T11:53:04.2710930Z 		test "$(git show --quiet --pretty=format:%an)" = "$GIT_AUTHOR_NAME" &&
> 2018-07-27T11:53:04.2734740Z 		test "$(git show --quiet --pretty=format:%ae)" = "$GIT_AUTHOR_EMAIL" &&
> 2018-07-27T11:53:04.2756700Z 		test "$(git show --quiet --date=raw --pretty=format:@%ad)" = "$GIT_AUTHOR_DATE"
> 2018-07-27T11:53:04.2778060Z 	)
> 2018-07-27T11:53:04.2788910Z 
> 2018-07-27T11:53:04.2810410Z ++ test_when_finished 'git rebase --abort ||:'
> 2018-07-27T11:53:04.2831430Z ++ test 0 = 0
> 2018-07-27T11:53:04.2852830Z ++ test_cleanup='{ git rebase --abort ||:
> 2018-07-27T11:53:04.2874430Z 		} && (exit "$eval_ret"); eval_ret=$?; :'
> 2018-07-27T11:53:04.2892720Z ++ git checkout master
> 2018-07-27T11:53:04.2911560Z Switched to branch 'master'
> 2018-07-27T11:53:04.2929490Z ++ set_fake_editor
> 2018-07-27T11:53:04.2947690Z ++ write_script fake-editor.sh
> 2018-07-27T11:53:04.2966040Z ++ echo '#!/bin/sh'
> 2018-07-27T11:53:04.2983780Z ++ cat
> 2018-07-27T11:53:04.3001950Z ++ chmod +x fake-editor.sh
> 2018-07-27T11:53:04.3019770Z +++ pwd
> 2018-07-27T11:53:04.3038530Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
> 2018-07-27T11:53:04.3057950Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
> 2018-07-27T11:53:04.3076510Z ++ export FAKE_EDITOR
> 2018-07-27T11:53:04.3094750Z ++ EDITOR='"$FAKE_EDITOR"'
> 2018-07-27T11:53:04.3112610Z ++ export EDITOR
> 2018-07-27T11:53:04.3130670Z ++ FAKE_LINES='edit 1'
> 2018-07-27T11:53:04.3153990Z ++ git rebase -i 'HEAD^'
> 2018-07-27T11:53:04.4674460Z rebase -i script before editing:
> 2018-07-27T11:53:04.4698420Z pick 8f99a4f E
> 2018-07-27T11:53:04.4710290Z 
> 2018-07-27T11:53:04.4792780Z rebase -i script after editing:
> 2018-07-27T11:53:04.4817630Z edit 8f99a4f E
> 2018-07-27T11:53:04.5025190Z Rebasing (1/1)
> 2018-07-27T11:53:04.5046400Z Stopped at 8f99a4f...  E
> 2018-07-27T11:53:04.5065680Z You can amend the commit now, with
> 2018-07-27T11:53:04.5074810Z 
> 2018-07-27T11:53:04.5092850Z   git commit --amend 
> 2018-07-27T11:53:04.5101800Z 
> 2018-07-27T11:53:04.5119900Z Once you are satisfied with your changes, run
> 2018-07-27T11:53:04.5129030Z 
> 2018-07-27T11:53:04.5147390Z   git rebase --continue
> 2018-07-27T11:53:04.5165610Z ++ test -f .git/rebase-merge/author-script
> 2018-07-27T11:53:04.5183820Z ++ sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
> 2018-07-27T11:53:04.5201610Z ++ unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE
> 2018-07-27T11:53:04.5220730Z ++ return 0
> 2018-07-27T11:53:04.5244590Z +++ cat .git/rebase-merge/author-script
> 2018-07-27T11:53:04.5269130Z ++ eval 'GIT_AUTHOR_NAME='\''A U Thor'\''
> 2018-07-27T11:53:04.5294440Z GIT_AUTHOR_EMAIL='\''author@example.com'\''
> 2018-07-27T11:53:04.5319550Z GIT_AUTHOR_DATE='\''@1112912233 -0700'\'''
> 2018-07-27T11:53:04.5345580Z +++ GIT_AUTHOR_NAME='A U Thor'
> 2018-07-27T11:53:04.5365960Z +++ GIT_AUTHOR_EMAIL=author@example.com
> 2018-07-27T11:53:04.5384150Z +++ GIT_AUTHOR_DATE='@1112912233 -0700'
> 2018-07-27T11:53:04.5402250Z +++ git show --quiet --pretty=format:%an
> 2018-07-27T11:53:04.5420400Z ++ test 'A U Thor' = 'A U Thor'
> 2018-07-27T11:53:04.5438410Z +++ git show --quiet --pretty=format:%ae
> 2018-07-27T11:53:04.5459570Z ++ test author@example.com = author@example.com
> 2018-07-27T11:53:04.5477950Z +++ git show --quiet --date=raw --pretty=format:@%ad
> 2018-07-27T11:53:04.5496180Z ++ test '@1112912233 -0700' = '@1112912233 -0700'
> 2018-07-27T11:53:04.5514390Z ++ git rebase --abort
> 2018-07-27T11:53:04.6924920Z ++ exit 0
> 2018-07-27T11:53:04.6950890Z ++ eval_ret=0
> 2018-07-27T11:53:04.6973900Z ++ :
> 2018-07-27T11:53:04.6998480Z ok 3 - rebase -i writes out .git/rebase-merge/author-script in "edit" that sh(1) can parse
> 2018-07-27T11:53:04.7009400Z 
> 2018-07-27T11:53:04.7030290Z expecting success: 
> 2018-07-27T11:53:04.7050880Z 	set_fake_editor &&
> 2018-07-27T11:53:04.7072520Z 	test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 &&
> 2018-07-27T11:53:04.7092910Z 	test_i18ncmp expect actual
> 2018-07-27T11:53:04.7102960Z 
> 2018-07-27T11:53:04.7123150Z ++ set_fake_editor
> 2018-07-27T11:53:04.7144080Z ++ write_script fake-editor.sh
> 2018-07-27T11:53:04.7164970Z ++ echo '#!/bin/sh'
> 2018-07-27T11:53:04.7185360Z ++ cat
> 2018-07-27T11:53:04.7212950Z ++ chmod +x fake-editor.sh
> 2018-07-27T11:53:04.7236720Z +++ pwd
> 2018-07-27T11:53:04.7263500Z ++ test_set_editor '/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
> 2018-07-27T11:53:04.7291390Z ++ FAKE_EDITOR='/Users/vsts/agent/2.138.3/work/1/s/t/trash directory.t3404-rebase-interactive/fake-editor.sh'
> 2018-07-27T11:53:04.7314090Z ++ export FAKE_EDITOR
> 2018-07-27T11:53:04.7338290Z ++ EDITOR='"$FAKE_EDITOR"'
> 2018-07-27T11:53:04.7362710Z ++ export EDITOR
> 2018-07-27T11:53:04.7386370Z ++ test_must_fail env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^'
> 2018-07-27T11:53:04.7412410Z ++ case "$1" in
> 2018-07-27T11:53:04.7436170Z ++ _test_ok=
> 2018-07-27T11:53:04.7455340Z ++ env 'FAKE_LINES=1 exec_true' git rebase -i 'HEAD^'
> 2018-07-27T11:53:04.9560670Z ++ exit_code=0
> 2018-07-27T11:53:04.9581700Z ++ test 0 -eq 0
> 2018-07-27T11:53:04.9603200Z ++ list_contains '' success
> 2018-07-27T11:53:04.9628730Z ++ case ",$1," in
> 2018-07-27T11:53:04.9652120Z ++ return 1
> 2018-07-27T11:53:04.9677740Z ++ echo 'test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^'
> 2018-07-27T11:53:04.9704590Z test_must_fail: command succeeded: env FAKE_LINES=1 exec_true git rebase -i HEAD^
> 2018-07-27T11:53:04.9730750Z ++ return 1
> 2018-07-27T11:53:04.9754580Z error: last command exited with $?=1
> 2018-07-27T11:53:04.9779100Z not ok 4 - rebase -i with empty HEAD
> 2018-07-27T11:53:04.9802670Z #	
> 2018-07-27T11:53:04.9826330Z #		set_fake_editor &&
> 2018-07-27T11:53:04.9851320Z #		test_must_fail env FAKE_LINES="1 exec_true" git rebase -i HEAD^ >actual 2>&1 &&
> 2018-07-27T11:53:04.9875680Z #		test_i18ncmp expect actual
> 2018-07-27T11:53:04.9899170Z #	
> 2018-07-27T11:53:04.9923310Z make[1]: *** [t3404-rebase-interactive.sh] Error 1
> 

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

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

>> The only consumer of a faulty author script written by the sequencer
>> is read_env_script() in sequencer.c which doesn't worry about
>> checking that quotes are paired. 
>
> That's not quite true anymore, recently another consumer
> read_author_ident() was added which uses sq_dequote() instead of
> custom code. Looking more closely at write_author_script() the quoting
> of single quotes is buggy they are escaped as \\' instead of \'.

That's embarrassing re-invention (instead of reuse) with additional
bug X-<.  It seems that all of that blame to d87d48b2 ("sequencer:
learn about the special "fake root commit" handling", 2018-05-04).

We should fix both broken writer and readers that compensate for
breakage in the writer, I guess.  Sigh...

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

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

>> (I think we had code to do so in "git am"
>> that was rewritten in C first).
>
> The code in builtin/am.c doesn't try to write valid posix shell (if
> one assumes it is the only consumer of the author script then it
> doesn't need to) which results in simpler code, but external scripts
> cannot safely eval it anymore.

Are you sure about that?  If so we probably should see if we can fix
the writer, and better yet, if we can share code with the writer
discussed here, as presumably we are fixing it in this thread.

But I do not see how builtin/am.c::write_author_script() would
produce something that would not eval correctly.  sq_quote_buf() was
introduced specifically to write correct string for shell's
consumption.

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

[Phillip Wood]

Hi Junio

On 18/07/18 18:17, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>>> (I think we had code to do so in "git am"
>>> that was rewritten in C first).
>>
>> The code in builtin/am.c doesn't try to write valid posix shell (if
>> one assumes it is the only consumer of the author script then it
>> doesn't need to) which results in simpler code, but external scripts
>> cannot safely eval it anymore.
> 
> Are you sure about that? If so we probably should see if we can fix> the writer, and better yet, if we can share code with the writer
> discussed here, as presumably we are fixing it in this thread.
> 
> But I do not see how builtin/am.c::write_author_script() would
> produce something that would not eval correctly.  sq_quote_buf() was
> introduced specifically to write correct string for shell's
> consumption.

You're right, I'm not sure how I missed the calls to sq_quote_buf()
yesterday, sharing the am code with the sequencer would clean things up
nicely.

Best Wishes

Phillip

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

[Johannes Schindelin]

Hi Phillip,

On Thu, 19 Jul 2018, Phillip Wood wrote:

> On 18/07/18 18:17, Junio C Hamano wrote:
> > Phillip Wood <phillip.wood@talktalk.net> writes:
> > 
> >>> (I think we had code to do so in "git am"
> >>> that was rewritten in C first).
> >>
> >> The code in builtin/am.c doesn't try to write valid posix shell (if
> >> one assumes it is the only consumer of the author script then it
> >> doesn't need to) which results in simpler code, but external scripts
> >> cannot safely eval it anymore.
> > 
> > Are you sure about that? If so we probably should see if we can fix> the writer, and better yet, if we can share code with the writer
> > discussed here, as presumably we are fixing it in this thread.
> > 
> > But I do not see how builtin/am.c::write_author_script() would
> > produce something that would not eval correctly.  sq_quote_buf() was
> > introduced specifically to write correct string for shell's
> > consumption.
> 
> You're right, I'm not sure how I missed the calls to sq_quote_buf()
> yesterday, sharing the am code with the sequencer would clean things up
> nicely.

No, actually Phillip was right. The `author-script` file written by
`git-am` was always an implementation detail, and as there was no
(intended) way to call shell scripts while running `git-am`, the only
shell script to intentionally use `author-script` was `git-am` itself.

Ever since `git-am` is a builtin, the `author-script` file format could be
changed, because it is an implementation detail, no more nor less, and I
think it *should* be changed, too. We're spending useless cycles on
quoting and dequoting, when writing a NUL-separated list of var=value
pairs would be totally sufficient to our ends.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> You're right, I'm not sure how I missed the calls to sq_quote_buf()
>> yesterday, sharing the am code with the sequencer would clean things up
>> nicely.
>
> No, actually Phillip was right. The `author-script` file written by
> `git-am` was always an implementation detail, and as there was no
> (intended) way to call shell scripts while running `git-am`, the only
> shell script to intentionally use `author-script` was `git-am` itself.

Well the thing is that you did not write "am".  When I wrote "am", I
did so with a deliberate design decision to keep the author-script
in the same format so that it can be read by shell.

You are behaving as if you made a silent decision to improve the
author-script by designing a better micro-format that deviates from
what shells read by doubling the backslash quoting and losing the
single quote at the end of the line for only the last one, and your
justification is that it does not matter how broken the new
micro-format is because it is an implementation detail nobody should
care.  And worse yet, you did that improvement without telling
anybody else why the new format is better.

That's just silly.

Just like everybody else, you are sometimes wrong and you sometimes
make mistakes.  The rest of time you are not wrong and your design
decisions are not mistaken, but trying to defend an obvious mistake
like this one with silly excuses is an easy way to lose credibility.

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

[Junio C Hamano]

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

> 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 &&

Is this "unset" safe?  Some POSIX compliant shells barf if you unset
a variable that is not set, so the answer to my question is yes only
if we know these three variables are always set.

> +	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"

Oh, actually it is even worse than that.  What if author-script is
bogus, like in the version before your patch fixes the code?  We do
not restore the AUTHOR_NAME/EMAIL/DATE after this test_expect_success
fails.  How does that, i.e. missing some variable, affect execution
of later steps in this same test script?

I _think_ the right and safe way to fix taht is to do something like
this:

	test -f .git/rebase-merge/author-script &&
	(
		safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&
		eval "$(cat .git/rebase-merge/author-script)" &&
		test ... &&
		test ... &&
		test ...
	)

That way, we won't have to worry about GIT_AUTHOR_* variables
getting modified and affecting the tests that come later in the
script.

> +'
> +
>  test_expect_success 'rebase -i with the exec command' '
>  	git checkout master &&
>  	(
> -- 
> 2.18.0

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

[Eric Sunshine]

On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano <gitster@pobox.com> wrote:
> I _think_ the right and safe way to fix taht is to do something like
> this:
>
>         test -f .git/rebase-merge/author-script &&
>         (
>                 safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&

s/safe_unset/sane_unset/

>                 eval "$(cat .git/rebase-merge/author-script)" &&
>                 test ... &&
>                 test ... &&
>                 test ...
>         )

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

[Junio C Hamano]

Yup ;-)

On Thu, Jul 12, 2018 at 1:16 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Jul 12, 2018 at 4:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>> I _think_ the right and safe way to fix taht is to do something like
>> this:
>>
>>         test -f .git/rebase-merge/author-script &&
>>         (
>>                 safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&
>
> s/safe_unset/sane_unset/
>
>>                 eval "$(cat .git/rebase-merge/author-script)" &&
>>                 test ... &&
>>                 test ... &&
>>                 test ...
>>         )

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

[Junio C Hamano]

I'll squash the following in (which I have been carrying in 'pu' for
the past few days) unless I hear otherwise soonish to correct the
issues raised during the review.

Thanks.

 t/t3404-rebase-interactive.sh | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

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
 	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"
+	(
+		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"
+	)
 '
 
 test_expect_success 'rebase -i with the exec command' '
-- 
2.18.0-129-ge3331758f1

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

[Akinori MUSHA]

That's perfectly fine with me.  I just thought each test case would
run in a separate shell process and that's why I chose not to use a
subshell for the last lines.  I've learned a lot from feedback from
you all.  Thanks!

On Wed, 18 Jul 2018 08:25:22 +0900,
Junio C Hamano wrote:
> 
> I'll squash the following in (which I have been carrying in 'pu' for
> the past few days) unless I hear otherwise soonish to correct the
> issues raised during the review.
> 
> Thanks.
> 
>  t/t3404-rebase-interactive.sh | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> 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
>  	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"
> +	(
> +		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"
> +	)
>  '
>  
>  test_expect_success 'rebase -i with the exec command' '
> -- 
> 2.18.0-129-ge3331758f1
> 

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

[Johannes Schindelin]

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"
 	)
 '
 

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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?

You are commenting on a wrong version.  Comment on the original.

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

[Johannes Schindelin]

Hi Junio,

On Thu, 26 Jul 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 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?
> 
> You are commenting on a wrong version.  Comment on the original.

Sorry, no time. Take this review, or leave it.

Ciao,
Dscho

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I _think_ the right and safe way to fix taht is to do something like
> this:
>
> 	test -f .git/rebase-merge/author-script &&
> 	(
> 		safe_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL ... &&
> 		eval "$(cat .git/rebase-merge/author-script)" &&
> 		test ... &&
> 		test ... &&
> 		test ...
> 	)
>
> That way, we won't have to worry about GIT_AUTHOR_* variables
> getting modified and affecting the tests that come later in the
> script.

It turns out that the use of subshell is *essential* for this test,
as GIT_AUTHOR_* variables are exported and must remain so.  unsetting
and reading back may allows us to ensure that shell variables have
the expected value, but then they are no longer exported, which will
mean later tests will use whatever random author ident the person or
the 'bot who is running the tests, not the one expected to be used
by the test author(s).

For tonight's pushout, I'll queue this on top.

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 12 Jul 2018 13:23:02 -0700
Subject: [PATCH] SQUASH???

---
 t/t3404-rebase-interactive.sh | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

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
 	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"
+	(
+		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"
+	)
 '
 
 test_expect_success 'rebase -i with the exec command' '
-- 
2.18.0-129-ge3331758f1

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

[Phillip Wood]

Hi Akinori

On 12/07/18 12:18, Akinori MUSHA wrote:
> 
> 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.
> 
> 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.
> 
> 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);

The third parameter here means that write_message() will append a new 
line (you can check this by looking at the file that's created) so 
strictly speaking we only need to add "'" to the end of the message, 
alternatively it would be more obvious to keep adding "'\n" and change 1 
to 0 in the call to write_message()

Best Wishes

Phillip

>   	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 &&
>   	(
> 

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

[Phillip Wood]

Hi Akinori
On 12/07/18 12:18, Akinori MUSHA wrote:
> 
> 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.
> 
> 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.
> 
> 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"
> +'

Have you checked that this test fails without your fix being applied? I 
just ran

sh -c 'eval "$(cat .git/rebase-merge/author-script)"; echo 
"$GIT_AUTHOR_NAME"'

while a rebase was stopped for an edit and it worked despite the fact 
that there is a missing quote at the end of the GIT_AUTHOR_DATE in the 
author script file.

Best Wishes

Phillip

>   test_expect_success 'rebase -i with the exec command' '
>   	git checkout master &&
>   	(
> 

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

[Phillip Wood]

On 18/07/18 14:50, Phillip Wood wrote:
> 
> Hi Akinori
> On 12/07/18 12:18, Akinori MUSHA wrote:
>>
>> 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.
>>
>> 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.
>>
>> 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"
>> +'
> 
> Have you checked that this test fails without your fix being applied? I 
> just ran
> 
> sh -c 'eval "$(cat .git/rebase-merge/author-script)"; echo 
> "$GIT_AUTHOR_NAME"'
> 
> while a rebase was stopped for an edit and it worked despite the fact 
> that there is a missing quote at the end of the GIT_AUTHOR_DATE in the 
> author script file.
> 
> Best Wishes
> 
> Phillip

Please ignore that, I messed up, there was a closing "'" as I was using 
the shell version by a mistake.

Sorry for the noise

Phillip

>>   test_expect_success 'rebase -i with the exec command' '
>>       git checkout master &&
>>       (
>>
>