git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Phillip Wood <phillip.wood@talktalk.net>
To: Junio C Hamano <gitster@pobox.com>, Akinori MUSHA <knu@iDaemons.org>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] sequencer.c: terminate the last line of author-script properly
Date: Wed, 18 Jul 2018 10:45:26 +0100	[thread overview]
Message-ID: <eb66f8fb-1535-37a8-526d-ed31f06c045c@talktalk.net> (raw)
In-Reply-To: <xmqqwou0xtar.fsf@gitster-ct.c.googlers.com>

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


  reply	other threads:[~2018-07-18  9:45 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-12 11:18 [PATCH] sequencer.c: terminate the last line of author-script properly Akinori MUSHA
2018-07-12 17:22 ` Junio C Hamano
2018-07-18  9:45   ` Phillip Wood [this message]
2018-07-18 13:46     ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood
2018-07-18 15:55       ` [RFC PATCH] sequencer: fix quoting in write_author_script Phillip Wood
2018-07-24 15:31         ` Junio C Hamano
2018-07-26 12:33         ` Johannes Schindelin
2018-07-27 10:36           ` Phillip Wood
2018-07-27 12:37             ` Johannes Schindelin
2018-07-30  9:35               ` Phillip Wood
2018-07-18 17:24       ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Junio C Hamano
2018-07-18 17:17     ` [PATCH] sequencer.c: terminate the last line of author-script properly Junio C Hamano
2018-07-19  9:20       ` Phillip Wood
2018-07-26 12:39         ` Johannes Schindelin
2018-07-26 17:53           ` Junio C Hamano
2018-07-12 20:13 ` Junio C Hamano
2018-07-12 20:16   ` Eric Sunshine
2018-07-12 20:23     ` Junio C Hamano
2018-07-17 23:25     ` Junio C Hamano
2018-07-18  6:23       ` Akinori MUSHA
2018-07-26 12:07       ` Johannes Schindelin
2018-07-26 17:44         ` Junio C Hamano
2018-07-27 15:49           ` Johannes Schindelin
2018-07-12 20:49   ` Junio C Hamano
2018-07-18  9:25 ` Phillip Wood
2018-07-18 13:50 ` Phillip Wood
2018-07-18 13:58   ` [PATCH] sequencer.c: terminate the last line of author-scriptproperly Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=eb66f8fb-1535-37a8-526d-ed31f06c045c@talktalk.net \
    --to=phillip.wood@talktalk.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=knu@iDaemons.org \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).