Re: Performance issue of 'git branch'

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2009, 05:52 UTC
Message-ID: <7veis83q0e.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.2.01.0907222204310.21520@localhost.localdomain>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 10 quoted lines

> On Wed, 22 Jul 2009, Junio C Hamano wrote:
>> 
>> Hmm, we now have to remember what this patch did, if we ever wanted to
>> introduce negative refs later (see ef06b91 do_for_each_ref: perform the
>> same sanity check for leftovers., 2006-11-18).  Not exactly nice to spread
>> the codepaths that need to be updated.
> ...
> And since git branch will do something _better_ than the 'has_sha1_file()' 
> check (by virtue of actually looking up the commit), I don't think that 
> part is an issue. So the only issue is the is_null_sha1() thing.

Exactly.

That is_null_sha1() thing was a remnant of your idea to represent deleted ref that has a packed counterpart by storing 0{40} in a loose ref, so that we can implement deletion efficiently.

Since we currently implement deletion by repacking packed refs if the ref has a packed (possibly stale) one, we do not use such a "negative ref", and skipping 0{40} done by the normal (i.e. non-raw) for_each_ref() family is not necessary.

I was inclined to say that, because I never saw anybody complained that deleting refs was too slow, we declare that we would forever stick to the current implementation of ref deletion, and remove the is_null_sha1() check from the do_one_ref() function, even for include-broken case.

But after thinking about it again, I'd say "if null, then skip" should be outside the DO_FOR_EACH_INCLUDE_BROKEN anyway, because the null check is not about brokenness of the ref, but is about a possible future expansion to represent deleted ref with such a "negative ref" entry.

If we remove is_null_sha1() from do_one_ref(), or if we move it out of the "include broken" thing, my "Not exactly nice" comment can be rescinded, as doing the former (i.e. removal of is_null_sha1() check) is a promise that we will never have to worry about negative refs, and doing the latter will still protect callers of do_for_each_rawref() from negative refs if we ever introduce them in some future.