On 31/07/18 08:33, Eric Sunshine wrote:
When "git rebase -i --root" creates a new root commit, it corrupts the
"author" header's timezone by repeating the last digit:
author A U Thor <author@xxxxxxxxxxx> @1112912773 -07000
This is due to two bugs.
First, write_author_script() neglects to add the closing quote to the
value of GIT_AUTHOR_DATE when generating "rebase-merge/author-script".
Second, although sq_dequote() correctly diagnoses the missing closing
quote, read_author_ident() ignores sq_dequote()'s return value and
blindly uses the result of the aborted dequote.
sq_dequote() performs dequoting in-place by removing quoting and
shifting content downward. When it detects misquoting (lack of closing
quote, in this case), it gives up and returns an error without inserting
a NUL-terminator at the end of the shifted content, which explains the
duplicated last digit in the timezone.
(Note that the "@" preceding the timestamp is a separate bug which
will be fixed subsequently.)
Signed-off-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
---
sequencer.c | 7 ++++++-
t/t3404-rebase-interactive.sh | 2 +-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 78864d9072..1008f6d71a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -654,6 +654,7 @@ static int write_author_script(const char *message)
strbuf_addch(&buf, *(message++));
else
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;
@@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf *buf)
eol = strchrnul(in, '\n');
*eol = '\0';
- sq_dequote(in);
+ if (!sq_dequote(in)) {
+ warning(_("bad quoting on %s value in '%s'"),
+ keys[i], rebase_path_author_script());
+ return NULL;
Unfortunately the caller does not treat NULL as an error, so this will
change the date and potentially the author of the commit. While that
isn't corruption in the sense that it creates a sane commit, it does
corrupt the author data compared to its expected value. I think it would
be better to die in the short term, or fix the caller.
+ }
len = strlen(in);
if (i > 0) /* separate values by spaces */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d6e9b52740..fd3a18154e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1420,7 +1420,7 @@ test_expect_success 'valid author header after --root swap' '
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]*$" out
+ grep "^author ..*> @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
'
test_done
