Re: Performance issue of 'git branch'
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 23, 2009, 06:04 UTC
- Message-ID
- <7vy6qg2aus.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <7veis83q0e.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
Show 27 quoted lines
> 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.That is, a patch like this (this should go to 'maint'), and my worries will go away.
-- >8 -- Subject: do_one_ref(): null_sha1 check is not about broken ref
f8948e2 (remote prune: warn dangling symrefs, 2009-02-08) introduced a more dangerous variant of for_each_ref() family that skips the check for dangling refs, but it also made another unrelated check optional by mistake.
The check to see if a ref points at 0{40} is not about brokenness, but is about a possible future plan to represent a deleted ref by writing 40 "0" in a loose ref when there is a stale version of the same ref already in .git/packed-refs, so that we can implement deletion of a ref without having to rewrite the packed refs file excluding the ref being deleted. This check has to be outside of the conditional.
Signed-off-by: Junio C Hamano <gitster@pobox.com> --- refs.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c index bb0762e..3da3c8c 100644 --- a/refs.c +++ b/refs.c @@ -531,9 +531,10 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim, { if (strncmp(base, entry->name, trim)) return 0; + /* Is this a "negative ref" that represents a deleted ref? */ + if (is_null_sha1(entry->sha1)) + return 0; if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) { - if (is_null_sha1(entry->sha1)) - return 0; if (!has_sha1_file(entry->sha1)) { error("%s does not point to a valid object!", entry->name); return 0;