Junio C Hamano <gitster@xxxxxxxxx> writes:
> 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@xxxxxxxxx>
---
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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html