From: Phillip Wood <phillip.wood@talktalk.net>
Date: Wed, 18 Jul 2018 13:46:17 GMT
Subject: Re: [PATCH] sequencer.c: terminate the last line of author-scriptproperly
Message-ID: <eb295aea-dae5-5e1c-bacf-2c77d3ce0195@talktalk.net>
In-Reply-To: <eb66f8fb-1535-37a8-526d-ed31f06c045c@talktalk.net>

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
>