Re: Crashes while trying to show tag objects with bad timestamps

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2013, 23:20 UTC
Message-ID: <7vppzsaqc5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130222230418.GC21579@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> On Fri, Feb 22, 2013 at 02:53:48PM -0800, Junio C Hamano wrote:
>
>> > I guess we should probably issue a warning, too. Also disappointingly,
>> > git-fsck does not seem to detect this breakage at all.
>> 
>> Yes for the warning, 
>
> Unfortunately, a good warning is harder than I had hoped. At the point
> where we notice the problem, pp_user_info, we have very little context.
> We can say only something like:
>
>   warning: malformed date in ident 'Jeff King <peff@peff.net> BOGUS'
>
> but we cannot say in which object, or even that it was a "tagger" line
> (and in some cases we do not even have an object, as in
> make_cover_letter).

As pp_user_info() is called from very few places, I do not think it is unreasonable to add an output parameter (i.e. "unsigned *") to let the caller know that we made a best guess given malformed input and handle the error in the caller. The make_cover_letter() caller may look like:

	pp_user_info(&pp, NULL, &sb, committer, encoding, &errors);
        if (errors & PP_CORRUPT_DATE)
		warning("unparsable datestamp in '%s'", committer);

although it is unlikely to see this error in practice, given that committer is coming from git_committer_info(0) and would have the current timestamp.

Show 7 quoted lines

> I also took a look at parsing routine of "cat-file -p". It's totally
> hand-rolled, separate from what "git show" does, and is not build on the
> pretty-print code at all. I wonder, though, if it actually makes sense
> to munge the date there. The commit-object pretty-printer for cat-file
> just shows the object intact. It seems weirdly inconsistent that we
> would munge tags just to rewrite the date. If you want a real
> pretty-printer, you should be using porcelain like "show".

The whole "cat-file -p" is a historical wart, aka poor-man's "show". I do not even consider it a part of the plumbing. It is a fair game for Porcelainisque improvement ;-)