repack_without_refs(): don't lock or unlock the packed refs
Change `repack_without_refs()` to expect the packed-refs lock to be held already, and not to release the lock before returning. Change the callers to deal with lock management. This change makes it possible for callers to hold the packed-refs lock for a longer span of time, a possibility that will eventually make it possible to fix some longstanding races. The only semantic change here is that `repack_without_refs()` used to forget to release the lock in the `if (!removed)` exit path. That omission is now fixed. Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Haggerty committed
Jul 1, 2017 at 20:31 UTC
e5cc7d7d2bf2fbb3dfba81fe0c0c2981607fbc84
2 files changed
+39
-40
refs/files-backend.c
+31
-16
@@ -1149,24 +1149,16 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
1149
if (!refnames->nr)
1150
return 0;
1151
1152
- result = repack_without_refs(refs->packed_ref_store, refnames, &err);
1153
- if (result) {
1154
- /*
1155
- * If we failed to rewrite the packed-refs file, then
1156
- * it is unsafe to try to remove loose refs, because
1157
- * doing so might expose an obsolete packed value for
1158
- * a reference that might even point at an object that
1159
- * has been garbage collected.
1160
- */
1161
- if (refnames->nr == 1)
1162
- error(_("could not delete reference %s: %s"),
1163
- refnames->items[0].string, err.buf);
1164
- else
1165
- error(_("could not delete references: %s"), err.buf);
1152
+ if (packed_refs_lock(refs->packed_ref_store, 0, &err))
1153
+ goto error;
1154
1167
- goto out;
1155
+ if (repack_without_refs(refs->packed_ref_store, refnames, &err)) {
1156
+ packed_refs_unlock(refs->packed_ref_store);
1157
+ goto error;
1158
}
1159
1160
+ packed_refs_unlock(refs->packed_ref_store);
1161
+
1162
for (i = 0; i < refnames->nr; i++) {
1163
const char *refname = refnames->items[i].string;
1164
@@ -1174,9 +1166,24 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
1166
result |= error(_("could not remove reference %s"), refname);
1167
}
1168
1177
-out:
1169
strbuf_release(&err);
1170
return result;
1171
+
1172
+error:
1173
+ /*
1174
+ * If we failed to rewrite the packed-refs file, then it is
1175
+ * unsafe to try to remove loose refs, because doing so might
1176
+ * expose an obsolete packed value for a reference that might
1177
+ * even point at an object that has been garbage collected.
1178
+ */
1179
+ if (refnames->nr == 1)
1180
+ error(_("could not delete reference %s: %s"),
1181
+ refnames->items[0].string, err.buf);
1182
+ else
1183
+ error(_("could not delete references: %s"), err.buf);
1184
+
1185
+ strbuf_release(&err);
1186
+ return -1;
1187
}
1188
1189
/*
@@ -2569,11 +2576,19 @@ static int files_transaction_finish(struct ref_store *ref_store,
2576
}
2577
}
2578
2579
+ if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
2580
+ ret = TRANSACTION_GENERIC_ERROR;
2581
+ goto cleanup;
2582
+ }
2583
+
2584
if (repack_without_refs(refs->packed_ref_store, &refs_to_delete, err)) {
2585
ret = TRANSACTION_GENERIC_ERROR;
2586
+ packed_refs_unlock(refs->packed_ref_store);
2587
goto cleanup;
2588
}
2589
2590
+ packed_refs_unlock(refs->packed_ref_store);
2591
+
2592
/* Delete the reflogs of any references that were deleted: */
2593
for_each_string_list_item(ref_to_delete, &refs_to_delete) {
2594
strbuf_reset(&sb);
refs/packed-backend.c
+8
-24
@@ -669,25 +669,12 @@ error:
669
return -1;
670
}
671
672
-/*
673
- * Rollback the lockfile for the packed-refs file, and discard the
674
- * in-memory packed reference cache. (The packed-refs file will be
675
- * read anew if it is needed again after this function is called.)
676
- */
677
-static void rollback_packed_refs(struct packed_ref_store *refs)
678
-{
679
- packed_assert_main_repository(refs, "rollback_packed_refs");
680
-
681
- if (!is_lock_file_locked(&refs->lock))
682
- die("BUG: packed-refs not locked");
683
- packed_refs_unlock(&refs->base);
684
- clear_packed_ref_cache(refs);
685
-}
686
-
672
/*
673
* Rewrite the packed-refs file, omitting any refs listed in
674
* 'refnames'. On error, leave packed-refs unchanged, write an error
690
- * message to 'err', and return a nonzero value.
675
+ * message to 'err', and return a nonzero value. The packed refs lock
676
+ * must be held when calling this function; it will still be held when
677
+ * the function returns.
678
*
679
* The refs in 'refnames' needn't be sorted. `err` must not be NULL.
680
*/
@@ -700,11 +687,13 @@ int repack_without_refs(struct ref_store *ref_store,
687
struct ref_dir *packed;
688
struct string_list_item *refname;
689
int needs_repacking = 0, removed = 0;
703
- int ret;
690
691
packed_assert_main_repository(refs, "repack_without_refs");
692
assert(err);
693
694
+ if (!is_lock_file_locked(&refs->lock))
695
+ die("BUG: repack_without_refs called without holding lock");
696
+
697
/* Look for a packed ref */
698
for_each_string_list_item(refname, refnames) {
699
if (get_packed_ref(refs, refname->string)) {
@@ -717,9 +706,6 @@ int repack_without_refs(struct ref_store *ref_store,
706
if (!needs_repacking)
707
return 0; /* no refname exists in packed refs */
708
720
- if (packed_refs_lock(&refs->base, 0, err))
721
- return -1;
722
-
709
packed = get_packed_refs(refs);
710
711
/* Remove refnames from the cache */
@@ -731,14 +717,12 @@ int repack_without_refs(struct ref_store *ref_store,
717
* All packed entries disappeared while we were
718
* acquiring the lock.
719
*/
734
- rollback_packed_refs(refs);
720
+ clear_packed_ref_cache(refs);
721
return 0;
722
}
723
724
/* Write what remains */
739
- ret = commit_packed_refs(&refs->base, err);
740
- packed_refs_unlock(ref_store);
741
- return ret;
725
+ return commit_packed_refs(&refs->base, err);
726
}
727
728
static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)