On 18/07/18 10:45, Phillip Wood wrote:
Hi Junio
On 12/07/18 18:22, Junio C Hamano wrote:
"Akinori MUSHA" <knu@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
---
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
