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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 15, 2010, 21:53 UTC
Message-ID: <7vei9ipusf.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20101213170225.GA16033@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

> 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?

Show 6 quoted lines

>   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.