Re: Performance issue of 'git branch'

Linus Torvalds [off-list ref] writes:

On Wed, 22 Jul 2009, Junio C Hamano wrote:

quoted

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.