| Message ID | 4696c8e901808853d17af10d5a6d95cd4711c6d5.1617929278.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Push negotiation | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > There are 2 code paths that do not go through fetch_refs_via_pack() that > needed to be individually excluded: the bundle transport (excluded > through requiring smart_options, which the bundle transport doesn't > support) and transport helpers that do not support takeover. > Fortunately, none of these support protocol v2. I am a bit puzzled by this mention of "Fortunately". If one says "this shiny new feature only works with protocol v2" and "transport X does not support protocol v2", doesn't it imply that the shiny new feature cannot be used with the transport X, which is unfortunate? I can understand "while interacting with the bundle transport, you cannot do independent negotiation, but there is nothing to negotiate with a static file that is a bundle anyway, so nothing is lost" as an explanation, though. > Documentation/technical/protocol-v2.txt | 8 +++ > builtin/fetch.c | 27 +++++++- > fetch-pack.c | 89 +++++++++++++++++++++++-- > fetch-pack.h | 11 +++ > object.h | 2 +- > t/t5701-git-serve.sh | 2 +- > t/t5702-protocol-v2.sh | 89 +++++++++++++++++++++++++ > transport-helper.c | 10 +++ > transport.c | 30 +++++++-- > transport.h | 6 ++ > upload-pack.c | 18 +++-- > 11 files changed, 275 insertions(+), 17 deletions(-) It is a bit surprising that there isn't much code removed, as I expected that we'd be factoring out and reusing existing code used in negotiation for fetching into a new helper function (hence the existing codepath would lose a lot of code to be replaced by a call to a new helper function), but that is apparently not what is going on. I'll have to revisit this step and the next step tomorrow. Thanks.
> Jonathan Tan <jonathantanmy@google.com> writes: > > > There are 2 code paths that do not go through fetch_refs_via_pack() that > > needed to be individually excluded: the bundle transport (excluded > > through requiring smart_options, which the bundle transport doesn't > > support) and transport helpers that do not support takeover. > > Fortunately, none of these support protocol v2. > > I am a bit puzzled by this mention of "Fortunately". If one says > "this shiny new feature only works with protocol v2" and "transport > X does not support protocol v2", doesn't it imply that the shiny new > feature cannot be used with the transport X, which is unfortunate? I meant to say that we don't have to do much about these cases because they are out of scope for the support that we planned (protocol v2), but perhaps "fortunately" is the wrong way to say it. Perhaps a better way of explaining it is that most code paths go through fetch_refs_via_pack(), and for the ones that don't (bundle transport and non-takeover transport helpers), we need to address them separately. But in this case, we are not planning to support either, so we just check if we have requested independent negotiation and if yes, report failure. > I can understand "while interacting with the bundle transport, you > cannot do independent negotiation, but there is nothing to negotiate > with a static file that is a bundle anyway, so nothing is lost" as > an explanation, though. > > > Documentation/technical/protocol-v2.txt | 8 +++ > > builtin/fetch.c | 27 +++++++- > > fetch-pack.c | 89 +++++++++++++++++++++++-- > > fetch-pack.h | 11 +++ > > object.h | 2 +- > > t/t5701-git-serve.sh | 2 +- > > t/t5702-protocol-v2.sh | 89 +++++++++++++++++++++++++ > > transport-helper.c | 10 +++ > > transport.c | 30 +++++++-- > > transport.h | 6 ++ > > upload-pack.c | 18 +++-- > > 11 files changed, 275 insertions(+), 17 deletions(-) > > It is a bit surprising that there isn't much code removed, as I > expected that we'd be factoring out and reusing existing code used > in negotiation for fetching into a new helper function (hence the > existing codepath would lose a lot of code to be replaced by a call > to a new helper function), but that is apparently not what is going > on. That's what I was trying to do (hence the earlier patches), but the main thing I ran into is that a lot of the fetch code assumes that you're fetching at least one ref, so I couldn't use a lot of it without more code updating. Having said that, I may have missed more opportunities for reuse. > I'll have to revisit this step and the next step tomorrow. > > Thanks. Thanks for taking a look.
On 4/8/21 9:10 PM, Jonathan Tan wrote: > Currently, the packfile negotiation step within a Git fetch cannot be > done independent of sending the packfile, even though there is at least > one application wherein this is useful. Therefore, make it possible for > this negotiation step to be done independently. A subsequent commit will > use this for one such application - push negotiation. ... > --- a/Documentation/technical/protocol-v2.txt > +++ b/Documentation/technical/protocol-v2.txt > @@ -346,6 +346,14 @@ explained below. > client should download from all given URIs. Currently, the > protocols supported are "http" and "https". > > +If the 'wait-for-done' feature is advertised, the following argument > +can be included in the client's request. > + > + wait-for-done > + Indicates to the server that it should never send "ready", but > + should wait for the client to say "done" before sending the > + packfile. > + Ok, this is a good way to handle the compatibility case. I'll check in the next patch how this feature is checked before sending a pack-file in 'git push'. > @@ -202,6 +203,8 @@ static struct option builtin_fetch_options[] = { > TRANSPORT_FAMILY_IPV6), > OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"), > N_("report that we have only objects reachable from this object")), > + OPT_BOOL(0, "negotiate-only", &negotiate_only, > + N_("do not fetch a packfile; instead, print ancestors of negotiation tips")), It seems a little strange for this step, which is mostly used for tests, to be so visible. I think it would be too much work to create a test helper with an equivalent implementation, so maybe describe this option as only for tests in Documentation/git-fetch.txt. > - if (remote) { > + if (negotiate_only) { > + struct oidset acked_commits = OIDSET_INIT; > + struct oidset_iter iter; > + const struct object_id *oid; > + > + if (!remote) > + die(_("Must supply remote when using --negotiate-only")); Please use lowercase letters to start your error messages. > + gtransport = prepare_transport(remote, 1); > + if (gtransport->smart_options) { > + gtransport->smart_options->acked_commits = &acked_commits; > + } else { > + warning(_("Protocol does not support --negotiate-only, exiting.")); > + return 1; > + } > + if (server_options.nr) > + gtransport->server_options = &server_options; > + result = transport_fetch_refs(gtransport, NULL); > + > + oidset_iter_init(&acked_commits, &iter); > + while ((oid = oidset_iter_next(&iter))) > + printf("%s\n", oid_to_hex(oid)); > + oidset_clear(&acked_commits); > + } else if (remote) { ... > +static int add_to_object_array(const struct object_id *oid, void *data) > +{ > + struct object_array *a = data; > + > + add_object_array(parse_object(the_repository, oid), "", a); > + return 0; > +} Why parse_object() over lookup_object()? > +void negotiate_using_fetch(const struct oid_array *negotiation_tips, > + const struct string_list *server_options, > + int stateless_rpc, > + int fd[], > + struct oidset *acked_commits) It might be worth a careful doc comment to say that we _must_ have already verified the wait-for-done capability. > +{ > + struct fetch_negotiator negotiator; > + struct packet_reader reader; > + struct object_array nt_object_array = OBJECT_ARRAY_INIT; > + struct strbuf req_buf = STRBUF_INIT; > + int haves_to_send = INITIAL_FLUSH; > + int in_vain = 0; > + int seen_ack = 0; > + int last_iteration = 0; As will become clear later, create: timestamp_t min_generation = GENERATION_NUMBER_INFINITY; > + > + fetch_negotiator_init(the_repository, &negotiator); > + mark_tips(&negotiator, negotiation_tips); > + > + packet_reader_init(&reader, fd[0], NULL, 0, > + PACKET_READ_CHOMP_NEWLINE | > + PACKET_READ_DIE_ON_ERR_PACKET); > + > + oid_array_for_each((struct oid_array *) negotiation_tips, > + add_to_object_array, > + &nt_object_array); > + > + while (!last_iteration) { > + int haves_added; > + struct object_id common_oid; > + int received_ready = 0; > + > + strbuf_reset(&req_buf); > + write_fetch_command_and_capabilities(&req_buf, server_options); > + > + packet_buf_write(&req_buf, "wait-for-done"); > + > + haves_added = add_haves(&negotiator, &req_buf, &haves_to_send); > + in_vain += haves_added; > + if (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN)) > + last_iteration = 1; > + > + /* Send request */ > + packet_buf_flush(&req_buf); > + if (write_in_full(fd[1], req_buf.buf, req_buf.len) < 0) > + die_errno(_("unable to write request to remote")); > + > + /* Process ACKs/NAKs */ > + process_section_header(&reader, "acknowledgments", 0); > + while (process_ack(&negotiator, &reader, &common_oid, > + &received_ready)) { > + struct commit *commit = lookup_commit(the_repository, > + &common_oid); > + if (commit) > + commit->object.flags |= COMMON; Making note of this portion of the code: you can update a min_generation value here, decreasing it if we see a smaller generation. > + in_vain = 0; > + seen_ack = 1; > + oidset_insert(acked_commits, &common_oid); > + } > + if (received_ready) > + die(_("unexpected 'ready' from remote")); > + else > + do_check_stateless_delimiter(stateless_rpc, &reader); > + if (can_all_from_reach_with_flag(&nt_object_array, COMMON, > + REACH_SCRATCH, 0, > + GENERATION_NUMBER_ZERO)) And finally here you can use min_generation. Otherwise, you will suffer performance problems when there is a deep history but this returns false. As long as min_generation is the minimum generation of a commit with the COMMON flag, you will be correct to use that limit. > + last_iteration = 1; > + } > + strbuf_release(&req_buf); Don't forget to clear the COMMON flag before returning. > +} > + > +/* > + * Execute the --negotiate-only mode of "git fetch", adding all known common > + * commits to acked_commits. > + */> +void negotiate_using_fetch(const struct oid_array *negotiation_tips, > + const struct string_list *server_options, > + int stateless_rpc, > + int fd[], > + struct oidset *acked_commits); Here is a good place to specifically mention the wait-for-done capability, or else this method will die() when seeing the "ready" from the server. > +setup_negotiate_only () { > + SERVER="$1" > + URI="$2" > + > + rm -rf "$SERVER" client > + > + git init "$SERVER" > + test_commit -C "$SERVER" one > + test_commit -C "$SERVER" two > + > + git clone "$URI" client > + test_commit -C client three > +} > + > +test_expect_success 'file:// --negotiate-only' ' > + SERVER="server" && > + URI="file://$(pwd)/server" && > + > + setup_negotiate_only "$SERVER" "$URI" && > + > + git -C client fetch \ > + --no-tags \ > + --negotiate-only \ > + --negotiation-tip=$(git -C client rev-parse HEAD) \ > + origin >out && > + COMMON=$(git -C "$SERVER" rev-parse two) && > + grep "$COMMON" out > +' > + > +test_expect_success 'file:// --negotiate-only with protocol v0' ' > + SERVER="server" && > + URI="file://$(pwd)/server" && > + > + setup_negotiate_only "$SERVER" "$URI" && > + > + test_must_fail git -c protocol.version=0 -C client fetch \ > + --no-tags \ > + --negotiate-only \ > + --negotiation-tip=$(git -C client rev-parse HEAD) \ > + origin 2>err && > + test_i18ngrep "negotiate-only requires protocol v2" err > +' > + > # Test protocol v2 with 'http://' transport > # > . "$TEST_DIRECTORY"/lib-httpd.sh > @@ -1035,6 +1078,52 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul > test_i18ngrep "disallowed submodule name" err > ' > > +test_expect_success 'http:// --negotiate-only' ' > + SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" && > + URI="$HTTPD_URL/smart/server" && > + > + setup_negotiate_only "$SERVER" "$URI" && > + > + git -C client fetch \ > + --no-tags \ > + --negotiate-only \ > + --negotiation-tip=$(git -C client rev-parse HEAD) \ > + origin >out && > + COMMON=$(git -C "$SERVER" rev-parse two) && > + grep "$COMMON" out > +' > + > +test_expect_success 'http:// --negotiate-only without wait-for-done support' ' > + SERVER="server" && > + URI="$HTTPD_URL/one_time_perl/server" && > + > + setup_negotiate_only "$SERVER" "$URI" && > + > + echo "s/ wait-for-done/ xxxx-xxx-xxxx/" \ > + >"$HTTPD_ROOT_PATH/one-time-perl" && > + > + test_must_fail git -C client fetch \ > + --no-tags \ > + --negotiate-only \ > + --negotiation-tip=$(git -C client rev-parse HEAD) \ > + origin 2>err && > + test_i18ngrep "server does not support wait-for-done" err > +' > + > +test_expect_success 'http:// --negotiate-only with protocol v0' ' > + SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" && > + URI="$HTTPD_URL/smart/server" && > + > + setup_negotiate_only "$SERVER" "$URI" && > + > + test_must_fail git -c protocol.version=0 -C client fetch \ > + --no-tags \ > + --negotiate-only \ > + --negotiation-tip=$(git -C client rev-parse HEAD) \ > + origin 2>err && > + test_i18ngrep "negotiate-only requires protocol v2" err > +' > + > # DO NOT add non-httpd-specific tests here, because the last part of this > # test script is only executed when httpd is available and enabled. > > diff --git a/transport-helper.c b/transport-helper.c > index 4cd76366fa..4be035edb8 100644 > --- a/transport-helper.c > +++ b/transport-helper.c > @@ -684,6 +684,16 @@ static int fetch(struct transport *transport, > return transport->vtable->fetch(transport, nr_heads, to_fetch); > } > > + /* > + * If we reach here, then the server, the client, and/or the transport > + * helper does not support protocol v2. --negotiate-only requires > + * protocol v2. > + */ > + if (data->transport_options.acked_commits) { > + warning(_("--negotiate-only requires protocol v2")); > + return -1; > + } > + This method continues to do a lot that doesn't seem specific to --negotiate-only. The warning message seems incorrect to me, but is also seems like this would break several cases when using protocol v0. It is equally possible that I'm misunderstanding what is going on here. > + if (data->options.acked_commits) { > + if (data->version < protocol_v2) { > + warning(_("--negotiate-only requires protocol v2")); > + ret = -1; > + } else if (!server_supports_feature("fetch", "wait-for-done", 0)) { > + warning(_("server does not support wait-for-done")); > + ret = -1; > + } else { > + negotiate_using_fetch(data->options.negotiation_tips, > + transport->server_options, > + transport->stateless_rpc, > + data->fd, > + data->options.acked_commits); > + ret = 0; > + } > + goto cleanup; > + } These prereqs look right. This makes it look rather strange that negotiate_using_fetch() returns void (and has several die() calls inside) and could instead return an 'int' that is consumed here (and use error() calls instead of die()). > refs = fetch_pack(&args, data->fd, > refs_tmp ? refs_tmp : transport->remote_refs, > to_fetch, nr_heads, &data->shallow, > &transport->pack_lockfiles, data->version); > > - close(data->fd[0]); > - close(data->fd[1]); > - if (finish_connect(data->conn)) > - ret = -1; > - data->conn = NULL; ... > +cleanup: > + close(data->fd[0]); > + close(data->fd[1]); > + if (finish_connect(data->conn)) > + ret = -1; > + data->conn = NULL; > + Good to keep these after skipping a big chunk of the fetch logic. > + > + /* > + * If set, whenever transport_fetch_refs() is called, add known common > + * commits to this oidset instead of fetching any packfiles. > + */ > + struct oidset *acked_commits; nit: s/If set/If allocated/ > @@ -1668,10 +1673,13 @@ int upload_pack_v2(struct repository *r, struct strvec *keys, > case FETCH_PROCESS_ARGS: > process_args(request, &data); > > - if (!data.want_obj.nr) { > + if (!data.want_obj.nr && !data.wait_for_done) { > /* > - * Request didn't contain any 'want' lines, > - * guess they didn't want anything. > + * Request didn't contain any 'want' lines (and > + * the request does not contain > + * "wait-for-done", in which it is reasonable > + * to just send 'have's without 'want's); guess > + * they didn't want anything. > */ > state = FETCH_DONE; > } else if (data.haves.nr) { > @@ -1723,7 +1731,7 @@ int upload_pack_advertise(struct repository *r, > int allow_sideband_all_value; > char *str = NULL; > > - strbuf_addstr(value, "shallow"); > + strbuf_addstr(value, "shallow wait-for-done"); It might be nice to have this wait-for-done capability configurable. I'd likely want this on by default, but in case there is an issue it is easier to change config values than ship new binaries. This is a pretty big patch. It might have been nice to split the server-side stuff out as its own patch with a follow-up for the client code and the tests. Thanks, -Stolee
On 5/3/21 11:25 AM, Derrick Stolee wrote: > On 4/8/21 9:10 PM, Jonathan Tan wrote: >> Currently, the packfile negotiation step within a Git fetch cannot be >> done independent of sending the packfile, even though there is at least >> one application wherein this is useful. Therefore, make it possible for >> this negotiation step to be done independently. A subsequent commit will >> use this for one such application - push negotiation. > ... >> diff --git a/transport-helper.c b/transport-helper.c >> index 4cd76366fa..4be035edb8 100644 >> --- a/transport-helper.c >> +++ b/transport-helper.c >> @@ -684,6 +684,16 @@ static int fetch(struct transport *transport, >> return transport->vtable->fetch(transport, nr_heads, to_fetch); >> } >> >> + /* >> + * If we reach here, then the server, the client, and/or the transport >> + * helper does not support protocol v2. --negotiate-only requires >> + * protocol v2. >> + */ >> + if (data->transport_options.acked_commits) { >> + warning(_("--negotiate-only requires protocol v2")); >> + return -1; >> + } >> + > > This method continues to do a lot that doesn't seem specific to > --negotiate-only. The warning message seems incorrect to me, but > is also seems like this would break several cases when using > protocol v0. It is equally possible that I'm misunderstanding > what is going on here. I didn't see this cause any failures when running the test suite with GIT_TEST_PROTOCOL_VERSION=0, but I _do_ see failures in your tests added in PATCH 6 with that environment variable set. You might want to set it manually in the important test cases. Thanks, -Stolee
> On 4/8/21 9:10 PM, Jonathan Tan wrote: > > Currently, the packfile negotiation step within a Git fetch cannot be > > done independent of sending the packfile, even though there is at least > > one application wherein this is useful. Therefore, make it possible for > > this negotiation step to be done independently. A subsequent commit will > > use this for one such application - push negotiation. > ... > > --- a/Documentation/technical/protocol-v2.txt > > +++ b/Documentation/technical/protocol-v2.txt > > @@ -346,6 +346,14 @@ explained below. > > client should download from all given URIs. Currently, the > > protocols supported are "http" and "https". > > > > +If the 'wait-for-done' feature is advertised, the following argument > > +can be included in the client's request. > > + > > + wait-for-done > > + Indicates to the server that it should never send "ready", but > > + should wait for the client to say "done" before sending the > > + packfile. > > + > > Ok, this is a good way to handle the compatibility case. I'll check > in the next patch how this feature is checked before sending a pack-file > in 'git push'. Thanks. > > @@ -202,6 +203,8 @@ static struct option builtin_fetch_options[] = { > > TRANSPORT_FAMILY_IPV6), > > OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"), > > N_("report that we have only objects reachable from this object")), > > + OPT_BOOL(0, "negotiate-only", &negotiate_only, > > + N_("do not fetch a packfile; instead, print ancestors of negotiation tips")), > > It seems a little strange for this step, which is mostly used for tests, > to be so visible. I think it would be too much work to create a test > helper with an equivalent implementation, so maybe describe this option > as only for tests in Documentation/git-fetch.txt. As you mention in your review of the subsequent patch [1], this is because we're calling "fetch" as a subprocess from "push", so it needs to accept command-line arguments specifying that we're not interested in the packfile. [1] https://lore.kernel.org/git/c2780570-81ec-bede-2f4e-75748b788bbb@gmail.com/ > > - if (remote) { > > + if (negotiate_only) { > > + struct oidset acked_commits = OIDSET_INIT; > > + struct oidset_iter iter; > > + const struct object_id *oid; > > + > > + if (!remote) > > + die(_("Must supply remote when using --negotiate-only")); > > Please use lowercase letters to start your error messages. Thanks - done. > > +static int add_to_object_array(const struct object_id *oid, void *data) > > +{ > > + struct object_array *a = data; > > + > > + add_object_array(parse_object(the_repository, oid), "", a); > > + return 0; > > +} > > Why parse_object() over lookup_object()? Switched to lookup_object(). I don't think it matters here because we'll need to parse all commits when doing negotiation anyway (to find their parents), but I changed it to avoid confusion. > > +void negotiate_using_fetch(const struct oid_array *negotiation_tips, > > + const struct string_list *server_options, > > + int stateless_rpc, > > + int fd[], > > + struct oidset *acked_commits) > > It might be worth a careful doc comment to say that we _must_ > have already verified the wait-for-done capability. OK - written in the .h file. [snip min_generation explanation] > And finally here you can use min_generation. Otherwise, you will suffer > performance problems when there is a deep history but this returns false. > > As long as min_generation is the minimum generation of a commit with the > COMMON flag, you will be correct to use that limit. Thanks for the precise guidance of how to use generation numbers to speed things up. I've implemented your suggestions. > > > + last_iteration = 1; > > + } > > + strbuf_release(&req_buf); > > Don't forget to clear the COMMON flag before returning. OK - done. (I don't think it matters here, but I guess it's clearer if we do as you suggest. The loop will take some time to run, but it's small compared to the network time needed anyway.) > > +/* > > + * Execute the --negotiate-only mode of "git fetch", adding all known common > > + * commits to acked_commits. > > + */> +void negotiate_using_fetch(const struct oid_array *negotiation_tips, > > + const struct string_list *server_options, > > + int stateless_rpc, > > + int fd[], > > + struct oidset *acked_commits); > > Here is a good place to specifically mention the wait-for-done capability, > or else this method will die() when seeing the "ready" from the server. Acknowledged. > > diff --git a/transport-helper.c b/transport-helper.c > > index 4cd76366fa..4be035edb8 100644 > > --- a/transport-helper.c > > +++ b/transport-helper.c > > @@ -684,6 +684,16 @@ static int fetch(struct transport *transport, > > return transport->vtable->fetch(transport, nr_heads, to_fetch); > > } > > > > + /* > > + * If we reach here, then the server, the client, and/or the transport > > + * helper does not support protocol v2. --negotiate-only requires > > + * protocol v2. > > + */ > > + if (data->transport_options.acked_commits) { > > + warning(_("--negotiate-only requires protocol v2")); > > + return -1; > > + } > > + > > This method continues to do a lot that doesn't seem specific to > --negotiate-only. The warning message seems incorrect to me, but > is also seems like this would break several cases when using > protocol v0. It is equally possible that I'm misunderstanding > what is going on here. It's specific to --negotiate-only in that acked_commits is set if and only if we use --negotiate-only. What cases are you think of that break when using protocol v0? > > + if (data->options.acked_commits) { > > + if (data->version < protocol_v2) { > > + warning(_("--negotiate-only requires protocol v2")); > > + ret = -1; > > + } else if (!server_supports_feature("fetch", "wait-for-done", 0)) { > > + warning(_("server does not support wait-for-done")); > > + ret = -1; > > + } else { > > + negotiate_using_fetch(data->options.negotiation_tips, > > + transport->server_options, > > + transport->stateless_rpc, > > + data->fd, > > + data->options.acked_commits); > > + ret = 0; > > + } > > + goto cleanup; > > + } > > These prereqs look right. This makes it look rather strange that > negotiate_using_fetch() returns void (and has several die() calls inside) > and could instead return an 'int' that is consumed here (and use > error() calls instead of die()). When negotiate_using_fetch() is used, we are already past the capability advertisement step, so we need to believe here that the wait-for-done prerequisite has been met. (Admittedly, said prerequisite wasn't documented, but I've documented it following your suggestion above.) So I think it would be too late to return an "int" after several negotiation steps have occurred and the packfile downloaded. > > + > > + /* > > + * If set, whenever transport_fetch_refs() is called, add known common > > + * commits to this oidset instead of fetching any packfiles. > > + */ > > + struct oidset *acked_commits; > > nit: s/If set/If allocated/ Changed. > > @@ -1668,10 +1673,13 @@ int upload_pack_v2(struct repository *r, struct strvec *keys, > > case FETCH_PROCESS_ARGS: > > process_args(request, &data); > > > > - if (!data.want_obj.nr) { > > + if (!data.want_obj.nr && !data.wait_for_done) { > > /* > > - * Request didn't contain any 'want' lines, > > - * guess they didn't want anything. > > + * Request didn't contain any 'want' lines (and > > + * the request does not contain > > + * "wait-for-done", in which it is reasonable > > + * to just send 'have's without 'want's); guess > > + * they didn't want anything. > > */ > > state = FETCH_DONE; > > } else if (data.haves.nr) { > > @@ -1723,7 +1731,7 @@ int upload_pack_advertise(struct repository *r, > > int allow_sideband_all_value; > > char *str = NULL; > > > > - strbuf_addstr(value, "shallow"); > > + strbuf_addstr(value, "shallow wait-for-done"); > > It might be nice to have this wait-for-done capability configurable. > I'd likely want this on by default, but in case there is an issue it > is easier to change config values than ship new binaries. I felt that we didn't need this knob on the server side as this doesn't expose any more information than a regular fetch, but I'm open to adding it if neeed be. > This is a pretty big patch. It might have been nice to split the > server-side stuff out as its own patch with a follow-up for the client > code and the tests. > > Thanks, > -Stolee Personally I prefer that such changes (server and client) be together, so that I can see on both sides how things work. There is (in theory) already a clear divide between both sides, even within a patch, since the server and client parts are usually in different parts. But if the majority prefers that such changes be separate, I'm OK with doing that too. Also, thanks for your review.
diff --git a/Documentation/technical/protocol-v2.txt b/Documentation/technical/protocol-v2.txt index a7c806a73e..0b371d8de4 100644 --- a/Documentation/technical/protocol-v2.txt +++ b/Documentation/technical/protocol-v2.txt @@ -346,6 +346,14 @@ explained below. client should download from all given URIs. Currently, the protocols supported are "http" and "https". +If the 'wait-for-done' feature is advertised, the following argument +can be included in the client's request. + + wait-for-done + Indicates to the server that it should never send "ready", but + should wait for the client to say "done" before sending the + packfile. + The response of `fetch` is broken into a number of sections separated by delimiter packets (0001), with each section beginning with its section header. Most sections are sent only when the packfile is sent. diff --git a/builtin/fetch.c b/builtin/fetch.c index 0b90de87c7..597973848a 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -82,6 +82,7 @@ static struct string_list server_options = STRING_LIST_INIT_DUP; static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP; static int fetch_write_commit_graph = -1; static int stdin_refspecs = 0; +static int negotiate_only; static int git_fetch_config(const char *k, const char *v, void *cb) { @@ -202,6 +203,8 @@ static struct option builtin_fetch_options[] = { TRANSPORT_FAMILY_IPV6), OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"), N_("report that we have only objects reachable from this object")), + OPT_BOOL(0, "negotiate-only", &negotiate_only, + N_("do not fetch a packfile; instead, print ancestors of negotiation tips")), OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options), OPT_BOOL(0, "auto-maintenance", &enable_auto_gc, N_("run 'maintenance --auto' after fetching")), @@ -1986,7 +1989,29 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) } } - if (remote) { + if (negotiate_only) { + struct oidset acked_commits = OIDSET_INIT; + struct oidset_iter iter; + const struct object_id *oid; + + if (!remote) + die(_("Must supply remote when using --negotiate-only")); + gtransport = prepare_transport(remote, 1); + if (gtransport->smart_options) { + gtransport->smart_options->acked_commits = &acked_commits; + } else { + warning(_("Protocol does not support --negotiate-only, exiting.")); + return 1; + } + if (server_options.nr) + gtransport->server_options = &server_options; + result = transport_fetch_refs(gtransport, NULL); + + oidset_iter_init(&acked_commits, &iter); + while ((oid = oidset_iter_next(&iter))) + printf("%s\n", oid_to_hex(oid)); + oidset_clear(&acked_commits); + } else if (remote) { if (filter_options.choice || has_promisor_remote()) fetch_one_setup_partial(remote); result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs); diff --git a/fetch-pack.c b/fetch-pack.c index 512fe5450d..63054e2fc8 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -23,6 +23,7 @@ #include "fetch-negotiator.h" #include "fsck.h" #include "shallow.h" +#include "commit-reach.h" static int transfer_unpack_limit = -1; static int fetch_unpack_limit = -1; @@ -45,6 +46,8 @@ static struct string_list uri_protocols = STRING_LIST_INIT_DUP; /* Remember to update object flag allocation in object.h */ #define COMPLETE (1U << 0) #define ALTERNATE (1U << 1) +#define COMMON (1U << 6) +#define REACH_SCRATCH (1U << 7) /* * After sending this many "have"s if we do not get any new ACK , we @@ -1523,10 +1526,10 @@ enum fetch_state { FETCH_DONE, }; -static void do_check_stateless_delimiter(const struct fetch_pack_args *args, +static void do_check_stateless_delimiter(int stateless_rpc, struct packet_reader *reader) { - check_stateless_delimiter(args->stateless_rpc, reader, + check_stateless_delimiter(stateless_rpc, reader, _("git fetch-pack: expected response end packet")); } @@ -1622,7 +1625,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, */ state = FETCH_GET_PACK; } else { - do_check_stateless_delimiter(args, &reader); + do_check_stateless_delimiter(args->stateless_rpc, &reader); state = FETCH_SEND_REQUEST; } break; @@ -1645,7 +1648,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, packfile_uris.nr ? &index_pack_args : NULL, sought, nr_sought, &fsck_options.gitmodules_found)) die(_("git fetch-pack: fetch failed.")); - do_check_stateless_delimiter(args, &reader); + do_check_stateless_delimiter(args->stateless_rpc, &reader); state = FETCH_DONE; break; @@ -1962,6 +1965,84 @@ struct ref *fetch_pack(struct fetch_pack_args *args, return ref_cpy; } +static int add_to_object_array(const struct object_id *oid, void *data) +{ + struct object_array *a = data; + + add_object_array(parse_object(the_repository, oid), "", a); + return 0; +} + +void negotiate_using_fetch(const struct oid_array *negotiation_tips, + const struct string_list *server_options, + int stateless_rpc, + int fd[], + struct oidset *acked_commits) +{ + struct fetch_negotiator negotiator; + struct packet_reader reader; + struct object_array nt_object_array = OBJECT_ARRAY_INIT; + struct strbuf req_buf = STRBUF_INIT; + int haves_to_send = INITIAL_FLUSH; + int in_vain = 0; + int seen_ack = 0; + int last_iteration = 0; + + fetch_negotiator_init(the_repository, &negotiator); + mark_tips(&negotiator, negotiation_tips); + + packet_reader_init(&reader, fd[0], NULL, 0, + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); + + oid_array_for_each((struct oid_array *) negotiation_tips, + add_to_object_array, + &nt_object_array); + + while (!last_iteration) { + int haves_added; + struct object_id common_oid; + int received_ready = 0; + + strbuf_reset(&req_buf); + write_fetch_command_and_capabilities(&req_buf, server_options); + + packet_buf_write(&req_buf, "wait-for-done"); + + haves_added = add_haves(&negotiator, &req_buf, &haves_to_send); + in_vain += haves_added; + if (!haves_added || (seen_ack && in_vain >= MAX_IN_VAIN)) + last_iteration = 1; + + /* Send request */ + packet_buf_flush(&req_buf); + if (write_in_full(fd[1], req_buf.buf, req_buf.len) < 0) + die_errno(_("unable to write request to remote")); + + /* Process ACKs/NAKs */ + process_section_header(&reader, "acknowledgments", 0); + while (process_ack(&negotiator, &reader, &common_oid, + &received_ready)) { + struct commit *commit = lookup_commit(the_repository, + &common_oid); + if (commit) + commit->object.flags |= COMMON; + in_vain = 0; + seen_ack = 1; + oidset_insert(acked_commits, &common_oid); + } + if (received_ready) + die(_("unexpected 'ready' from remote")); + else + do_check_stateless_delimiter(stateless_rpc, &reader); + if (can_all_from_reach_with_flag(&nt_object_array, COMMON, + REACH_SCRATCH, 0, + GENERATION_NUMBER_ZERO)) + last_iteration = 1; + } + strbuf_release(&req_buf); +} + int report_unmatched_refs(struct ref **sought, int nr_sought) { int i, ret = 0; diff --git a/fetch-pack.h b/fetch-pack.h index f114d72775..7c8f49aca7 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -5,6 +5,7 @@ #include "run-command.h" #include "protocol.h" #include "list-objects-filter-options.h" +#include "oidset.h" struct oid_array; @@ -81,6 +82,16 @@ struct ref *fetch_pack(struct fetch_pack_args *args, struct string_list *pack_lockfiles, enum protocol_version version); +/* + * Execute the --negotiate-only mode of "git fetch", adding all known common + * commits to acked_commits. + */ +void negotiate_using_fetch(const struct oid_array *negotiation_tips, + const struct string_list *server_options, + int stateless_rpc, + int fd[], + struct oidset *acked_commits); + /* * Print an appropriate error message for each sought ref that wasn't * matched. Return 0 if all sought refs were matched, otherwise 1. diff --git a/object.h b/object.h index 59daadce21..4806fa8e66 100644 --- a/object.h +++ b/object.h @@ -60,7 +60,7 @@ struct object_array { /* * object flag allocation: * revision.h: 0---------10 15 23------26 - * fetch-pack.c: 01 + * fetch-pack.c: 01 67 * negotiator/default.c: 2--5 * walker.c: 0-2 * upload-pack.c: 4 11-----14 16-----19 diff --git a/t/t5701-git-serve.sh b/t/t5701-git-serve.sh index 509f379d49..f03bb04803 100755 --- a/t/t5701-git-serve.sh +++ b/t/t5701-git-serve.sh @@ -16,7 +16,7 @@ test_expect_success 'test capability advertisement' ' version 2 agent=git/$(git version | cut -d" " -f3) ls-refs=unborn - fetch=shallow + fetch=shallow wait-for-done server-option object-format=$(test_oid algo) 0000 diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 2e1243ca40..5d91f067d0 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -585,6 +585,49 @@ test_expect_success 'deepen-relative' ' test_cmp expected actual ' +setup_negotiate_only () { + SERVER="$1" + URI="$2" + + rm -rf "$SERVER" client + + git init "$SERVER" + test_commit -C "$SERVER" one + test_commit -C "$SERVER" two + + git clone "$URI" client + test_commit -C client three +} + +test_expect_success 'file:// --negotiate-only' ' + SERVER="server" && + URI="file://$(pwd)/server" && + + setup_negotiate_only "$SERVER" "$URI" && + + git -C client fetch \ + --no-tags \ + --negotiate-only \ + --negotiation-tip=$(git -C client rev-parse HEAD) \ + origin >out && + COMMON=$(git -C "$SERVER" rev-parse two) && + grep "$COMMON" out +' + +test_expect_success 'file:// --negotiate-only with protocol v0' ' + SERVER="server" && + URI="file://$(pwd)/server" && + + setup_negotiate_only "$SERVER" "$URI" && + + test_must_fail git -c protocol.version=0 -C client fetch \ + --no-tags \ + --negotiate-only \ + --negotiation-tip=$(git -C client rev-parse HEAD) \ + origin 2>err && + test_i18ngrep "negotiate-only requires protocol v2" err +' + # Test protocol v2 with 'http://' transport # . "$TEST_DIRECTORY"/lib-httpd.sh @@ -1035,6 +1078,52 @@ test_expect_success 'packfile-uri with transfer.fsckobjects fails when .gitmodul test_i18ngrep "disallowed submodule name" err ' +test_expect_success 'http:// --negotiate-only' ' + SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" && + URI="$HTTPD_URL/smart/server" && + + setup_negotiate_only "$SERVER" "$URI" && + + git -C client fetch \ + --no-tags \ + --negotiate-only \ + --negotiation-tip=$(git -C client rev-parse HEAD) \ + origin >out && + COMMON=$(git -C "$SERVER" rev-parse two) && + grep "$COMMON" out +' + +test_expect_success 'http:// --negotiate-only without wait-for-done support' ' + SERVER="server" && + URI="$HTTPD_URL/one_time_perl/server" && + + setup_negotiate_only "$SERVER" "$URI" && + + echo "s/ wait-for-done/ xxxx-xxx-xxxx/" \ + >"$HTTPD_ROOT_PATH/one-time-perl" && + + test_must_fail git -C client fetch \ + --no-tags \ + --negotiate-only \ + --negotiation-tip=$(git -C client rev-parse HEAD) \ + origin 2>err && + test_i18ngrep "server does not support wait-for-done" err +' + +test_expect_success 'http:// --negotiate-only with protocol v0' ' + SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" && + URI="$HTTPD_URL/smart/server" && + + setup_negotiate_only "$SERVER" "$URI" && + + test_must_fail git -c protocol.version=0 -C client fetch \ + --no-tags \ + --negotiate-only \ + --negotiation-tip=$(git -C client rev-parse HEAD) \ + origin 2>err && + test_i18ngrep "negotiate-only requires protocol v2" err +' + # DO NOT add non-httpd-specific tests here, because the last part of this # test script is only executed when httpd is available and enabled. diff --git a/transport-helper.c b/transport-helper.c index 4cd76366fa..4be035edb8 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -684,6 +684,16 @@ static int fetch(struct transport *transport, return transport->vtable->fetch(transport, nr_heads, to_fetch); } + /* + * If we reach here, then the server, the client, and/or the transport + * helper does not support protocol v2. --negotiate-only requires + * protocol v2. + */ + if (data->transport_options.acked_commits) { + warning(_("--negotiate-only requires protocol v2")); + return -1; + } + if (!data->get_refs_list_called) get_refs_list_using_list(transport, 0); diff --git a/transport.c b/transport.c index ef66e73090..80caeaa72f 100644 --- a/transport.c +++ b/transport.c @@ -392,16 +392,29 @@ static int fetch_refs_via_pack(struct transport *transport, else if (data->version <= protocol_v1) die_if_server_options(transport); + if (data->options.acked_commits) { + if (data->version < protocol_v2) { + warning(_("--negotiate-only requires protocol v2")); + ret = -1; + } else if (!server_supports_feature("fetch", "wait-for-done", 0)) { + warning(_("server does not support wait-for-done")); + ret = -1; + } else { + negotiate_using_fetch(data->options.negotiation_tips, + transport->server_options, + transport->stateless_rpc, + data->fd, + data->options.acked_commits); + ret = 0; + } + goto cleanup; + } + refs = fetch_pack(&args, data->fd, refs_tmp ? refs_tmp : transport->remote_refs, to_fetch, nr_heads, &data->shallow, &transport->pack_lockfiles, data->version); - close(data->fd[0]); - close(data->fd[1]); - if (finish_connect(data->conn)) - ret = -1; - data->conn = NULL; data->got_remote_heads = 0; data->options.self_contained_and_connected = args.self_contained_and_connected; @@ -412,6 +425,13 @@ static int fetch_refs_via_pack(struct transport *transport, if (report_unmatched_refs(to_fetch, nr_heads)) ret = -1; +cleanup: + close(data->fd[0]); + close(data->fd[1]); + if (finish_connect(data->conn)) + ret = -1; + data->conn = NULL; + free_refs(refs_tmp); free_refs(refs); return ret; diff --git a/transport.h b/transport.h index 4d5db0a7f2..b8b3d42e62 100644 --- a/transport.h +++ b/transport.h @@ -47,6 +47,12 @@ struct git_transport_options { * transport_set_option(). */ struct oid_array *negotiation_tips; + + /* + * If set, whenever transport_fetch_refs() is called, add known common + * commits to this oidset instead of fetching any packfiles. + */ + struct oidset *acked_commits; }; enum transport_family { diff --git a/upload-pack.c b/upload-pack.c index e19583ae0f..b432ef0119 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -103,6 +103,7 @@ struct upload_pack_data { unsigned use_ofs_delta : 1; unsigned no_progress : 1; unsigned use_include_tag : 1; + unsigned wait_for_done : 1; unsigned allow_filter : 1; unsigned allow_filter_fallback : 1; unsigned long tree_filter_max_depth; @@ -1496,6 +1497,10 @@ static void process_args(struct packet_reader *request, data->done = 1; continue; } + if (!strcmp(arg, "wait-for-done")) { + data->wait_for_done = 1; + continue; + } /* Shallow related arguments */ if (process_shallow(arg, &data->shallows)) @@ -1578,7 +1583,7 @@ static int send_acks(struct upload_pack_data *data, struct oid_array *acks) oid_to_hex(&acks->oid[i])); } - if (ok_to_give_up(data)) { + if (!data->wait_for_done && ok_to_give_up(data)) { /* Send Ready */ packet_writer_write(&data->writer, "ready\n"); return 1; @@ -1668,10 +1673,13 @@ int upload_pack_v2(struct repository *r, struct strvec *keys, case FETCH_PROCESS_ARGS: process_args(request, &data); - if (!data.want_obj.nr) { + if (!data.want_obj.nr && !data.wait_for_done) { /* - * Request didn't contain any 'want' lines, - * guess they didn't want anything. + * Request didn't contain any 'want' lines (and + * the request does not contain + * "wait-for-done", in which it is reasonable + * to just send 'have's without 'want's); guess + * they didn't want anything. */ state = FETCH_DONE; } else if (data.haves.nr) { @@ -1723,7 +1731,7 @@ int upload_pack_advertise(struct repository *r, int allow_sideband_all_value; char *str = NULL; - strbuf_addstr(value, "shallow"); + strbuf_addstr(value, "shallow wait-for-done"); if (!repo_config_get_bool(the_repository, "uploadpack.allowfilter",
Currently, the packfile negotiation step within a Git fetch cannot be done independent of sending the packfile, even though there is at least one application wherein this is useful. Therefore, make it possible for this negotiation step to be done independently. A subsequent commit will use this for one such application - push negotiation. This feature is for protocol v2 only. (An implementation for protocol v0 would require a separate implementation in the fetch, transport, and transport helper code.) In the protocol, the main hindrance towards independent negotiation is that the server can unilaterally decide to send the packfile. This is solved by a "wait-for-done" argument: the server will then wait for the client to say "done". In practice, the client will never say it; instead it will cease requests once it is satisfied. In the client, the main change lies in the transport and transport helper code. fetch_refs_via_pack() performs everything needed - protocol version and capability checks, and the negotiation itself. There are 2 code paths that do not go through fetch_refs_via_pack() that needed to be individually excluded: the bundle transport (excluded through requiring smart_options, which the bundle transport doesn't support) and transport helpers that do not support takeover. Fortunately, none of these support protocol v2. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- Documentation/technical/protocol-v2.txt | 8 +++ builtin/fetch.c | 27 +++++++- fetch-pack.c | 89 +++++++++++++++++++++++-- fetch-pack.h | 11 +++ object.h | 2 +- t/t5701-git-serve.sh | 2 +- t/t5702-protocol-v2.sh | 89 +++++++++++++++++++++++++ transport-helper.c | 10 +++ transport.c | 30 +++++++-- transport.h | 6 ++ upload-pack.c | 18 +++-- 11 files changed, 275 insertions(+), 17 deletions(-)