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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 31, 2018, 10:15 UTC
Message-ID: <CAPig+cRu6fi_SG7LeTBjAWG8aoCT7LmSipDq2a9bPR0_Ae1pFg@mail.gmail.com>
In-Reply-To: <c80cf729-1bbe-10f5-6837-b074d371b91c@talktalk.net>

On Tue, Jul 31, 2018 at 5:50 AM Phillip Wood <phillip.wood@talktalk.net> wrote:

Show 12 quoted lines

> On 31/07/18 08:33, Eric Sunshine wrote:
> > -             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.

Although the caller may not treat NULL as an _error_, it is nevertheless prepared to handle NULL. Moreover, this new code mirrors existing behavior in read_author_ident() in which it already returns NULL for other errors encountered during the parse, so I think returning NULL is the correct thing to do within the scope of this patch series, especially considering that this series is about fixing genuine commit object corruption from which a typical will be unable to recover (and may not even notice until some time in the future).

While the unexpected change in value you describe is unfortunate, this patch neither helps nor hurts that since the problem already exists in that other parts of this function already return NULL, and since the junk value used by read_author_ident() before this patch was already an "unexpected change". More importantly, the commit object itself is not corrupted by this issue, and this sort of "error" is something from which a typical user is likely to be able to recover, so IMHO is outside the scope of the critical bug fixes of this series. Fixing that issue can easily be done on top of this series.

Thanks for thinking critically about this.