Re: [PATCH 2/2] sequencer: fix "rebase -i --root" corrupting author header timezone
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jul 30, 2018, 18:45 UTC
- Message-ID
- <CAPig+cQyWHSmww59qP8DTyCFpbB8HdL+cnANX8uSXVsuOviBCw@mail.gmail.com>
- In-Reply-To
- <f079bf7b-d627-20dc-cd93-dfa3496f6877@talktalk.net>
On Mon, Jul 30, 2018 at 8:20 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
Show 25 quoted lines
> 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.