Re: [PATCH] parse_object: try internal cache before reading object db

From: Jeff King <peff@peff.net>
Date: Jan 5, 2012, 22:18 UTC
Message-ID: <20120105221849.GA5459@sigill.intra.peff.net>
In-Reply-To: <7vehvdn7at.fsf@alter.siamese.dyndns.org>

On Thu, Jan 05, 2012 at 01:55:22PM -0800, Junio C Hamano wrote:

Show 15 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > The worst potential problem I could come up with is if you somehow had
> > an object whose "parsed" flag was set, but somehow didn't have its other
> > fields set (like type).
> > ...
> > So I think it is safe short of somebody doing some horrible manual
> > munging of a "struct object".
> 
> Yeah, I was worried about codepaths like commit-pretty-printing might be
> mucking with the contents of commit->buffer, perhaps reencoding the text
> and then calling parse_object() to get the unmodified original back, or
> something silly like that. But the lookup_object() call at the beginning
> of the parse_object() already prevents us from doing such a thing, so we
> should be OK, I would think.

Er, without my patch there is no such lookup_object, is there?

What saves you is that the parse_*_buffer functions all do nothing when the object.parsed flag is set, and the code I added makes sure that object.parsed is set in the object that lookup_object returns.

So yeah, anytime you tweak the contents of commit->buffer but don't unset the "parsed" flag, you are asking for trouble.

Here's another possible code path where the behavior is changed:

  1. You set the global save_commit_buffer to 0.

  2. You call parse_commit(commit) on an unparsed commit object, which
     does not save the commit buffer, but does set
     commit->object.parsed.

  3. You call parse_object(commit->object.sha1).

     a. Without my patch, we read the file contents again, do _not_
        re-parse them (because we look up the existing object and notice
        that its "parsed" flag is set), but we _do_ assign the buffer to
        commit->buffer.

     b. With my patch, we see that there is an existing object that is
        already parsed, and return early. commit->buffer remains NULL.

I would argue that this doesn't matter, since "parse_commit" uses the exact same optimization (it returns early without setting commit->buffer if the parsed flag is set). So any program turning off save_commit_buffer has to be ready to deal with a NULL commit->buffer in the first place. The only exception would be a program that then tries to fill in the commit->buffer field by manually running parse_object on an already-parsed, buffer-less commit object. I don't think we do that.

You can verify that commit->buffer is the only place where these issues can happen by following the logic in parse_object_buffer.

Sorry to belabor the discussion, but this is such a core piece of code, I want to make sure the optimization isn't hurting anybody (I don't think it is, and certainly the tests are all happy, but I think talking through the cases is a good thing).

-Peff