summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorMichael Haggerty <mhagger@alum.mit.edu>2017-10-24 17:16:25 +0200
committerJunio C Hamano <gitster@pobox.com>2017-10-25 15:08:26 +0900
commitda5267f1b66dfe83a92698e49fa10dee5f74224f (patch)
treeb6bb17b9c90c1417281c8f277eb6e54474dcd78d
parent2e9de01aa0c44b539fca05b3ee552013d64c283a (diff)
downloadgit-da5267f1b66dfe83a92698e49fa10dee5f74224f.tar.gz
files_transaction_prepare(): fix handling of ref lock failuremh/ref-locking-fix
Since dc39e09942 (files_ref_store: use a transaction to update packed refs, 2017-09-08), failure to lock a reference has been handled incorrectly by `files_transaction_prepare()`. If `lock_ref_for_update()` fails in the lock-acquisition loop of that function, it sets `ret` then breaks out of that loop. Prior to dc39e09942, that was OK, because the only thing following the loop was the cleanup code. But dc39e09942 added another blurb of code between the loop and the cleanup. That blurb sometimes resets `ret` to zero, making the cleanup code think that the locking was successful. Specifically, whenever * One or more reference deletions have been processed successfully in the lock-acquisition loop. (Processing the first such reference causes a packed-ref transaction to be initialized.) * Then `lock_ref_for_update()` fails for a subsequent reference. Such a failure can happen for a number of reasons, such as the old SHA-1 not being correct, lock contention, etc. This causes a `break` out of the lock-acquisition loop. * The `packed-refs` lock is acquired successfully and `ref_transaction_prepare()` succeeds for the packed-ref transaction. This has the effect of resetting `ret` back to 0, and making the cleanup code think that lock acquisition was successful. In that case, any reference updates that were processed prior to breaking out of the loop would be carried out (loose and packed), but the reference that couldn't be locked and any subsequent references would silently be ignored. This can easily cause data loss if, for example, the user was trying to push a new name for an existing branch while deleting the old name. After the push, the branch could be left unreachable, and could even subsequently be garbage-collected. This problem was noticed in the context of deleting one reference and creating another in a single transaction, when the two references D/F conflict with each other, like git update-ref --stdin <<EOF delete refs/foo create refs/foo/bar HEAD EOF This triggers the above bug because the deletion is processed successfully for `refs/foo`, then the D/F conflict causes `lock_ref_for_update()` to fail when `refs/foo/bar` is processed. In this case the transaction *should* fail, but instead it causes `refs/foo` to be deleted without creating `refs/foo`. This could easily result in data loss. The fix is simple: instead of just breaking out of the loop, jump directly to the cleanup code. This fixes some tests in t1404 that were added in the previous commit. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
-rw-r--r--refs/files-backend.c2
-rwxr-xr-xt/t1404-update-ref-errors.sh16
2 files changed, 9 insertions, 9 deletions
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 961424a4ea..a8c29b7865 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2527,7 +2527,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
ret = lock_ref_for_update(refs, update, transaction,
head_ref, &affected_refnames, err);
if (ret)
- break;
+ goto cleanup;
if (update->flags & REF_DELETING &&
!(update->flags & REF_LOG_ONLY) &&
diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
index f26a17df33..3a887b5113 100755
--- a/t/t1404-update-ref-errors.sh
+++ b/t/t1404-update-ref-errors.sh
@@ -271,11 +271,11 @@ test_expect_success 'D/F conflict prevents add short + delete long' '
df_test refs/df-as-dl --add-del foo foo/bar
'
-test_expect_failure 'D/F conflict prevents delete long + add short' '
+test_expect_success 'D/F conflict prevents delete long + add short' '
df_test refs/df-dl-as --del-add foo/bar foo
'
-test_expect_failure 'D/F conflict prevents delete short + add long' '
+test_expect_success 'D/F conflict prevents delete short + add long' '
df_test refs/df-ds-al --del-add foo foo/bar
'
@@ -287,17 +287,17 @@ test_expect_success 'D/F conflict prevents add short + delete long packed' '
df_test refs/df-as-dlp --pack --add-del foo foo/bar
'
-test_expect_failure 'D/F conflict prevents delete long packed + add short' '
+test_expect_success 'D/F conflict prevents delete long packed + add short' '
df_test refs/df-dlp-as --pack --del-add foo/bar foo
'
-test_expect_failure 'D/F conflict prevents delete short packed + add long' '
+test_expect_success 'D/F conflict prevents delete short packed + add long' '
df_test refs/df-dsp-al --pack --del-add foo foo/bar
'
# Try some combinations involving symbolic refs...
-test_expect_failure 'D/F conflict prevents indirect add long + delete short' '
+test_expect_success 'D/F conflict prevents indirect add long + delete short' '
df_test refs/df-ial-ds --sym-add --add-del foo/bar foo
'
@@ -309,11 +309,11 @@ test_expect_success 'D/F conflict prevents indirect add short + indirect delete
df_test refs/df-ias-idl --sym-add --sym-del --add-del foo foo/bar
'
-test_expect_failure 'D/F conflict prevents indirect delete long + indirect add short' '
+test_expect_success 'D/F conflict prevents indirect delete long + indirect add short' '
df_test refs/df-idl-ias --sym-add --sym-del --del-add foo/bar foo
'
-test_expect_failure 'D/F conflict prevents indirect add long + delete short packed' '
+test_expect_success 'D/F conflict prevents indirect add long + delete short packed' '
df_test refs/df-ial-dsp --sym-add --pack --add-del foo/bar foo
'
@@ -325,7 +325,7 @@ test_expect_success 'D/F conflict prevents add long + indirect delete short pack
df_test refs/df-al-idsp --sym-del --pack --add-del foo/bar foo
'
-test_expect_failure 'D/F conflict prevents indirect delete long packed + indirect add short' '
+test_expect_success 'D/F conflict prevents indirect delete long packed + indirect add short' '
df_test refs/df-idlp-ias --sym-add --sym-del --pack --del-add foo/bar foo
'