[PATCH v4 2/2] sequencer: fix quoting in write_author_script

[Phillip Wood <phillip.wood@talktalk.net>]

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

Single quotes should be escaped as \' not \\'. The bad quoting breaks
the interactive version of 'rebase --root' (which is used when there
is no '--onto' even if the user does not specify --interactive) for
authors that contain "'" as sq_dequote() called by read_author_ident()
errors out on the bad quoting.

For other interactive rebases this only affects external scripts that
read the author script and users whose git is upgraded from the shell
version of rebase -i while rebase was stopped when the author contains
"'". This is because the parsing in read_env_script() expected the
broken quoting.

This patch includes code to handle the broken quoting when
git has been upgraded while rebase was stopped. It does this by
detecting the missing "'" at the end of the GIT_AUTHOR_DATE line to see
if it should dequote \\' as "'". Note this is only implemented for
normal picks, not for creating a new root commit (rebase will stop with
an error complaining out bad quoting in that case).

The fallback code has been manually tested by reverting both the quoting
fixes in write_author_script() and the previous fix for the missing "'"
at the end of the GIT_AUTHOR_DATE line and running
t3404-rebase-interactive.sh.

Ideally rebase and am would share the same code for reading and
writing the author script, but this commit just fixes the immediate
bug.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Notes:
    changes since v3:
    
     - Reverted the implementation to v2 with more robust detection of the
       missing "'" on the last line of the author script based on a
       suggestion by Eric. This means that this series needs to progress
       closely with Eric's series of fixes or the fallback code will never
       be called.
    
    - Tweaked the last test

 sequencer.c                   | 36 ++++++++++++++++++++++++++++-------
 t/t3404-rebase-interactive.sh | 18 +++++++++++++++---
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c4e4418559..ba11fe5bca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -636,42 +636,64 @@ 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++));
 	strbuf_addch(&buf, '\'');
 	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
 	strbuf_release(&buf);
 	return res;
 }
 
+
+/*
+ * write_author_script() used to fail to terminate the last line with a "'" and
+ * also escaped "'" incorrectly as "'\\\\''" rather than "'\\''". We check for
+ * the terminating "'" on the last line to see how "'" has been escaped in case
+ * git was upgraded while rebase was stopped.
+ */
+static int quoting_is_broken(const char *s, size_t n)
+{
+	/* Skip any empty lines in case the file was hand edited */
+	while (n > 0 && s[--n] == '\n')
+		; /* empty */
+	if (n > 0 && s[n] != '\'')
+		return 1;
+
+	return 0;
+}
+
 /*
  * Read a list of environment variable assignments (such as the author-script
  * file) into an environment block. Returns -1 on error, 0 otherwise.
  */
 static int read_env_script(struct argv_array *env)
 {
 	struct strbuf script = STRBUF_INIT;
-	int i, count = 0;
-	char *p, *p2;
+	int i, count = 0, sq_bug;
+	const char *p2;
+	char *p;
 
 	if (strbuf_read_file(&script, rebase_path_author_script(), 256) <= 0)
 		return -1;
-
+	/* write_author_script() used to quote incorrectly */
+	sq_bug = quoting_is_broken(script.buf, script.len);
 	for (p = script.buf; *p; p++)
-		if (skip_prefix(p, "'\\\\''", (const char **)&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 b72167ecd5..834a124d60 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1382,9 +1382,21 @@ test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.gpgSign' '
 test_expect_success 'valid author header after --root swap' '
 	rebase_setup_and_clean author-header no-conflict-branch &&
 	set_fake_editor &&
-	FAKE_LINES="2 1" git rebase -i --root &&
-	git cat-file commit HEAD^ >out &&
-	grep "^author ..*> [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
+	git commit --amend --author="Au ${SQ}thor <author@example.com>" --no-edit &&
+	git cat-file commit HEAD | grep ^author >expected &&
+	FAKE_LINES="5 1" git rebase -i --root &&
+	git cat-file commit HEAD^ | grep ^author >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'valid author header when author contains single quote' '
+	rebase_setup_and_clean author-header no-conflict-branch &&
+	set_fake_editor &&
+	git commit --amend --author="Au ${SQ}thor <author@example.com>" --no-edit &&
+	git cat-file commit HEAD | grep ^author >expected &&
+	FAKE_LINES="2" git rebase -i HEAD~2 &&
+	git cat-file commit HEAD | grep ^author >actual &&
+	test_cmp expected actual
 '
 
 test_done
-- 
2.18.0