Commit 8c14224103a7f2d254635a340a2a2f8b720b2b21
2019-06-14T08:20:05
refdb: make sure to remove packed refs first This fixes part of the issue where, given a concurrent `git pack-refs`, a ref lookup could return an old, vestigial value from the packed file, as the valid loose one would have been deleted.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63
diff --git a/src/refdb_fs.c b/src/refdb_fs.c
index 1849877..9fb66a8 100644
--- a/src/refdb_fs.c
+++ b/src/refdb_fs.c
@@ -1439,7 +1439,7 @@ static int refdb_fs_backend__delete_tail(
{
refdb_fs_backend *backend = GIT_CONTAINER_OF(_backend, refdb_fs_backend, parent);
int error = 0, cmp = 0;
- bool loose_deleted = 0;
+ bool packed_deleted = 0;
error = cmp_old_ref(&cmp, _backend, ref_name, old_id, old_target);
if (error < 0)
@@ -1451,26 +1451,40 @@ static int refdb_fs_backend__delete_tail(
goto cleanup;
}
- /* If a loose reference exists, remove it from the filesystem */
- if ((error = loose_delete(backend, ref_name)) < 0 && error != GIT_ENOTFOUND)
+ /*
+ * To ensure that an external observer will see either the current ref value
+ * (because the loose ref still exists), or a missing ref (after the packed-file is
+ * unlocked, there will be nothing left), we must ensure things happen in the
+ * following order:
+ *
+ * - the packed-ref file is locked and loaded, as well as a loose one, if it exists
+ * - we optimistically delete a packed ref, keeping track of whether it existed
+ * - we delete the loose ref, note that we have its .lock
+ * - the loose ref is "unlocked", then the packed-ref file is rewritten and unlocked
+ * - we should prune the path components if a loose ref was deleted
+ *
+ * Note that, because our packed backend doesn't expose its filesystem lock,
+ * we might not be able to guarantee that this is what actually happens (ie.
+ * as our current code never write packed-refs.lock, nothing stops observers
+ * from grabbing a "stale" value from there).
+ */
+ if ((error = packed_delete(backend, ref_name)) < 0 && error != GIT_ENOTFOUND)
goto cleanup;
- if (error == GIT_ENOTFOUND)
- error = 0;
- else if (error == 0)
- loose_deleted = 1;
+ if (error == 0)
+ packed_deleted = 1;
- if ((error = packed_delete(backend, ref_name)) < 0 && error != GIT_ENOTFOUND)
+ if ((error = loose_delete(backend, ref_name)) < 0 && error != GIT_ENOTFOUND)
goto cleanup;
if (error == GIT_ENOTFOUND) {
- error = loose_deleted ? 0 : ref_error_notfound(ref_name);
+ error = packed_deleted ? 0 : ref_error_notfound(ref_name);
goto cleanup;
}
cleanup:
git_filebuf_cleanup(file);
- if (loose_deleted)
+ if (error == 0)
refdb_fs_backend__prune_refs(backend, ref_name, "");
return error;
}