From: Junio C Hamano Date: Mon, 10 Jun 2013 07:39:24 GMT Subject: Re: [PATCH v2 4/4] log: --author-date-order Message-ID: <7vobbel8ib.fsf@alter.siamese.dyndns.org> In-Reply-To: <20130610055014.GF3621@sigill.intra.peff.net> Jeff King writes: > I'm not excited about introducing yet another place that parses commit > objects (mostly not for correctness, but because we have had > inconsistency in how malformed objects are treated). It is at least > using split_ident_line which covers the hard bits. I wonder how much > slower it would be to simply call format_commit_message to do the > parsing. The thought certainly crossed my mind, not exactly in that form but more about splitting the machinery used in pretty.c into a more reusable form. The result of my attempt however did not become all that reusable (admittedly I didn't spend too much brain cycles on it), so I punted ;-). > The record_author_date function assumes that commit->buffer is valid > (i.e., not NULL). We seem to assume that the commits are parsed already > (for looking at parents, and at the committer date). I thought that the latter is warranted, as the function worked on the output of limit_list(), and by the time limit_list() finishes, everything relevant must have been parsed already. But you are right. The commit->buffer may no longer be there, and the --author-date-order option needs to read the object again in this codepath. That would be in line with what --pretty/format would do, I guess. Or we could extend parse_commit() API to take an optional commit info slab to store not just author date but other non-essential stuff like people's names, and we arrange that extended API to be triggered when we know --author-date-order is in effect?