Re: [PATCH v2 4/5] get_sha1: speed up ambiguous 40-hex test

From: Jeff King <peff@peff.net>
Date: Jan 14, 2014, 09:50 UTC
Message-ID: <20140114095002.GA32258@sigill.intra.peff.net>
In-Reply-To: <20140110094120.GB17443@sigill.intra.peff.net>

On Fri, Jan 10, 2014 at 04:41:20AM -0500, Jeff King wrote:

Show 5 quoted lines

> That being said, we could further optimize this by not opening the files
> at all (and make that the responsibility of do_one_ref, which we are
> avoiding here). I am slightly worried about the open() cost of my
> solution. It's amortized away in a big call, but it is probably
> noticeable for something like `git rev-parse <40-hex>`.

I took a look at this. It gets a bit hairy. My strategy is to add a flag to ask read_loose_refs to create REF_INCOMPLETE values. We currently use this flag for loose REF_DIRs to mean "we haven't opendir()'d the subdirectory yet". This would extend it to the non-REF_DIR case to mean "we haven't opened the loose ref file yet". We'd check REF_INCOMPLETE before handing the ref_entry to a callback, and complete it if necessary.

It gets ugly, though, because we need to pass that flag through quite a bit of callstack. get_ref_dir() needs to know it, which means all of find_containing_dir, etc need it, meaning it pollutes all of the packed-refs code paths too.

I have a half-done patch in this direction if that doesn't sound too nasty.

> > This doesn't correctly handle the rule
> > 
> > 	"refs/remotes/%.*s/HEAD"
> [...]

> I'll see how painful it is to make it work.

It's actually reasonably painful. I thought at first we could get away with more cleverly parsing the rule, find the prefix (up to the placeholder), and then look for the suffix ("/HEAD") inside there. But it can never work with the current do_for_each_* code. That code only triggers a callback when we see a concrete ref. It _never_ lets the callbacks see an intermediate directory.

So a NO_RECURSE flag is not sufficient to handle this case. I'd need to teach do_for_each_ref to recurse based on pathspecs, or a custom callback function. And that is getting quite complicated.

I think it might be simpler to just do my own custom traversal. What I need is much simpler than what do_for_each_entry provides. I don't need recursion, and I don't actually need to look at the loose and packed refs together. It's OK for me to do them one at a time because I don't care about the actual value; I just want to know about which refs exist.

-Peff