fetch-pack: unify ref in and out param
When a user fetches: - at least one up-to-date ref and at least one non-up-to-date ref, - using HTTP with protocol v0 (or something else that uses the fetch command of a remote helper) some refs might not be updated after the fetch. This bug was introduced in commit 989b8c4452 ("fetch-pack: put shallow info in output parameter", 2018-06-28) which allowed transports to report the refs that they have fetched in a new out-parameter "fetched_refs". If they do so, transport_fetch_refs() makes this information available to its caller. Users of "fetched_refs" rely on the following 3 properties: (1) it is the complete list of refs that was passed to transport_fetch_refs(), (2) it has shallow information (REF_STATUS_REJECT_SHALLOW set if relevant), and (3) it has updated OIDs if ref-in-want was used (introduced after 989b8c4452). In an effort to satisfy (1), whenever transport_fetch_refs() filters the refs sent to the transport, it re-adds the filtered refs to whatever the transport supplies before returning it to the user. However, the implementation in 989b8c4452 unconditionally re-adds the filtered refs without checking if the transport refrained from reporting anything in "fetched_refs" (which it is allowed to do), resulting in an incomplete list, no longer satisfying (1). An earlier effort to resolve this [1] solved the issue by readding the filtered refs only if the transport did not refrain from reporting in "fetched_refs", but after further discussion, it seems that the better solution is to revert the API change that introduced "fetched_refs". This API change was first suggested as part of a ref-in-want implementation that allowed for ref patterns and, thus, there could be drastic differences between the input refs and the refs actually fetched [2]; we eventually decided to only allow exact ref names, but this API change remained even though its necessity was decreased. Therefore, revert this API change by reverting commit 989b8c4452, and make receive_wanted_refs() update the OIDs in the sought array (like how update_shallow() updates shallow information in the sought array) instead. A test is also included to show that the user-visible bug discussed at the beginning of this commit message no longer exists. [1] https://public-inbox.org/git/20180801171806.GA122458@google.com/ [2] https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathantanmy@google.com/ Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jonathan Tan committed
Aug 1, 2018 at 13:13 UTC
e2842b39f4168e3cd39a53961c1e50bb940eedf1
9 files changed
+50
-84
builtin/clone.c
+2
-2
@@ -1155,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
1155
}
1156
1157
if (!is_local && !complete_refs_before_fetch)
1158
- transport_fetch_refs(transport, mapped_refs, NULL);
1158
+ transport_fetch_refs(transport, mapped_refs);
1159
1160
remote_head = find_ref_by_name(refs, "HEAD");
1161
remote_head_points_at =
@@ -1197,7 +1197,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
1197
if (is_local)
1198
clone_local(path, git_dir);
1199
else if (refs && complete_refs_before_fetch)
1200
- transport_fetch_refs(transport, mapped_refs, NULL);
1200
+ transport_fetch_refs(transport, mapped_refs);
1201
1202
update_remote_refs(refs, mapped_refs, remote_head_points_at,
1203
branch_top.buf, reflog_msg.buf, transport,
builtin/fetch.c
+4
-24
@@ -948,13 +948,11 @@ static int quickfetch(struct ref *ref_map)
948
return check_connected(iterate_ref_map, &rm, &opt);
949
}
950
951
-static int fetch_refs(struct transport *transport, struct ref *ref_map,
952
- struct ref **updated_remote_refs)
951
+static int fetch_refs(struct transport *transport, struct ref *ref_map)
952
{
953
int ret = quickfetch(ref_map);
954
if (ret)
956
- ret = transport_fetch_refs(transport, ref_map,
957
- updated_remote_refs);
955
+ ret = transport_fetch_refs(transport, ref_map);
956
if (!ret)
957
/*
958
* Keep the new pack's ".keep" file around to allow the caller
@@ -1119,7 +1117,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)
1117
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
1118
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
1119
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
1122
- if (!fetch_refs(transport, ref_map, NULL))
1120
+ if (!fetch_refs(transport, ref_map))
1121
consume_refs(transport, ref_map);
1122
1123
if (gsecondary) {
@@ -1135,7 +1133,6 @@ static int do_fetch(struct transport *transport,
1133
int autotags = (transport->remote->fetch_tags == 1);
1134
int retcode = 0;
1135
const struct ref *remote_refs;
1138
- struct ref *updated_remote_refs = NULL;
1136
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
1137
1138
if (tags == TAGS_DEFAULT) {
@@ -1186,24 +1183,7 @@ static int do_fetch(struct transport *transport,
1183
transport->url);
1184
}
1185
}
1189
-
1190
- if (fetch_refs(transport, ref_map, &updated_remote_refs)) {
1191
- free_refs(ref_map);
1192
- retcode = 1;
1193
- goto cleanup;
1194
- }
1195
- if (updated_remote_refs) {
1196
- /*
1197
- * Regenerate ref_map using the updated remote refs. This is
1198
- * to account for additional information which may be provided
1199
- * by the transport (e.g. shallow info).
1200
- */
1201
- free_refs(ref_map);
1202
- ref_map = get_ref_map(transport->remote, updated_remote_refs, rs,
1203
- tags, &autotags);
1204
- free_refs(updated_remote_refs);
1205
- }
1206
- if (consume_refs(transport, ref_map)) {
1186
+ if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) {
1187
free_refs(ref_map);
1188
retcode = 1;
1189
goto cleanup;
fetch-object.c
+1
-1
@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref *ref)
19
20
transport_set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
21
transport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, "1");
22
- transport_fetch_refs(transport, ref, NULL);
22
+ transport_fetch_refs(transport, ref);
23
fetch_if_missing = original_fetch_if_missing;
24
}
25
fetch-pack.c
+15
-15
@@ -1338,25 +1338,26 @@ static void receive_shallow_info(struct fetch_pack_args *args,
1338
args->deepen = 1;
1339
}
1340
1341
-static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
1341
+static void receive_wanted_refs(struct packet_reader *reader,
1342
+ struct ref **sought, int nr_sought)
1343
{
1344
process_section_header(reader, "wanted-refs", 0);
1345
while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
1346
struct object_id oid;
1347
const char *end;
1347
- struct ref *r = NULL;
1348
+ int i;
1349
1350
if (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ')
1351
die("expected wanted-ref, got '%s'", reader->line);
1352
1352
- for (r = refs; r; r = r->next) {
1353
- if (!strcmp(end, r->name)) {
1354
- oidcpy(&r->old_oid, &oid);
1353
+ for (i = 0; i < nr_sought; i++) {
1354
+ if (!strcmp(end, sought[i]->name)) {
1355
+ oidcpy(&sought[i]->old_oid, &oid);
1356
break;
1357
}
1358
}
1359
1359
- if (!r)
1360
+ if (i == nr_sought)
1361
die("unexpected wanted-ref: '%s'", reader->line);
1362
}
1363
@@ -1439,7 +1440,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
1440
receive_shallow_info(args, &reader);
1441
1442
if (process_section_header(&reader, "wanted-refs", 1))
1442
- receive_wanted_refs(&reader, ref);
1443
+ receive_wanted_refs(&reader, sought, nr_sought);
1444
1445
/* get the pack */
1446
process_section_header(&reader, "packfile", 0);
@@ -1503,13 +1504,12 @@ static int remove_duplicates_in_refs(struct ref **ref, int nr)
1504
}
1505
1506
static void update_shallow(struct fetch_pack_args *args,
1506
- struct ref *refs,
1507
+ struct ref **sought, int nr_sought,
1508
struct shallow_info *si)
1509
{
1510
struct oid_array ref = OID_ARRAY_INIT;
1511
int *status;
1512
int i;
1512
- struct ref *r;
1513
1514
if (args->deepen && alternate_shallow_file) {
1515
if (*alternate_shallow_file == '\0') { /* --unshallow */
@@ -1551,8 +1551,8 @@ static void update_shallow(struct fetch_pack_args *args,
1551
remove_nonexistent_theirs_shallow(si);
1552
if (!si->nr_ours && !si->nr_theirs)
1553
return;
1554
- for (r = refs; r; r = r->next)
1555
- oid_array_append(&ref, &r->old_oid);
1554
+ for (i = 0; i < nr_sought; i++)
1555
+ oid_array_append(&ref, &sought[i]->old_oid);
1556
si->ref = &ref;
1557
1558
if (args->update_shallow) {
@@ -1586,12 +1586,12 @@ static void update_shallow(struct fetch_pack_args *args,
1586
* remote is also shallow, check what ref is safe to update
1587
* without updating .git/shallow
1588
*/
1589
- status = xcalloc(ref.nr, sizeof(*status));
1589
+ status = xcalloc(nr_sought, sizeof(*status));
1590
assign_shallow_commits_to_refs(si, NULL, status);
1591
if (si->nr_ours || si->nr_theirs) {
1592
- for (r = refs, i = 0; r; r = r->next, i++)
1592
+ for (i = 0; i < nr_sought; i++)
1593
if (status[i])
1594
- r->status = REF_STATUS_REJECT_SHALLOW;
1594
+ sought[i]->status = REF_STATUS_REJECT_SHALLOW;
1595
}
1596
free(status);
1597
oid_array_clear(&ref);
@@ -1654,7 +1654,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
1654
args->connectivity_checked = 1;
1655
}
1656
1657
- update_shallow(args, ref_cpy, &si);
1657
+ update_shallow(args, sought, nr_sought, &si);
1658
cleanup:
1659
clear_shallow_info(&si);
1660
return ref_cpy;
t/t5551-http-fetch-smart.sh
+18
@@ -369,6 +369,24 @@ test_expect_success 'custom http headers' '
369
submodule update sub
370
'
371
372
+test_expect_success 'using fetch command in remote-curl updates refs' '
373
+ SERVER="$HTTPD_DOCUMENT_ROOT_PATH/twobranch" &&
374
+ rm -rf "$SERVER" client &&
375
+
376
+ git init "$SERVER" &&
377
+ test_commit -C "$SERVER" foo &&
378
+ git -C "$SERVER" update-ref refs/heads/anotherbranch foo &&
379
+
380
+ git clone $HTTPD_URL/smart/twobranch client &&
381
+
382
+ test_commit -C "$SERVER" bar &&
383
+ git -C client -c protocol.version=0 fetch &&
384
+
385
+ git -C "$SERVER" rev-parse master >expect &&
386
+ git -C client rev-parse origin/master >actual &&
387
+ test_cmp expect actual
388
+'
389
+
390
test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '
391
rm -rf clone &&
392
echo "Set-Cookie: Foo=1" >cookies &&
transport-helper.c
+2
-4
@@ -651,16 +651,14 @@ static int connect_helper(struct transport *transport, const char *name,
651
}
652
653
static int fetch(struct transport *transport,
654
- int nr_heads, struct ref **to_fetch,
655
- struct ref **fetched_refs)
654
+ int nr_heads, struct ref **to_fetch)
655
{
656
struct helper_data *data = transport->data;
657
int i, count;
658
659
if (process_connect(transport, 0)) {
660
do_take_over(transport);
662
- return transport->vtable->fetch(transport, nr_heads, to_fetch,
663
- fetched_refs);
661
+ return transport->vtable->fetch(transport, nr_heads, to_fetch);
662
}
663
664
count = 0;
transport-internal.h
+1
-8
@@ -36,18 +36,11 @@ struct transport_vtable {
36
* Fetch the objects for the given refs. Note that this gets
37
* an array, and should ignore the list structure.
38
*
39
- * The transport *may* provide, in fetched_refs, the list of refs that
40
- * it fetched. If the transport knows anything about the fetched refs
41
- * that the caller does not know (for example, shallow status), it
42
- * should provide that list of refs and include that information in the
43
- * list.
44
- *
39
* If the transport did not get hashes for refs in
40
* get_refs_list(), it should set the old_sha1 fields in the
41
* provided refs now.
42
**/
49
- int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs,
50
- struct ref **fetched_refs);
43
+ int (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);
44
45
/**
46
* Push the objects and refs. Send the necessary objects, and
transport.c
+6
-28
@@ -151,8 +151,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
151
}
152
153
static int fetch_refs_from_bundle(struct transport *transport,
154
- int nr_heads, struct ref **to_fetch,
155
- struct ref **fetched_refs)
154
+ int nr_heads, struct ref **to_fetch)
155
{
156
struct bundle_transport_data *data = transport->data;
157
return unbundle(&data->header, data->fd,
@@ -288,8 +287,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus
287
}
288
289
static int fetch_refs_via_pack(struct transport *transport,
291
- int nr_heads, struct ref **to_fetch,
292
- struct ref **fetched_refs)
290
+ int nr_heads, struct ref **to_fetch)
291
{
292
int ret = 0;
293
struct git_transport_data *data = transport->data;
@@ -357,12 +355,8 @@ static int fetch_refs_via_pack(struct transport *transport,
355
if (report_unmatched_refs(to_fetch, nr_heads))
356
ret = -1;
357
360
- if (fetched_refs)
361
- *fetched_refs = refs;
362
- else
363
- free_refs(refs);
364
-
358
free_refs(refs_tmp);
359
+ free_refs(refs);
360
free(dest);
361
return ret;
362
}
@@ -1222,31 +1216,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,
1216
return transport->remote_refs;
1217
}
1218
1225
-int transport_fetch_refs(struct transport *transport, struct ref *refs,
1226
- struct ref **fetched_refs)
1219
+int transport_fetch_refs(struct transport *transport, struct ref *refs)
1220
{
1221
int rc;
1222
int nr_heads = 0, nr_alloc = 0, nr_refs = 0;
1223
struct ref **heads = NULL;
1231
- struct ref *nop_head = NULL, **nop_tail = &nop_head;
1224
struct ref *rm;
1225
1226
for (rm = refs; rm; rm = rm->next) {
1227
nr_refs++;
1228
if (rm->peer_ref &&
1229
!is_null_oid(&rm->old_oid) &&
1238
- !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) {
1239
- /*
1240
- * These need to be reported as fetched, but we don't
1241
- * actually need to fetch them.
1242
- */
1243
- if (fetched_refs) {
1244
- struct ref *nop_ref = copy_ref(rm);
1245
- *nop_tail = nop_ref;
1246
- nop_tail = &nop_ref->next;
1247
- }
1230
+ !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid))
1231
continue;
1249
- }
1232
ALLOC_GROW(heads, nr_heads + 1, nr_alloc);
1233
heads[nr_heads++] = rm;
1234
}
@@ -1264,11 +1246,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs,
1246
heads[nr_heads++] = rm;
1247
}
1248
1267
- rc = transport->vtable->fetch(transport, nr_heads, heads, fetched_refs);
1268
- if (fetched_refs && nop_head) {
1269
- *nop_tail = *fetched_refs;
1270
- *fetched_refs = nop_head;
1271
- }
1249
+ rc = transport->vtable->fetch(transport, nr_heads, heads);
1250
1251
free(heads);
1252
return rc;
transport.h
+1
-2
@@ -229,8 +229,7 @@ int transport_push(struct transport *connection,
229
const struct ref *transport_get_remote_refs(struct transport *transport,
230
const struct argv_array *ref_prefixes);
231
232
-int transport_fetch_refs(struct transport *transport, struct ref *refs,
233
- struct ref **fetched_refs);
232
+int transport_fetch_refs(struct transport *transport, struct ref *refs);
233
void transport_unlock_pack(struct transport *transport);
234
int transport_disconnect(struct transport *transport);
235
char *transport_anonymize_url(const char *url);