On Tue, Jul 31, 2018 at 5:50 AM Phillip Wood <phillip.wood@xxxxxxxxxxxx> wrote:
> 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.
