fetch: make `--atomic` flag cover backfilling of tags
When fetching references from a remote we by default also fetch all tags which point into the history we have fetched. This is a separate step performed after updating local references because it requires us to walk over the history on the client-side to determine whether the remote has announced any tags which point to one of the fetched commits. This backfilling of tags isn't covered by the `--atomic` flag: right now, it only applies to the step where we update our local references. This is an oversight at the time the flag was introduced: its purpose is to either update all references or none, but right now we happily update local references even in the case where backfilling failed. Fix this by pulling up creation of the reference transaction such that we can pass the same transaction to both the code which updates local references and to the code which backfills tags. This allows us to only commit the transaction in case both actions succeed. Note that we also have to start passing the transaction into `find_non_local_tags()`: this function is responsible for finding all tags which we need to backfill. Right now, it will happily return tags which have already been updated with our local references. But when we use a single transaction for both local references and backfilling then it may happen that we try to queue the same reference update twice to the transaction, which consequently triggers a bug. We thus have to skip over any tags which have already been queued. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Patrick Steinhardt committed
Feb 17, 2022 at 14:04 UTC
b3a804663c4682f6df55dd6703f8f8af9a7c6ab5
2 files changed
+69
-34
builtin/fetch.c
+66
-26
@@ -349,7 +349,19 @@ static void clear_item(struct refname_hash_entry *item)
349
item->ignore = 1;
350
}
351
352
+
353
+static void add_already_queued_tags(const char *refname,
354
+ const struct object_id *old_oid,
355
+ const struct object_id *new_oid,
356
+ void *cb_data)
357
+{
358
+ struct hashmap *queued_tags = cb_data;
359
+ if (starts_with(refname, "refs/tags/") && new_oid)
360
+ (void) refname_hash_add(queued_tags, refname, new_oid);
361
+}
362
+
363
static void find_non_local_tags(const struct ref *refs,
364
+ struct ref_transaction *transaction,
365
struct ref **head,
366
struct ref ***tail)
367
{
@@ -367,6 +379,16 @@ static void find_non_local_tags(const struct ref *refs,
379
create_fetch_oidset(head, &fetch_oids);
380
381
for_each_ref(add_one_refname, &existing_refs);
382
+
383
+ /*
384
+ * If we already have a transaction, then we need to filter out all
385
+ * tags which have already been queued up.
386
+ */
387
+ if (transaction)
388
+ ref_transaction_for_each_queued_update(transaction,
389
+ add_already_queued_tags,
390
+ &existing_refs);
391
+
392
for (ref = refs; ref; ref = ref->next) {
393
if (!starts_with(ref->name, "refs/tags/"))
394
continue;
@@ -600,7 +622,7 @@ static struct ref *get_ref_map(struct remote *remote,
622
/* also fetch all tags */
623
get_fetch_map(remote_refs, tag_refspec, &tail, 0);
624
else if (tags == TAGS_DEFAULT && *autotags)
603
- find_non_local_tags(remote_refs, &ref_map, &tail);
625
+ find_non_local_tags(remote_refs, NULL, &ref_map, &tail);
626
627
/* Now append any refs to be updated opportunistically: */
628
*tail = orefs;
@@ -1083,12 +1105,12 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
1105
"to avoid this check\n");
1106
1107
static int store_updated_refs(const char *raw_url, const char *remote_name,
1086
- int connectivity_checked, struct ref *ref_map,
1108
+ int connectivity_checked,
1109
+ struct ref_transaction *transaction, struct ref *ref_map,
1110
struct fetch_head *fetch_head, struct worktree **worktrees)
1111
{
1112
int url_len, i, rc = 0;
1113
struct strbuf note = STRBUF_INIT, err = STRBUF_INIT;
1091
- struct ref_transaction *transaction = NULL;
1114
const char *what, *kind;
1115
struct ref *rm;
1116
char *url;
@@ -1110,14 +1132,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
1132
}
1133
}
1134
1113
- if (atomic_fetch) {
1114
- transaction = ref_transaction_begin(&err);
1115
- if (!transaction) {
1116
- error("%s", err.buf);
1117
- goto abort;
1118
- }
1119
- }
1120
-
1135
prepare_format_display(ref_map);
1136
1137
/*
@@ -1233,14 +1247,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
1247
}
1248
}
1249
1236
- if (!rc && transaction) {
1237
- rc = ref_transaction_commit(transaction, &err);
1238
- if (rc) {
1239
- error("%s", err.buf);
1240
- goto abort;
1241
- }
1242
- }
1243
-
1250
if (rc & STORE_REF_ERROR_DF_CONFLICT)
1251
error(_("some local refs could not be updated; try running\n"
1252
" 'git remote prune %s' to remove any old, conflicting "
@@ -1258,7 +1264,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
1264
abort:
1265
strbuf_release(¬e);
1266
strbuf_release(&err);
1261
- ref_transaction_free(transaction);
1267
free(url);
1268
return rc;
1269
}
@@ -1299,6 +1304,7 @@ static int check_exist_and_connected(struct ref *ref_map)
1304
}
1305
1306
static int fetch_and_consume_refs(struct transport *transport,
1307
+ struct ref_transaction *transaction,
1308
struct ref *ref_map,
1309
struct fetch_head *fetch_head,
1310
struct worktree **worktrees)
@@ -1323,7 +1329,8 @@ static int fetch_and_consume_refs(struct transport *transport,
1329
1330
trace2_region_enter("fetch", "consume_refs", the_repository);
1331
ret = store_updated_refs(transport->url, transport->remote->name,
1326
- connectivity_checked, ref_map, fetch_head, worktrees);
1332
+ connectivity_checked, transaction, ref_map,
1333
+ fetch_head, worktrees);
1334
trace2_region_leave("fetch", "consume_refs", the_repository);
1335
1336
out:
@@ -1496,6 +1503,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
1503
}
1504
1505
static int backfill_tags(struct transport *transport,
1506
+ struct ref_transaction *transaction,
1507
struct ref *ref_map,
1508
struct fetch_head *fetch_head,
1509
struct worktree **worktrees)
@@ -1519,7 +1527,7 @@ static int backfill_tags(struct transport *transport,
1527
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
1528
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
1529
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
1522
- retcode = fetch_and_consume_refs(transport, ref_map, fetch_head, worktrees);
1530
+ retcode = fetch_and_consume_refs(transport, transaction, ref_map, fetch_head, worktrees);
1531
1532
if (gsecondary) {
1533
transport_disconnect(gsecondary);
@@ -1532,6 +1540,7 @@ static int backfill_tags(struct transport *transport,
1540
static int do_fetch(struct transport *transport,
1541
struct refspec *rs)
1542
{
1543
+ struct ref_transaction *transaction = NULL;
1544
struct ref *ref_map = NULL;
1545
int autotags = (transport->remote->fetch_tags == 1);
1546
int retcode = 0;
@@ -1541,6 +1550,7 @@ static int do_fetch(struct transport *transport,
1550
int must_list_refs = 1;
1551
struct worktree **worktrees = get_worktrees();
1552
struct fetch_head fetch_head = { 0 };
1553
+ struct strbuf err = STRBUF_INIT;
1554
1555
if (tags == TAGS_DEFAULT) {
1556
if (transport->remote->fetch_tags == 2)
@@ -1602,6 +1612,14 @@ static int do_fetch(struct transport *transport,
1612
if (retcode)
1613
goto cleanup;
1614
1615
+ if (atomic_fetch) {
1616
+ transaction = ref_transaction_begin(&err);
1617
+ if (!transaction) {
1618
+ retcode = error("%s", err.buf);
1619
+ goto cleanup;
1620
+ }
1621
+ }
1622
+
1623
if (tags == TAGS_DEFAULT && autotags)
1624
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
1625
if (prune) {
@@ -1621,7 +1639,7 @@ static int do_fetch(struct transport *transport,
1639
retcode = 1;
1640
}
1641
1624
- if (fetch_and_consume_refs(transport, ref_map, &fetch_head, worktrees)) {
1642
+ if (fetch_and_consume_refs(transport, transaction, ref_map, &fetch_head, worktrees)) {
1643
retcode = 1;
1644
goto cleanup;
1645
}
@@ -1633,21 +1651,37 @@ static int do_fetch(struct transport *transport,
1651
if (tags == TAGS_DEFAULT && autotags) {
1652
struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
1653
1636
- find_non_local_tags(remote_refs, &tags_ref_map, &tail);
1654
+ find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
1655
if (tags_ref_map) {
1656
/*
1657
* If backfilling of tags fails then we want to tell
1658
* the user so, but we have to continue regardless to
1659
* populate upstream information of the references we
1642
- * have already fetched above.
1660
+ * have already fetched above. The exception though is
1661
+ * when `--atomic` is passed: in that case we'll abort
1662
+ * the transaction and don't commit anything.
1663
*/
1644
- if (backfill_tags(transport, tags_ref_map, &fetch_head, worktrees))
1664
+ if (backfill_tags(transport, transaction, tags_ref_map,
1665
+ &fetch_head, worktrees))
1666
retcode = 1;
1667
}
1668
1669
free_refs(tags_ref_map);
1670
}
1671
1672
+ if (transaction) {
1673
+ if (retcode)
1674
+ goto cleanup;
1675
+
1676
+ retcode = ref_transaction_commit(transaction, &err);
1677
+ if (retcode) {
1678
+ error("%s", err.buf);
1679
+ ref_transaction_free(transaction);
1680
+ transaction = NULL;
1681
+ goto cleanup;
1682
+ }
1683
+ }
1684
+
1685
commit_fetch_head(&fetch_head);
1686
1687
if (set_upstream) {
@@ -1705,7 +1739,13 @@ static int do_fetch(struct transport *transport,
1739
}
1740
1741
cleanup:
1742
+ if (retcode && transaction) {
1743
+ ref_transaction_abort(transaction, &err);
1744
+ error("%s", err.buf);
1745
+ }
1746
+
1747
close_fetch_head(&fetch_head);
1748
+ strbuf_release(&err);
1749
free_refs(ref_map);
1750
free_worktrees(worktrees);
1751
return retcode;
t/t5503-tagfollow.sh
+3
-8
@@ -180,11 +180,8 @@ test_expect_success 'atomic fetch with failing backfill' '
180
EOF
181
182
test_must_fail git -C clone3 fetch --atomic .. $B:refs/heads/something &&
183
-
184
- # Creation of the tag has failed, so ideally refs/heads/something
185
- # should not exist. The fact that it does demonstrates that there is
186
- # a bug in the `--atomic` flag.
187
- test $B = "$(git -C clone3 rev-parse --verify refs/heads/something)"
183
+ test_must_fail git -C clone3 rev-parse --verify refs/heads/something &&
184
+ test_must_fail git -C clone3 rev-parse --verify refs/tags/tag2
185
'
186
187
test_expect_success 'atomic fetch with backfill should use single transaction' '
@@ -197,12 +194,10 @@ test_expect_success 'atomic fetch with backfill should use single transaction' '
194
prepared
195
$ZERO_OID $B refs/heads/something
196
$ZERO_OID $S refs/tags/tag2
197
+ $ZERO_OID $T refs/tags/tag1
198
committed
199
$ZERO_OID $B refs/heads/something
200
$ZERO_OID $S refs/tags/tag2
203
- prepared
204
- $ZERO_OID $T refs/tags/tag1
205
- committed
201
$ZERO_OID $T refs/tags/tag1
202
EOF
203