fetch-pack: be more precise in parsing v2 response

Each section in a protocol v2 response is followed by either a DELIM packet (indicating more sections to follow) or a FLUSH packet (indicating none to follow). But when parsing the "acknowledgments" section, do_fetch_pack_v2() is liberal in accepting both, but determines whether to continue reading or not based solely on the contents of the "acknowledgments" section, not on whether DELIM or FLUSH was read. There is no issue with a protocol-compliant server, but can result in confusing error messages when communicating with a server that serves unexpected additional sections. Consider a server that sends "new-section" after "acknowledgments": - client writes request - client reads the "acknowledgments" section which contains no "ready", then DELIM - since there was no "ready", client needs to continue negotiation, and writes request - client reads "new-section", and reports to the end user "expected 'acknowledgments', received 'new-section'" For the person debugging the involved Git implementation(s), the error message is confusing in that "new-section" was not received in response to the latest request, but to the first one. One solution is to always continue reading after DELIM, but in this case, we can do better. We know from the protocol that "ready" means at least the packfile section is coming (hence, DELIM) and that no "ready" means that no sections are to follow (hence, FLUSH). So teach process_acks() to enforce this. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jonathan Tan committed Oct 19, 2018 at 15:54 UTC 5400b2a2d989a4f09e7f666a41efac9ee3936f10
2 files changed +62
fetch-pack.c
+12
@@ -1248,6 +1248,18 @@ static int process_acks(struct fetch_negotiator *negotiator,
1248 reader->status != PACKET_READ_DELIM)
1249 die(_("error processing acks: %d"), reader->status);
1250
1251 + /*
1252 + * If an "acknowledgments" section is sent, a packfile is sent if and
1253 + * only if "ready" was sent in this section. The other sections
1254 + * ("shallow-info" and "wanted-refs") are sent only if a packfile is
1255 + * sent. Therefore, a DELIM is expected if "ready" is sent, and a FLUSH
1256 + * otherwise.
1257 + */
1258 + if (received_ready && reader->status != PACKET_READ_DELIM)
1259 + die(_("expected packfile to be sent after 'ready'"));
1260 + if (!received_ready && reader->status != PACKET_READ_FLUSH)
1261 + die(_("expected no other sections to be sent after no 'ready'"));
1262 +
1263 /* return 0 if no common, 1 if there are common, or 2 if ready */
1264 return received_ready ? 2 : (received_ack ? 1 : 0);
1265 }
t/t5702-protocol-v2.sh
+50
@@ -512,6 +512,56 @@ test_expect_success 'push with http:// and a config of v2 does not request v2' '
512 ! grep "git< version 2" log
513 '
514
515 +test_expect_success 'when server sends "ready", expect DELIM' '
516 + rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child &&
517 +
518 + git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
519 + test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
520 +
521 + git clone "$HTTPD_URL/smart/http_parent" http_child &&
522 +
523 + test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" two &&
524 +
525 + # After "ready" in the acknowledgments section, pretend that a FLUSH
526 + # (0000) was sent instead of a DELIM (0001).
527 + printf "/ready/,$ s/0001/0000/" \
528 + >"$HTTPD_ROOT_PATH/one-time-sed" &&
529 +
530 + test_must_fail git -C http_child -c protocol.version=2 \
531 + fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
532 + test_i18ngrep "expected packfile to be sent after .ready." err
533 +'
534 +
535 +test_expect_success 'when server does not send "ready", expect FLUSH' '
536 + rm -rf "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" http_child log &&
537 +
538 + git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
539 + test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" one &&
540 +
541 + git clone "$HTTPD_URL/smart/http_parent" http_child &&
542 +
543 + test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" two &&
544 +
545 + # Create many commits to extend the negotiation phase across multiple
546 + # requests, so that the server does not send "ready" in the first
547 + # request.
548 + for i in $(test_seq 1 32)
549 + do
550 + test_commit -C http_child c$i
551 + done &&
552 +
553 + # After the acknowledgments section, pretend that a DELIM
554 + # (0001) was sent instead of a FLUSH (0000).
555 + printf "/acknowledgments/,$ s/0000/0001/" \
556 + >"$HTTPD_ROOT_PATH/one-time-sed" &&
557 +
558 + test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" git -C http_child \
559 + -c protocol.version=2 \
560 + fetch "$HTTPD_URL/one_time_sed/http_parent" 2> err &&
561 + grep "fetch< acknowledgments" log &&
562 + ! grep "fetch< ready" log &&
563 + test_i18ngrep "expected no other sections to be sent after no .ready." err
564 +'
565
566 stop_httpd
567