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()
This commit is contained in:
Junio C Hamano
commit
0b07eecf6e
50
fetch-pack.c
50
fetch-pack.c
@ -1143,6 +1143,7 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)
|
||||
}
|
||||
|
||||
static int add_haves(struct fetch_negotiator *negotiator,
|
||||
int seen_ack,
|
||||
struct strbuf *req_buf,
|
||||
int *haves_to_send, int *in_vain)
|
||||
{
|
||||
@ -1157,7 +1158,7 @@ static int add_haves(struct fetch_negotiator *negotiator,
|
||||
}
|
||||
|
||||
*in_vain += haves_added;
|
||||
if (!haves_added || *in_vain >= MAX_IN_VAIN) {
|
||||
if (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) {
|
||||
/* Send Done */
|
||||
packet_buf_write(req_buf, "done\n");
|
||||
ret = 1;
|
||||
@ -1173,7 +1174,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
|
||||
struct fetch_pack_args *args,
|
||||
const struct ref *wants, struct oidset *common,
|
||||
int *haves_to_send, int *in_vain,
|
||||
int sideband_all)
|
||||
int sideband_all, int seen_ack)
|
||||
{
|
||||
int ret = 0;
|
||||
struct strbuf req_buf = STRBUF_INIT;
|
||||
@ -1230,7 +1231,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
|
||||
add_common(&req_buf, common);
|
||||
|
||||
/* Add initial haves */
|
||||
ret = add_haves(negotiator, &req_buf, haves_to_send, in_vain);
|
||||
ret = add_haves(negotiator, seen_ack, &req_buf,
|
||||
haves_to_send, in_vain);
|
||||
}
|
||||
|
||||
/* Send request */
|
||||
@ -1268,9 +1270,29 @@ static int process_section_header(struct packet_reader *reader,
|
||||
return ret;
|
||||
}
|
||||
|
||||
static int process_acks(struct fetch_negotiator *negotiator,
|
||||
struct packet_reader *reader,
|
||||
struct oidset *common)
|
||||
enum common_found {
|
||||
/*
|
||||
* No commit was found to be possessed by both the client and the
|
||||
* server, and "ready" was not received.
|
||||
*/
|
||||
NO_COMMON_FOUND,
|
||||
|
||||
/*
|
||||
* At least one commit was found to be possessed by both the client and
|
||||
* the server, and "ready" was not received.
|
||||
*/
|
||||
COMMON_FOUND,
|
||||
|
||||
/*
|
||||
* "ready" was received, indicating that the server is ready to send
|
||||
* the packfile without any further negotiation.
|
||||
*/
|
||||
READY
|
||||
};
|
||||
|
||||
static enum common_found process_acks(struct fetch_negotiator *negotiator,
|
||||
struct packet_reader *reader,
|
||||
struct oidset *common)
|
||||
{
|
||||
/* received */
|
||||
int received_ready = 0;
|
||||
@ -1285,6 +1307,7 @@ static int process_acks(struct fetch_negotiator *negotiator,
|
||||
|
||||
if (skip_prefix(reader->line, "ACK ", &arg)) {
|
||||
struct object_id oid;
|
||||
received_ack = 1;
|
||||
if (!get_oid_hex(arg, &oid)) {
|
||||
struct commit *commit;
|
||||
oidset_insert(common, &oid);
|
||||
@ -1319,8 +1342,8 @@ static int process_acks(struct fetch_negotiator *negotiator,
|
||||
if (!received_ready && reader->status != PACKET_READ_FLUSH)
|
||||
die(_("expected no other sections to be sent after no 'ready'"));
|
||||
|
||||
/* return 0 if no common, 1 if there are common, or 2 if ready */
|
||||
return received_ready ? 2 : (received_ack ? 1 : 0);
|
||||
return received_ready ? READY :
|
||||
(received_ack ? COMMON_FOUND : NO_COMMON_FOUND);
|
||||
}
|
||||
|
||||
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,
|
||||
int haves_to_send = INITIAL_FLUSH;
|
||||
struct fetch_negotiator negotiator_alloc;
|
||||
struct fetch_negotiator *negotiator;
|
||||
int seen_ack = 0;
|
||||
|
||||
if (args->no_dependents) {
|
||||
negotiator = NULL;
|
||||
@ -1500,7 +1524,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
|
||||
if (send_fetch_request(negotiator, fd[1], args, ref,
|
||||
&common,
|
||||
&haves_to_send, &in_vain,
|
||||
reader.use_sideband))
|
||||
reader.use_sideband,
|
||||
seen_ack))
|
||||
state = FETCH_GET_PACK;
|
||||
else
|
||||
state = FETCH_PROCESS_ACKS;
|
||||
@ -1508,13 +1533,14 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
|
||||
case FETCH_PROCESS_ACKS:
|
||||
/* Process ACKs/NAKs */
|
||||
switch (process_acks(negotiator, &reader, &common)) {
|
||||
case 2:
|
||||
case READY:
|
||||
state = FETCH_GET_PACK;
|
||||
break;
|
||||
case 1:
|
||||
case COMMON_FOUND:
|
||||
in_vain = 0;
|
||||
seen_ack = 1;
|
||||
/* fallthrough */
|
||||
default:
|
||||
case NO_COMMON_FOUND:
|
||||
state = FETCH_SEND_REQUEST;
|
||||
break;
|
||||
}
|
||||
|
||||
@ -385,6 +385,54 @@ test_expect_success 'clone shallow with packed refs' '
|
||||
test_cmp count8.expected count8.actual
|
||||
'
|
||||
|
||||
test_expect_success 'in_vain not triggered before first ACK' '
|
||||
rm -rf myserver myclient trace &&
|
||||
git init myserver &&
|
||||
test_commit -C myserver foo &&
|
||||
git clone "file://$(pwd)/myserver" myclient &&
|
||||
|
||||
# MAX_IN_VAIN is 256. Because of batching, the client will send 496
|
||||
# (16+32+64+128+256) commits, not 256, before giving up. So create 496
|
||||
# irrelevant commits.
|
||||
test_commit_bulk -C myclient 496 &&
|
||||
|
||||
# The new commit that the client wants to fetch.
|
||||
test_commit -C myserver bar &&
|
||||
|
||||
GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin &&
|
||||
test_i18ngrep "Total 3 " trace
|
||||
'
|
||||
|
||||
test_expect_success 'in_vain resetted upon ACK' '
|
||||
rm -rf myserver myclient trace &&
|
||||
git init myserver &&
|
||||
|
||||
# Linked list of commits on master. The first is common; the rest are
|
||||
# not.
|
||||
test_commit -C myserver first_master_commit &&
|
||||
git clone "file://$(pwd)/myserver" myclient &&
|
||||
test_commit_bulk -C myclient 255 &&
|
||||
|
||||
# Another linked list of commits on anotherbranch with no connection to
|
||||
# master. The first is common; the rest are not.
|
||||
git -C myserver checkout --orphan anotherbranch &&
|
||||
test_commit -C myserver first_anotherbranch_commit &&
|
||||
git -C myclient fetch origin anotherbranch:refs/heads/anotherbranch &&
|
||||
git -C myclient checkout anotherbranch &&
|
||||
test_commit_bulk -C myclient 255 &&
|
||||
|
||||
# The new commit that the client wants to fetch.
|
||||
git -C myserver checkout master &&
|
||||
test_commit -C myserver to_fetch &&
|
||||
|
||||
# The client will send (as "have"s) all 256 commits in anotherbranch
|
||||
# first. The 256th commit is common between the client and the server,
|
||||
# and should reset in_vain. This allows negotiation to continue until
|
||||
# the client reports that first_anotherbranch_commit is common.
|
||||
GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master &&
|
||||
test_i18ngrep "Total 3 " trace
|
||||
'
|
||||
|
||||
test_expect_success 'fetch in shallow repo unreachable shallow objects' '
|
||||
(
|
||||
git clone --bare --branch B --single-branch "file://$(pwd)/." no-reflog &&
|
||||
|
||||
Loading…
Reference in New Issue
Block a user