[PATCH v2 17/33] repack_without_ref(): silence errors for dangling packed refs

[Michael Haggerty <mhagger@alum.mit.edu>]

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>
---
 refs.c               | 37 +++++++++++++++++++++++++++++++++++--
 t/t3210-pack-refs.sh |  2 +-
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index ed54ed4..2957f6d 100644
--- a/refs.c
+++ b/refs.c
@@ -1901,8 +1901,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 */
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index c032d88..559f602 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -142,7 +142,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 &&
-- 
1.8.2.1