Re: [RFC PATCH] git log: support "auto" decorations

From: Jeff King <peff@peff.net>
Date: May 30, 2014, 17:03 UTC
Message-ID: <20140530170330.GA25443@sigill.intra.peff.net>
In-Reply-To: <xmqqvbsn9pfx.fsf@gitster.dls.corp.google.com>

On Fri, May 30, 2014 at 09:55:14AM -0700, Junio C Hamano wrote:

Show 24 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, May 29, 2014 at 09:54:10PM -0700, Linus Torvalds wrote:
> >
> >> That said, part of it is just that show-signature is so suboptimal
> >> performance-wise, re-parsing the commit buffer for each commit when
> >> "show_signature" is set. That's just crazy, we've already parsed the
> >> commit text, we already *could* know if it has a signature or not, and
> >> skip it if it doesn't. That would require one of the flag bits in the
> >> object, though, or something, so it's probably not worth doing.
> >
> > Wow, it's really quite bad. Not only do we spend time on commits that we
> > could otherwise know do not have signatures, but we actually pull the
> > buffer from disk, even though we generally have it saved as
> > commit->buffer.
> 
> The one for the signature on the commit itself is me being lazy and
> defensive; I did not want to have to worry about people mucking with
> what is in commit->buffer for whatever reason (e.g. re-encode in
> different charset, etc.) and then asking the signature validated.
> 
> The other one for the merge-tag is me just being lazy, as it is
> unlikely to be corrupt by any reasonable kinds of mucking with
> commit->buffer on a merge.

I don't think we need to worry about commit->buffer being mucked with. It is always either NULL, or points to the original object contents. Encoded log messages are always placed in a separate buffer (and in fact we use the same "optionally point to commit->buffer" trick there). And things like mucking with parents always happen on the parsed form.

Of course I may be missing a site, and it's certainly a maintenance risk for the future. But I'd go so far as to say that anything modifying commit->buffer is wrong, and that side should be fixed.

Do you want me to roll it up with a real commit message?

The other option is to do something like Linus suggested, and note the presence/absence of signature and mergetag headers with a few bits (we could even use a commit slab if we don't want to waste bits in the object struct). That would prevent us hitting this code at all for most commits, so we would save not only the read_sha1_file cost, but the extra parsing cost.

However, that does nothing to help the cases where we _do_ have signatures. A repo where somebody GPG-signed every commit, for example, would still perform terribly. So even if we go that route, I think we'd want to apply this technique, too.

-Peff