From: René Scharfe Date: Wed, 17 Apr 2013 17:59:28 GMT Subject: Re: git log - crash and core dump Message-ID: <516EE300.7020200@lsrfire.ath.cx> In-Reply-To: <20130417063942.GA27703@sigill.intra.peff.net> Am 17.04.2013 08:39, schrieb Jeff King: > On Tue, Apr 16, 2013 at 10:26:40PM -0700, Junio C Hamano wrote: > >> Junio C Hamano writes: >> >>> René Scharfe writes: >>> >>>> How about making split_ident_line() a bit friendlier be letting it >>>> provide the epoch as default time stamp instead of NULL? >>> >>> Two knee-jerk concerns I have without going back to the callers: >>> >>> * Would that "0" ever be given to the approxidate parser, which >>> rejects ancient dates in numbers-since-epoch format without @ >>> prefix? >>> >>> * Does any existing caller use the NULL as a sign to see the input >>> was without date and act on that information? >> >> I looked at all the callers (there aren't that many), and none of >> them did "Do this on a person-only ident, and do that on an ident >> with timestamp". So for the callers that ignore timestamp, your >> patch will be a no-op, and for others that assume there is a >> timestamp, it will turn a crash/segv into output with funny >> timestamp. > > What about sane_ident_split in builtin/commit.c? It explicitly rejects a > NULL date. The logic in determine_author_info is a little hard to follow > (it assembles the ident line with fmt_ident and then reparses it), but I > believe it should be catching a bogus line from "commit -c", or from > GIT_AUTHOR_DATE in the environment. Right, so let's keep the NULLs and fix the individual cases. A quick "git grep -W -e date_begin -e date_end -e tz_begin -e tz_end" reveals that there are only the ones we talked about: blame, pretty, commit and -- of course -- ident. And only the first two need fixing. > As a side note, when determine_author_info sees a bogus ident, it > appears to just silently ignore it, which is probably a bad thing. > Shouldn't we by complaining? Or am I mis-reading the code? The code looks complicated, but I just tried it: fmt_ident() dies if you give it an invalid date. René