summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorLibravatar Junio C Hamano <gitster@pobox.com>2020-05-01 13:40:00 -0700
committerLibravatar Junio C Hamano <gitster@pobox.com>2020-05-01 13:40:00 -0700
commit0b07eecf6ed9334f09d6624732a4af2da03e38eb (patch)
treedc9a051dc91ba996bae50b24e6e0169a1d339375
parentMerge branch 'js/anonymise-push-url-in-errors' (diff)
parentfetch-pack: in protocol v2, reset in_vain upon ACK (diff)
downloadtgif-0b07eecf6ed9334f09d6624732a4af2da03e38eb.tar.xz
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()
-rw-r--r--fetch-pack.c50
-rwxr-xr-xt/t5500-fetch-pack.sh48
2 files changed, 86 insertions, 12 deletions
diff --git a/fetch-pack.c b/fetch-pack.c
index 8e98b3d4a5..f73a2ce6cb 100644
--- a/fetch-pack.c
+++ b/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;
}
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index baa1a99f45..52dd1a688c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -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 &&