Re: [PATCH/RFC] ident: die on bogus date format

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> There are two down-sides to this approach:
>
>   1. Technically this breaks somebody doing something like
>      "git commit --date=now", which happened to work because
>      bogus data is the same as "now". Though we do
>      explicitly handle the empty string, so anybody passing
>      an empty variable through the environment will still
>      work.

These days I think the bogodate parser knows what "now" is, but you can
change the example to use "ahora" instead of "now" and your argument does
not change.  But if you force user to change something in order to work
with a new version of git, it is a regression, no matter how small that
change is.

Having said that, I don't think --date=ahora is something we need to worry
about within the context of "git commit", as the regression feels purely
technical (the author-date defaults to the current time anyway, so there
is no reason to give --date=ahora to the command, even though giving an
explicit date via the flag may have some uses).  On the other hand, as
fmt_ident() is fairly low-level, there might be other callers to which it
made sense to give "now" to them, and we wouldn't know without looking.

>      If the error is too much, perhaps it can be downgraded
>      to a warning?

I think dying is actually Ok for this caller, as we already pass
IDENT_ERROR_ON_NO_NAME to fmt_ident() expecting it to die for us upon a
bad input.  Even though I suspect that we do not need to be conditional on
this (the only reason ON_NO_NAME exists is because reflogs may record your
name when you switch branches, and if you are only sightseeing it doesn't
matter if your name is "johndoe@(null)"), using IDENT_ERROR_ON_NO_DATE may
be safer perhaps?

>   2. The error checking happens _after_ the commit message
>      is written, which can be annoying to the user. We can
>      put explicit checks closer to the beginning of
>      git-commit, but that feels a little hack-ish; suddenly
>      git-commit has to care about how fmt_ident works. Maybe
>      we could simply call fmt_ident earlier?

After determine_author_info() returns to prepare_to_commit(), we have a
call to git_committer_info() only to discard the outcome from.  I think
this call was an earlier attempt to catch "You do not exist" and related
low-level errors, and the codepath feels the right place to catch more
recent errors like the one under discussion.  Instead of passing 0, how
about passing IDENT_ERROR_ON_NO_NAME and IDENT_ERROR_ON_NO_DATE there,
store and return its output from the prepare_to_commit(), and then give
that string to commit_tree() later in cmd_commit().  We can do this by
adding a new parameter (strbuf) to prepare_to_commit(), I think.