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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jul 30, 2018 at 8:20 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> On 30/07/18 10:29, 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:
> > [...]
> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> > ---
> > 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.