Re: [PATCH 2/2] sequencer: fix "rebase -i --root" corrupting author header timezone

On Mon, Jul 30, 2018 at 8:20 AM Phillip Wood [off-list ref] wrote:

On 30/07/18 10:29, Eric Sunshine wrote:

quoted

When "git rebase -i --root" creates a new root commit, it corrupts the
"author" header's timezone by repeating the last digit:
[...]
Signed-off-by: Eric Sunshine <redacted>
---

diff --git a/sequencer.c b/sequencer.c

@@ -654,6 +654,7 @@ static int write_author_script(const char *message)
+     strbuf_addch(&buf, '\'');

@@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf *buf)
-             sq_dequote(in);
+             if (!sq_dequote(in)) {
+                     warning(_("bad quoting on %s value in '%s'"),
+                             keys[i], rebase_path_author_script());
+                     return NULL;

I think we want to handle the broken author script properly rather than
returning NULL. If we had a single function
int read_author_script(const char **name, const char **author, const
char **date)
to read the author script that tried sq_dequote() and then fell back to
code based on read_env_script() that handled the missing "'" at the end
and also the bad quoting of "'" if sq_dequote() failed it would make it
easier to fix the existing bugs, rather than having to fix
read_author_ident() and read_env_script() separately. What do you think?

That makes sense as a long-term plan, however, I'm concerned with the
immediate problem that a released version of Git can (and did, in my
case) corrupt commit objects. So, in the short term, I think it makes
sense to get this minimal fix landed, and build the more "correctly
engineered" solution on top of it, without the pressure of worrying
about corruption spreading.