Re: [PATCH v2 4/4] sequencer: don't die() on bogus user-edited timestamp

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jul 31, 2018 at 6:02 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
> On 31/07/18 08:33, Eric Sunshine wrote:
> > +     /* validate date since fmt_ident() will die() on bad value */
> > +     if (parse_date(val[2], &out)){
> > +             warning(_("invalid date format '%s' in '%s'"),
> > +                     val[2], rebase_path_author_script());
> > +             strbuf_release(&out);
> > +             return NULL;
> > +     }
>
> I think if you're going to do this then the caller needs to be changed
> to treat NULL as an error

I humbly disagree for the reasons outlined in my response[1] to your
review of 2/4. Summarized, this function already returns NULL in
several cases upon error, so the change made by this patch is
consistent with existing behavior (and certainly doesn't make the
situation worse). This patch series is about fixing the critical bugs
resulting in genuine commit object corruption; it's not about
re-engineering a case which was perhaps not thought through thoroughly
(changing commit data unexpectedly) but which does not genuinely
corrupt the commit object itself.

[1]: https://public-inbox.org/git/CAPig+cRu6fi_SG7LeTBjAWG8aoCT7LmSipDq2a9bPR0_Ae1pFg@mail.gmail.com/