refs: move duplicate refname update check to generic layer
Move the tracking of refnames in `affected_refnames` from individual backends into the generic layer in 'refs.c'. This centralizes the duplicate refname detection that was previously handled separately by each backend. Make some changes to accommodate this move: - Add a `string_list` field `refnames` to `ref_transaction` to contain all the references in a transaction. This field is updated whenever a new update is added via `ref_transaction_add_update`, so manual additions in reference backends are dropped. - Modify the backends to use this field internally as needed. The backends need to check if an update for refname already exists when splitting symrefs or adding an update for 'HEAD'. - In the reftable backend, within `reftable_be_transaction_prepare()`, move the `string_list_has_string()` check above `ref_transaction_add_update()`. Since `ref_transaction_add_update()` automatically adds the refname to `transaction->refnames`, performing the check after will always return true, so we perform the check before adding the update. This helps reduce duplication of functionality between the backends and makes it easier to make changes in a more centralized manner. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> Acked-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Karthik Nayak committed
Apr 8, 2025 at 10:51 UTC
c3baddf04f8fb20bec590f492f00189fd6c02a35
5 files changed
+51
-114
refs.c
+17
@@ -1175,6 +1175,7 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
1175
CALLOC_ARRAY(tr, 1);
1176
tr->ref_store = refs;
1177
tr->flags = flags;
1178
+ string_list_init_dup(&tr->refnames);
1179
return tr;
1180
}
1181
@@ -1205,6 +1206,7 @@ void ref_transaction_free(struct ref_transaction *transaction)
1206
free((char *)transaction->updates[i]->old_target);
1207
free(transaction->updates[i]);
1208
}
1209
+ string_list_clear(&transaction->refnames, 0);
1210
free(transaction->updates);
1211
free(transaction);
1212
}
@@ -1218,6 +1220,7 @@ struct ref_update *ref_transaction_add_update(
1220
const char *committer_info,
1221
const char *msg)
1222
{
1223
+ struct string_list_item *item;
1224
struct ref_update *update;
1225
1226
if (transaction->state != REF_TRANSACTION_OPEN)
@@ -1245,6 +1248,16 @@ struct ref_update *ref_transaction_add_update(
1248
update->msg = normalize_reflog_message(msg);
1249
}
1250
1251
+ /*
1252
+ * This list is generally used by the backends to avoid duplicates.
1253
+ * But we do support multiple log updates for a given refname within
1254
+ * a single transaction.
1255
+ */
1256
+ if (!(update->flags & REF_LOG_ONLY)) {
1257
+ item = string_list_append(&transaction->refnames, refname);
1258
+ item->util = update;
1259
+ }
1260
+
1261
return update;
1262
}
1263
@@ -2405,6 +2418,10 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
2418
return -1;
2419
}
2420
2421
+ string_list_sort(&transaction->refnames);
2422
+ if (ref_update_reject_duplicates(&transaction->refnames, err))
2423
+ return TRANSACTION_GENERIC_ERROR;
2424
+
2425
ret = refs->be->transaction_prepare(refs, transaction, err);
2426
if (ret)
2427
return ret;
refs/files-backend.c
+14
-53
@@ -2378,9 +2378,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
2378
*/
2379
static int split_head_update(struct ref_update *update,
2380
struct ref_transaction *transaction,
2381
- const char *head_ref,
2382
- struct string_list *affected_refnames,
2383
- struct strbuf *err)
2381
+ const char *head_ref, struct strbuf *err)
2382
{
2383
struct ref_update *new_update;
2384
@@ -2398,7 +2396,7 @@ static int split_head_update(struct ref_update *update,
2396
* transaction. This check is O(lg N) in the transaction
2397
* size, but it happens at most once per transaction.
2398
*/
2401
- if (string_list_has_string(affected_refnames, "HEAD")) {
2399
+ if (string_list_has_string(&transaction->refnames, "HEAD")) {
2400
/* An entry already existed */
2401
strbuf_addf(err,
2402
"multiple updates for 'HEAD' (including one "
@@ -2420,7 +2418,6 @@ static int split_head_update(struct ref_update *update,
2418
*/
2419
if (strcmp(new_update->refname, "HEAD"))
2420
BUG("%s unexpectedly not 'HEAD'", new_update->refname);
2423
- string_list_insert(affected_refnames, new_update->refname);
2421
2422
return 0;
2423
}
@@ -2436,7 +2433,6 @@ static int split_head_update(struct ref_update *update,
2433
static int split_symref_update(struct ref_update *update,
2434
const char *referent,
2435
struct ref_transaction *transaction,
2439
- struct string_list *affected_refnames,
2436
struct strbuf *err)
2437
{
2438
struct ref_update *new_update;
@@ -2448,7 +2444,7 @@ static int split_symref_update(struct ref_update *update,
2444
* size, but it happens at most once per symref in a
2445
* transaction.
2446
*/
2451
- if (string_list_has_string(affected_refnames, referent)) {
2447
+ if (string_list_has_string(&transaction->refnames, referent)) {
2448
/* An entry already exists */
2449
strbuf_addf(err,
2450
"multiple updates for '%s' (including one "
@@ -2486,15 +2482,6 @@ static int split_symref_update(struct ref_update *update,
2482
update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
2483
update->flags &= ~REF_HAVE_OLD;
2484
2489
- /*
2490
- * Add the referent. This insertion is O(N) in the transaction
2491
- * size, but it happens at most once per symref in a
2492
- * transaction. Make sure to add new_update->refname, which will
2493
- * be valid as long as affected_refnames is in use, and NOT
2494
- * referent, which might soon be freed by our caller.
2495
- */
2496
- string_list_insert(affected_refnames, new_update->refname);
2497
-
2485
return 0;
2486
}
2487
@@ -2558,7 +2545,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,
2545
struct ref_transaction *transaction,
2546
const char *head_ref,
2547
struct string_list *refnames_to_check,
2561
- struct string_list *affected_refnames,
2548
struct strbuf *err)
2549
{
2550
struct strbuf referent = STRBUF_INIT;
@@ -2575,8 +2561,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
2561
update->flags |= REF_DELETING;
2562
2563
if (head_ref) {
2578
- ret = split_head_update(update, transaction, head_ref,
2579
- affected_refnames, err);
2564
+ ret = split_head_update(update, transaction, head_ref, err);
2565
if (ret)
2566
goto out;
2567
}
@@ -2586,9 +2571,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,
2571
lock->count++;
2572
} else {
2573
ret = lock_raw_ref(refs, update->refname, mustexist,
2589
- refnames_to_check, affected_refnames,
2590
- &lock, &referent,
2591
- &update->type, err);
2574
+ refnames_to_check, &transaction->refnames,
2575
+ &lock, &referent, &update->type, err);
2576
if (ret) {
2577
char *reason;
2578
@@ -2642,9 +2626,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,
2626
* of processing the split-off update, so we
2627
* don't have to do it here.
2628
*/
2645
- ret = split_symref_update(update,
2646
- referent.buf, transaction,
2647
- affected_refnames, err);
2629
+ ret = split_symref_update(update, referent.buf,
2630
+ transaction, err);
2631
if (ret)
2632
goto out;
2633
}
@@ -2799,7 +2782,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,
2782
"ref_transaction_prepare");
2783
size_t i;
2784
int ret = 0;
2802
- struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
2785
struct string_list refnames_to_check = STRING_LIST_INIT_NODUP;
2786
char *head_ref = NULL;
2787
int head_type;
@@ -2818,12 +2800,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
2800
transaction->backend_data = backend_data;
2801
2802
/*
2821
- * Fail if a refname appears more than once in the
2822
- * transaction. (If we end up splitting up any updates using
2823
- * split_symref_update() or split_head_update(), those
2824
- * functions will check that the new updates don't have the
2825
- * same refname as any existing ones.) Also fail if any of the
2826
- * updates use REF_IS_PRUNING without REF_NO_DEREF.
2803
+ * Fail if any of the updates use REF_IS_PRUNING without REF_NO_DEREF.
2804
*/
2805
for (i = 0; i < transaction->nr; i++) {
2806
struct ref_update *update = transaction->updates[i];
@@ -2831,16 +2808,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,
2808
if ((update->flags & REF_IS_PRUNING) &&
2809
!(update->flags & REF_NO_DEREF))
2810
BUG("REF_IS_PRUNING set without REF_NO_DEREF");
2834
-
2835
- if (update->flags & REF_LOG_ONLY)
2836
- continue;
2837
-
2838
- string_list_append(&affected_refnames, update->refname);
2839
- }
2840
- string_list_sort(&affected_refnames);
2841
- if (ref_update_reject_duplicates(&affected_refnames, err)) {
2842
- ret = TRANSACTION_GENERIC_ERROR;
2843
- goto cleanup;
2811
}
2812
2813
/*
@@ -2882,7 +2849,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
2849
2850
ret = lock_ref_for_update(refs, update, transaction,
2851
head_ref, &refnames_to_check,
2885
- &affected_refnames, err);
2852
+ err);
2853
if (ret)
2854
goto cleanup;
2855
@@ -2929,7 +2896,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,
2896
* So instead, we accept the race for now.
2897
*/
2898
if (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check,
2932
- &affected_refnames, NULL, 0, err)) {
2899
+ &transaction->refnames, NULL, 0, err)) {
2900
ret = TRANSACTION_NAME_CONFLICT;
2901
goto cleanup;
2902
}
@@ -2975,7 +2942,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,
2942
2943
cleanup:
2944
free(head_ref);
2978
- string_list_clear(&affected_refnames, 0);
2945
string_list_clear(&refnames_to_check, 0);
2946
2947
if (ret)
@@ -3050,13 +3016,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
3016
if (transaction->state != REF_TRANSACTION_PREPARED)
3017
BUG("commit called for transaction that is not prepared");
3018
3053
- /* Fail if a refname appears more than once in the transaction: */
3054
- for (i = 0; i < transaction->nr; i++)
3055
- if (!(transaction->updates[i]->flags & REF_LOG_ONLY))
3056
- string_list_append(&affected_refnames,
3057
- transaction->updates[i]->refname);
3058
- string_list_sort(&affected_refnames);
3059
- if (ref_update_reject_duplicates(&affected_refnames, err)) {
3019
+ string_list_sort(&transaction->refnames);
3020
+ if (ref_update_reject_duplicates(&transaction->refnames, err)) {
3021
ret = TRANSACTION_GENERIC_ERROR;
3022
goto cleanup;
3023
}
@@ -3074,7 +3035,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
3035
* that we are creating already exists.
3036
*/
3037
if (refs_for_each_rawref(&refs->base, ref_present,
3077
- &affected_refnames))
3038
+ &transaction->refnames))
3039
BUG("initial ref transaction called with existing refs");
3040
3041
packed_transaction = ref_store_transaction_begin(refs->packed_ref_store,
refs/packed-backend.c
+1
-24
@@ -1622,8 +1622,6 @@ int is_packed_transaction_needed(struct ref_store *ref_store,
1622
struct packed_transaction_backend_data {
1623
/* True iff the transaction owns the packed-refs lock. */
1624
int own_lock;
1625
-
1626
- struct string_list updates;
1625
};
1626
1627
static void packed_transaction_cleanup(struct packed_ref_store *refs,
@@ -1632,8 +1630,6 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,
1630
struct packed_transaction_backend_data *data = transaction->backend_data;
1631
1632
if (data) {
1635
- string_list_clear(&data->updates, 0);
1636
-
1633
if (is_tempfile_active(refs->tempfile))
1634
delete_tempfile(&refs->tempfile);
1635
@@ -1658,7 +1654,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
1654
REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,
1655
"ref_transaction_prepare");
1656
struct packed_transaction_backend_data *data;
1661
- size_t i;
1657
int ret = TRANSACTION_GENERIC_ERROR;
1658
1659
/*
@@ -1671,34 +1666,16 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
1666
*/
1667
1668
CALLOC_ARRAY(data, 1);
1674
- string_list_init_nodup(&data->updates);
1669
1670
transaction->backend_data = data;
1671
1678
- /*
1679
- * Stick the updates in a string list by refname so that we
1680
- * can sort them:
1681
- */
1682
- for (i = 0; i < transaction->nr; i++) {
1683
- struct ref_update *update = transaction->updates[i];
1684
- struct string_list_item *item =
1685
- string_list_append(&data->updates, update->refname);
1686
-
1687
- /* Store a pointer to update in item->util: */
1688
- item->util = update;
1689
- }
1690
- string_list_sort(&data->updates);
1691
-
1692
- if (ref_update_reject_duplicates(&data->updates, err))
1693
- goto failure;
1694
-
1672
if (!is_lock_file_locked(&refs->lock)) {
1673
if (packed_refs_lock(ref_store, 0, err))
1674
goto failure;
1675
data->own_lock = 1;
1676
}
1677
1701
- if (write_with_updates(refs, &data->updates, err))
1678
+ if (write_with_updates(refs, &transaction->refnames, err))
1679
goto failure;
1680
1681
transaction->state = REF_TRANSACTION_PREPARED;
refs/refs-internal.h
+2
@@ -3,6 +3,7 @@
3
4
#include "refs.h"
5
#include "iterator.h"
6
+#include "string-list.h"
7
8
struct fsck_options;
9
struct ref_transaction;
@@ -198,6 +199,7 @@ enum ref_transaction_state {
199
struct ref_transaction {
200
struct ref_store *ref_store;
201
struct ref_update **updates;
202
+ struct string_list refnames;
203
size_t alloc;
204
size_t nr;
205
enum ref_transaction_state state;
refs/reftable-backend.c
+17
-37
@@ -1076,7 +1076,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
1076
struct reftable_ref_store *refs =
1077
reftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, "ref_transaction_prepare");
1078
struct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT;
1079
- struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
1079
struct string_list refnames_to_check = STRING_LIST_INIT_NODUP;
1080
struct reftable_transaction_data *tx_data = NULL;
1081
struct reftable_backend *be;
@@ -1101,10 +1100,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
1100
transaction->updates[i], err);
1101
if (ret)
1102
goto done;
1104
-
1105
- if (!(transaction->updates[i]->flags & REF_LOG_ONLY))
1106
- string_list_append(&affected_refnames,
1107
- transaction->updates[i]->refname);
1103
}
1104
1105
/*
@@ -1116,17 +1111,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
1111
tx_data->args[i].updates_alloc = tx_data->args[i].updates_expected;
1112
}
1113
1119
- /*
1120
- * Fail if a refname appears more than once in the transaction.
1121
- * This code is taken from the files backend and is a good candidate to
1122
- * be moved into the generic layer.
1123
- */
1124
- string_list_sort(&affected_refnames);
1125
- if (ref_update_reject_duplicates(&affected_refnames, err)) {
1126
- ret = TRANSACTION_GENERIC_ERROR;
1127
- goto done;
1128
- }
1129
-
1114
/*
1115
* TODO: it's dubious whether we should reload the stack that "HEAD"
1116
* belongs to or not. In theory, it may happen that we only modify
@@ -1194,14 +1178,12 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
1178
!(u->flags & REF_LOG_ONLY) &&
1179
!(u->flags & REF_UPDATE_VIA_HEAD) &&
1180
!strcmp(rewritten_ref, head_referent.buf)) {
1197
- struct ref_update *new_update;
1198
-
1181
/*
1182
* First make sure that HEAD is not already in the
1183
* transaction. This check is O(lg N) in the transaction
1184
* size, but it happens at most once per transaction.
1185
*/
1204
- if (string_list_has_string(&affected_refnames, "HEAD")) {
1186
+ if (string_list_has_string(&transaction->refnames, "HEAD")) {
1187
/* An entry already existed */
1188
strbuf_addf(err,
1189
_("multiple updates for 'HEAD' (including one "
@@ -1211,12 +1193,11 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
1193
goto done;
1194
}
1195
1214
- new_update = ref_transaction_add_update(
1215
- transaction, "HEAD",
1216
- u->flags | REF_LOG_ONLY | REF_NO_DEREF,
1217
- &u->new_oid, &u->old_oid, NULL, NULL, NULL,
1218
- u->msg);
1219
- string_list_insert(&affected_refnames, new_update->refname);
1196
+ ref_transaction_add_update(
1197
+ transaction, "HEAD",
1198
+ u->flags | REF_LOG_ONLY | REF_NO_DEREF,
1199
+ &u->new_oid, &u->old_oid, NULL, NULL, NULL,
1200
+ u->msg);
1201
}
1202
1203
ret = reftable_backend_read_ref(be, rewritten_ref,
@@ -1281,6 +1262,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
1262
if (!strcmp(rewritten_ref, "HEAD"))
1263
new_flags |= REF_UPDATE_VIA_HEAD;
1264
1265
+ if (string_list_has_string(&transaction->refnames, referent.buf)) {
1266
+ strbuf_addf(err,
1267
+ _("multiple updates for '%s' (including one "
1268
+ "via symref '%s') are not allowed"),
1269
+ referent.buf, u->refname);
1270
+ ret = TRANSACTION_NAME_CONFLICT;
1271
+ goto done;
1272
+ }
1273
+
1274
/*
1275
* If we are updating a symref (eg. HEAD), we should also
1276
* update the branch that the symref points to.
@@ -1305,16 +1295,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
1295
*/
1296
u->flags |= REF_LOG_ONLY | REF_NO_DEREF;
1297
u->flags &= ~REF_HAVE_OLD;
1308
-
1309
- if (string_list_has_string(&affected_refnames, new_update->refname)) {
1310
- strbuf_addf(err,
1311
- _("multiple updates for '%s' (including one "
1312
- "via symref '%s') are not allowed"),
1313
- referent.buf, u->refname);
1314
- ret = TRANSACTION_NAME_CONFLICT;
1315
- goto done;
1316
- }
1317
- string_list_insert(&affected_refnames, new_update->refname);
1298
}
1299
}
1300
@@ -1383,7 +1363,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
1363
}
1364
}
1365
1386
- ret = refs_verify_refnames_available(ref_store, &refnames_to_check, &affected_refnames, NULL,
1366
+ ret = refs_verify_refnames_available(ref_store, &refnames_to_check,
1367
+ &transaction->refnames, NULL,
1368
transaction->flags & REF_TRANSACTION_FLAG_INITIAL,
1369
err);
1370
if (ret < 0)
@@ -1401,7 +1382,6 @@ done:
1382
strbuf_addf(err, _("reftable: transaction prepare: %s"),
1383
reftable_error_str(ret));
1384
}
1404
- string_list_clear(&affected_refnames, 0);
1385
strbuf_release(&referent);
1386
strbuf_release(&head_referent);
1387
string_list_clear(&refnames_to_check, 0);