[PATCH v2 17/33] repack_without_ref(): silence errors for dangling packed refs
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Apr 22, 2013, 19:52 UTC
- Message-ID
- <1366660361-21831-18-git-send-email-mhagger@alum.mit.edu>
- In-Reply-To
- <1366660361-21831-1-git-send-email-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