Skip to content

Commit 0b07eec

Browse files
gitstergitster
committed
Merge branch 'jt/v2-fetch-nego-fix'
The upload-pack protocol v2 gave up too early before finding a common ancestor, resulting in a wasteful fetch from a fork of a project. This has been corrected to match the behaviour of v0 protocol. * jt/v2-fetch-nego-fix: fetch-pack: in protocol v2, reset in_vain upon ACK fetch-pack: in protocol v2, in_vain only after ACK fetch-pack: return enum from process_acks()
2 parents 2c42fb7 + 2f0a093 commit 0b07eec

File tree

2 files changed

+86
-12
lines changed

2 files changed

+86
-12
lines changed

‎fetch-pack.c‎

Lines changed: 38 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,6 +1143,7 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)
11431143
}
11441144

11451145
static int add_haves(struct fetch_negotiator *negotiator,
1146+
int seen_ack,
11461147
struct strbuf *req_buf,
11471148
int *haves_to_send, int *in_vain)
11481149
{
@@ -1157,7 +1158,7 @@ static int add_haves(struct fetch_negotiator *negotiator,
11571158
}
11581159

11591160
*in_vain += haves_added;
1160-
if (!haves_added || *in_vain >= MAX_IN_VAIN) {
1161+
if (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {
11611162
/* Send Done */
11621163
packet_buf_write(req_buf, "done\n");
11631164
ret = 1;
@@ -1173,7 +1174,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
11731174
struct fetch_pack_args *args,
11741175
const struct ref *wants, struct oidset *common,
11751176
int *haves_to_send, int *in_vain,
1176-
int sideband_all)
1177+
int sideband_all, int seen_ack)
11771178
{
11781179
int ret = 0;
11791180
struct strbuf req_buf = STRBUF_INIT;
@@ -1230,7 +1231,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
12301231
add_common(&req_buf, common);
12311232

12321233
/* Add initial haves */
1233-
ret = add_haves(negotiator, &req_buf, haves_to_send, in_vain);
1234+
ret = add_haves(negotiator, seen_ack, &req_buf,
1235+
haves_to_send, in_vain);
12341236
}
12351237

12361238
/* Send request */
@@ -1268,9 +1270,29 @@ static int process_section_header(struct packet_reader *reader,
12681270
return ret;
12691271
}
12701272

1271-
static int process_acks(struct fetch_negotiator *negotiator,
1272-
struct packet_reader *reader,
1273-
struct oidset *common)
1273+
enum common_found {
1274+
/*
1275+
* No commit was found to be possessed by both the client and the
1276+
* server, and "ready" was not received.
1277+
*/
1278+
NO_COMMON_FOUND,
1279+
1280+
/*
1281+
* At least one commit was found to be possessed by both the client and
1282+
* the server, and "ready" was not received.
1283+
*/
1284+
COMMON_FOUND,
1285+
1286+
/*
1287+
* "ready" was received, indicating that the server is ready to send
1288+
* the packfile without any further negotiation.
1289+
*/
1290+
READY
1291+
};
1292+
1293+
static enum common_found process_acks(struct fetch_negotiator *negotiator,
1294+
struct packet_reader *reader,
1295+
struct oidset *common)
12741296
{
12751297
/* received */
12761298
int received_ready = 0;
@@ -1285,6 +1307,7 @@ static int process_acks(struct fetch_negotiator *negotiator,
12851307

12861308
if (skip_prefix(reader->line, "ACK ", &arg)) {
12871309
struct object_id oid;
1310+
received_ack = 1;
12881311
if (!get_oid_hex(arg, &oid)) {
12891312
struct commit *commit;
12901313
oidset_insert(common, &oid);
@@ -1319,8 +1342,8 @@ static int process_acks(struct fetch_negotiator *negotiator,
13191342
if (!received_ready && reader->status != PACKET_READ_FLUSH)
13201343
die(_("expected no other sections to be sent after no 'ready'"));
13211344

1322-
/* return 0 if no common, 1 if there are common, or 2 if ready */
1323-
return received_ready ? 2 : (received_ack ? 1 : 0);
1345+
return received_ready ? READY :
1346+
(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);
13241347
}
13251348

13261349
static void receive_shallow_info(struct fetch_pack_args *args,
@@ -1444,6 +1467,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
14441467
int haves_to_send = INITIAL_FLUSH;
14451468
struct fetch_negotiator negotiator_alloc;
14461469
struct fetch_negotiator *negotiator;
1470+
int seen_ack = 0;
14471471

14481472
if (args->no_dependents) {
14491473
negotiator = NULL;
@@ -1500,21 +1524,23 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
15001524
if (send_fetch_request(negotiator, fd[1], args, ref,
15011525
&common,
15021526
&haves_to_send, &in_vain,
1503-
reader.use_sideband))
1527+
reader.use_sideband,
1528+
seen_ack))
15041529
state = FETCH_GET_PACK;
15051530
else
15061531
state = FETCH_PROCESS_ACKS;
15071532
break;
15081533
case FETCH_PROCESS_ACKS:
15091534
/* Process ACKs/NAKs */
15101535
switch (process_acks(negotiator, &reader, &common)) {
1511-
case 2:
1536+
case READY:
15121537
state = FETCH_GET_PACK;
15131538
break;
1514-
case 1:
1539+
case COMMON_FOUND:
15151540
in_vain = 0;
1541+
seen_ack = 1;
15161542
/* fallthrough */
1517-
default:
1543+
case NO_COMMON_FOUND:
15181544
state = FETCH_SEND_REQUEST;
15191545
break;
15201546
}

‎t/t5500-fetch-pack.sh‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,54 @@ test_expect_success 'clone shallow with packed refs' '
385385
test_cmp count8.expected count8.actual
386386
'
387387

388+
test_expect_success 'in_vain not triggered before first ACK' '
389+
rm -rf myserver myclient trace &&
390+
git init myserver &&
391+
test_commit -C myserver foo &&
392+
git clone "file://$(pwd)/myserver" myclient &&
393+
394+
# MAX_IN_VAIN is 256. Because of batching, the client will send 496
395+
# (16+32+64+128+256) commits, not 256, before giving up. So create 496
396+
# irrelevant commits.
397+
test_commit_bulk -C myclient 496 &&
398+
399+
# The new commit that the client wants to fetch.
400+
test_commit -C myserver bar &&
401+
402+
GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin &&
403+
test_i18ngrep "Total 3 " trace
404+
'
405+
406+
test_expect_success 'in_vain resetted upon ACK' '
407+
rm -rf myserver myclient trace &&
408+
git init myserver &&
409+
410+
# Linked list of commits on master. The first is common; the rest are
411+
# not.
412+
test_commit -C myserver first_master_commit &&
413+
git clone "file://$(pwd)/myserver" myclient &&
414+
test_commit_bulk -C myclient 255 &&
415+
416+
# Another linked list of commits on anotherbranch with no connection to
417+
# master. The first is common; the rest are not.
418+
git -C myserver checkout --orphan anotherbranch &&
419+
test_commit -C myserver first_anotherbranch_commit &&
420+
git -C myclient fetch origin anotherbranch:refs/heads/anotherbranch &&
421+
git -C myclient checkout anotherbranch &&
422+
test_commit_bulk -C myclient 255 &&
423+
424+
# The new commit that the client wants to fetch.
425+
git -C myserver checkout master &&
426+
test_commit -C myserver to_fetch &&
427+
428+
# The client will send (as "have"s) all 256 commits in anotherbranch
429+
# first. The 256th commit is common between the client and the server,
430+
# and should reset in_vain. This allows negotiation to continue until
431+
# the client reports that first_anotherbranch_commit is common.
432+
GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master &&
433+
test_i18ngrep "Total 3 " trace
434+
'
435+
388436
test_expect_success 'fetch in shallow repo unreachable shallow objects' '
389437
(
390438
git clone --bare --branch B --single-branch "file://$(pwd)/." no-reflog &&

0 commit comments

Comments
 (0)