Re: Performance issue of 'git branch'

[Linus Torvalds <torvalds@linux-foundation.org>]

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.  Is the cold cache performance of
> "git branch" to list your local branches that important?

Hmm. I do think that 7.5s is _way_ too long to wait for something as 
simple as "what branches do I have?".

And yes, it's also an operation that I'd expect to be quite possibly the 
first one you do when moving to a new repo, so cold-cache is realistic.

And the 'rawref' thing is exactly the same as the 'ref' version, except it 
doesn't do the null_sha1 check and the 'has_sha1-file()' check.

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.

And quite frankly, while the null-sha1 check may make sense, the way the 
flag is named right now (DO_FOR_EACH_INCLUDE_BROKEN), I think we might be 
better off re-thinking things later if we ever end up caring. That 
'is_null_sha1()' check should possibly be under a separate flag.

That said, while I think my patch was the simplest and most 
the problem could certainly have been fixed differently.

For example, instead of using 'for_each_ref()' and then splitting them by 
kind with that "detect kind" loop, it could instead have done two loops, 
ie

	if (kinds & REF_LOCAL_BRANCH)
		for_each_ref_in("refs/heads/", append_local, &ref_list);
	if (kinds & REF_REMOTE_BRANCH)
		for_each_ref_in("refs/remotes/", append_remote, &ref_list);

and avoided the other refs we aren't interested in _that_ way instead.

But it would be a bigger and involved patch. It gets really messy too (I 
tried), because when you use 'for_each_ref_in()' it removes the prefix as 
it goes along, but then the code in builtin-branch.c wants the prefix 
after all.

		Linus