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

[Jeff King <peff@peff.net>]

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

> 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