repack_without_ref(): silence errors for dangling packed refs
authorMichael Haggerty <mhagger@alum.mit.edu>
Mon, 22 Apr 2013 19:52:25 +0000 (21:52 +0200)
committerJunio C Hamano <gitster@pobox.com>
Wed, 1 May 2013 22:33:10 +0000 (15:33 -0700)
Stop emitting an error message when deleting a packed reference if we
find another dangling packed reference that is overridden by a loose
reference.  See the previous commit for a longer explanation of the
issue.

We have to be careful to make sure that the invalid packed reference
really *is* overridden by a loose reference; otherwise what we have
found is repository corruption, which we *should* report.

Please note that this approach is vulnerable to a race condition
similar to the race conditions already known to affect packed
references [1]:

* Process 1 tries to peel packed reference X as part of deleting
  another packed reference.  It discovers that X does not refer to a
  valid object (because the object that it referred to has been
  garbage collected).

* Process 2 tries to delete reference X.  It starts by deleting the
  loose reference X.

* Process 1 checks whether there is a loose reference X.  There is not
  (it has just been deleted by process 2), so process 1 reports a
  spurious error "X does not point to a valid object!"

The worst case seems relatively harmless, and the fix is identical to
the fix that will be needed for the other race conditions (namely
holding a lock on the packed-refs file during *all* reference
deletions), so we leave the cleaning up of all of them as a future
project.

[1] http://thread.gmane.org/gmane.comp.version-control.git/211956

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs.c
t/t3210-pack-refs.sh

diff --git a/refs.c b/refs.c
index d884f7d..002daeb 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -1902,8 +1902,41 @@ static int repack_without_ref_fn(struct ref_entry *entry, void *cb_data)
 
        if (!strcmp(data->refname, entry->name))
                return 0;
-       if (!ref_resolves_to_object(entry))
-               return 0; /* Skip broken refs */
+       if (entry->flag & REF_ISBROKEN) {
+               /* This shouldn't happen to packed refs. */
+               error("%s is broken!", entry->name);
+               return 0;
+       }
+       if (!has_sha1_file(entry->u.value.sha1)) {
+               unsigned char sha1[20];
+               int flags;
+
+               if (read_ref_full(entry->name, sha1, 0, &flags))
+                       /* We should at least have found the packed ref. */
+                       die("Internal error");
+               if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED))
+                       /*
+                        * This packed reference is overridden by a
+                        * loose reference, so it is OK that its value
+                        * is no longer valid; for example, it might
+                        * refer to an object that has been garbage
+                        * collected.  For this purpose we don't even
+                        * care whether the loose reference itself is
+                        * invalid, broken, symbolic, etc.  Silently
+                        * omit the packed reference from the output.
+                        */
+                       return 0;
+               /*
+                * There is no overriding loose reference, so the fact
+                * that this reference doesn't refer to a valid object
+                * indicates some kind of repository corruption.
+                * Report the problem, then omit the reference from
+                * the output.
+                */
+               error("%s does not point to a valid object!", entry->name);
+               return 0;
+       }
+
        len = snprintf(line, sizeof(line), "%s %s\n",
                       sha1_to_hex(entry->u.value.sha1), entry->name);
        /* this should not happen but just being defensive */
index 675f06a..1a2080e 100755 (executable)
@@ -140,7 +140,7 @@ test_expect_success 'delete ref with dangling packed version' '
        test_cmp /dev/null result
 '
 
-test_expect_failure 'delete ref while another dangling packed ref' '
+test_expect_success 'delete ref while another dangling packed ref' '
        git branch lamb &&
        git commit --allow-empty -m "future garbage" &&
        git pack-refs --all &&