| Message ID | 20220208235631.732466-1-calvinwan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fetch —object-info-format: client option for object-info | expand |
On 08/02/2022 23:56, Calvin Wan wrote: > Add ‘—object-info-format’ option to fetch. This option allows > the client to make an object-info [1] command request to a server > that supports protocol v2. > > The transport implementation uses vtables [2], similar to how Git > fetches refs, to determine whether a process needs to be taken over > before sending the object-info request. Different protocols > require different setups for making requests. > > [1] https://lore.kernel.org/git/20210420233830.2181153-1-bga@google.com/ > [2] https://lore.kernel.org/git/26f276956001a120cc9105b0071762c2fd4a45c5.15= > 13287544.git.jonathantanmy@google.com/ > > Helped-by: Jonathan Tan <jonathantanmy@google.com> > Signed-off-by: Calvin Wan <calvinwan@google.com> > > --- > Please ignore the patch above. It was sent with a stale patch message. > builtin/fetch.c | 26 ++++++++++++ > fetch-pack.c | 53 +++++++++++++++++++++++ > fetch-pack.h | 7 ++++ > t/t5583-fetch-object-info.sh | 81 ++++++++++++++++++++++++++++++++++++ > transport-helper.c | 12 ++++++ > transport-internal.h | 1 + > transport.c | 59 ++++++++++++++++++++++++++ > transport.h | 1 + > 8 files changed, 240 insertions(+) > create mode 100755 t/t5583-fetch-object-info.sh > > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 5f06b21f8e..b48d9e93d0 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -29,6 +29,9 @@ > #include "commit-graph.h" > #include "shallow.h" > #include "worktree.h" > +#include "protocol.h" > +#include "pkt-line.h" > +#include "connect.h" > > #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) > > @@ -37,6 +40,7 @@ static const char * const builtin_fetch_usage[] = { > N_("git fetch [<options>] <group>"), > N_("git fetch --multiple [<options>] [(<repository> | <group>)...]"), > N_("git fetch --all [<options>]"), > + N_("git fetch --object-info-format=[<arguments>] <remote> [<object-ids>]"), > NULL > }; Doesn't this also need a matching Documentarian update for the option? -- Philip
Calvin Wan <calvinwan@google.com> writes: > Add ‘—object-info-format’ option to fetch. This option allows > the client to make an object-info [1] command request to a server > that supports protocol v2. Avoid using characters above 127 in commit messages (unless, say, as part of someone's name). They make it hard to search for, and in this case, whatever dash is there is wrong (it should be "--"). > The transport implementation uses vtables [2], similar to how Git > fetches refs, You should be explicit that you're adding a new function to the vtable. (Whether that is what we should do is another issue: let me look at the patch to see.) > to determine whether a process needs to be taken over > before sending the object-info request. The vtable is not to determine whether a process needs to be taken over, but so that we support multiple protocols (HTTP, SSH, etc.). In any case, this detail is probably not relevant. > Different protocols > require different setups for making requests. This is true, but I don't see the relevance. > [1] https://lore.kernel.org/git/20210420233830.2181153-1-bga@google.com/ > [2] https://lore.kernel.org/git/26f276956001a120cc9105b0071762c2fd4a45c5.15= > 13287544.git.jonathantanmy@google.com/ For merged code, quote the commit, not the email. > @@ -220,6 +225,8 @@ static struct option builtin_fetch_options[] = { > N_("write the commit-graph after fetching")), > OPT_BOOL(0, "stdin", &stdin_refspecs, > N_("accept refspecs from stdin")), > + OPT_STRING_LIST(0, "object-info-format", &object_info_format, N_("option"), > + N_("command request arguments")), I would have expected a parameter named "format" to take a format string, but taking a string list of the fields we need might work too. In any case, maybe rename it to "--object-info" or similar. > @@ -2000,6 +2007,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > struct remote *remote = NULL; > int result = 0; > int prune_tags_ok = 1; > + struct oid_array oids = OID_ARRAY_INIT; > + struct object_id oid; The "oids" at function level needs a more descriptive name (e.g. "oids_for_object_info"). The name of "oid" is fine, since it's just used as a temporary variable, but since it is temporary, it should be declared in the block where it's used. (Same for "oids", actually: declare it in the block it's used, and in that case you can keep the name since it's more tightly scoped.) > @@ -2057,6 +2066,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > if (dry_run) > write_fetch_head = 0; > > + if (object_info_format.nr > 0) { > + if (argc == 0 || argc == 1) { > + die(_("must supply remote and object ids when using --object-info-format")); > + } else { > + remote = remote_get(argv[0]); > + for (i = 1; i < argc; i++) { > + if (get_oid(argv[i], &oid)) > + return error(_("malformed object name '%s'"), argv[i]); > + oid_array_append(&oids, &oid); > + } > + } > + gtransport = prepare_transport(remote, 0); > + gtransport->server_options = &object_info_format; > + result = transport_fetch_object_info(gtransport, &oids); > + return result; > + } I was thinking that this should reuse the refspec parsing mechanism (which also supports stdin), but upon more thought, using the refspec parser means that we would also need to check that all refspecs are exact OIDs (because we wouldn't know what to do with them otherwise). OK, parsing the objects by ourselves looks reasonable. The assignment of object_info_format to server_options is probably not a good idea, though, since readers of server_options would expect server options, not what you're assigning. The best place to put this information is in smart_options. (See the negotiate_only code.) > +static void write_object_info_command_and_capabilities(struct strbuf *req_buf, > + const struct string_list *server_options) > +{ [snip contents] This code is very similar to code in fetch-pack.c. If you stick to crafting the request in builtin/fetch.c, you should refactor fetch-pack.{c,h} to expose this functionality (in a preparatory commit) and then use that function from here. > +void send_object_info_request(int fd_out, struct object_info_args *args) > +{ > + struct strbuf req_buf = STRBUF_INIT; > + int i; > + > + write_object_info_command_and_capabilities(&req_buf, args->server_options); > + > + if (string_list_has_string(args->server_options, "size")) > + packet_buf_write(&req_buf, "size"); What happens if "size" is not in the list? > + printf "%s %d\n" "$object_id" "$length" >expect && You can just write "echo $object_id $length >expect". Also, test the following: - more than one OID - an OID that's not on the remote - a malformed OID - a server that doesn't support protocol v2 - a server that supports protocol v2 but not object-format (You don't have to do this for all protocols; just pick one. I prefer HTTP, since that's the most complex.) Other than that, the tests look good. Thanks for testing the different protocols. > @@ -1269,6 +1280,7 @@ static struct transport_vtable vtable = { > .get_refs_list = get_refs_list, > .fetch_refs = fetch_refs, > .push_refs = push_refs, > + .fetch_object_info = fetch_object_info, > .connect = connect_helper, > .disconnect = release_helper > }; Adding a function to the transport vtable is not so disruptive since we don't have many transport vtables, but better if we can avoid this disruption. In this case, I think it's better to reuse fetch_refs. Mainly, the plumbing from transport_fetch_refs() to all the helper functions in fetch-pack.c already exists, so reusing fetch_refs would allow us to reuse that plumbing. This also means that we don't have to expose the protocol functionality in fetch-pack.c that you copied over to builtin/fetch.c in this patch, which is an added bonus. > +static int fetch_object_info(struct transport *transport, struct oid_array *oids) > +{ [snip contents] And reusing the plumbing might mean that we don't need this function too. Taking a step back, there also needs to be a fallback mechanism for when the server doesn't support object-info. If you generally agree with my review comments, I would say that your next steps are: - investigate if we can reuse the transport_fetch_pack -> fetch-pack.c machinery - make a fallback for when the server doesn't support object-info (might be easier when we use the machinery, so I would start with that first)
Yes it does, thank you! On Wed, Feb 9, 2022 at 4:48 AM Philip Oakley <philipoakley@iee.email> wrote: > > On 08/02/2022 23:56, Calvin Wan wrote: > > Add ‘—object-info-format’ option to fetch. This option allows > > the client to make an object-info [1] command request to a server > > that supports protocol v2. > > > > The transport implementation uses vtables [2], similar to how Git > > fetches refs, to determine whether a process needs to be taken over > > before sending the object-info request. Different protocols > > require different setups for making requests. > > > > [1] https://lore.kernel.org/git/20210420233830.2181153-1-bga@google.com/ > > [2] https://lore.kernel.org/git/26f276956001a120cc9105b0071762c2fd4a45c5.15= > > 13287544.git.jonathantanmy@google.com/ > > > > Helped-by: Jonathan Tan <jonathantanmy@google.com> > > Signed-off-by: Calvin Wan <calvinwan@google.com> > > > > --- > > Please ignore the patch above. It was sent with a stale patch message. > > builtin/fetch.c | 26 ++++++++++++ > > fetch-pack.c | 53 +++++++++++++++++++++++ > > fetch-pack.h | 7 ++++ > > t/t5583-fetch-object-info.sh | 81 ++++++++++++++++++++++++++++++++++++ > > transport-helper.c | 12 ++++++ > > transport-internal.h | 1 + > > transport.c | 59 ++++++++++++++++++++++++++ > > transport.h | 1 + > > 8 files changed, 240 insertions(+) > > create mode 100755 t/t5583-fetch-object-info.sh > > > > diff --git a/builtin/fetch.c b/builtin/fetch.c > > index 5f06b21f8e..b48d9e93d0 100644 > > --- a/builtin/fetch.c > > +++ b/builtin/fetch.c > > @@ -29,6 +29,9 @@ > > #include "commit-graph.h" > > #include "shallow.h" > > #include "worktree.h" > > +#include "protocol.h" > > +#include "pkt-line.h" > > +#include "connect.h" > > > > #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) > > > > @@ -37,6 +40,7 @@ static const char * const builtin_fetch_usage[] = { > > N_("git fetch [<options>] <group>"), > > N_("git fetch --multiple [<options>] [(<repository> | <group>)...]"), > > N_("git fetch --all [<options>]"), > > + N_("git fetch --object-info-format=[<arguments>] <remote> [<object-ids>]"), > > NULL > > }; > > Doesn't this also need a matching Documentarian update for the option? > -- > Philip
> If you generally agree with my review comments Everything you've said makes sense. Thanks for taking an in depth look at this! I'll make the changes you suggested and then start working on the next steps. On Wed, Feb 9, 2022 at 12:42 PM Jonathan Tan <jonathantanmy@google.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > Add ‘—object-info-format’ option to fetch. This option allows > > the client to make an object-info [1] command request to a server > > that supports protocol v2. > > Avoid using characters above 127 in commit messages (unless, say, as part of > someone's name). They make it hard to search for, and in this case, whatever > dash is there is wrong (it should be "--"). > > > The transport implementation uses vtables [2], similar to how Git > > fetches refs, > > You should be explicit that you're adding a new function to the vtable. > (Whether that is what we should do is another issue: let me look at the > patch to see.) > > > to determine whether a process needs to be taken over > > before sending the object-info request. > > The vtable is not to determine whether a process needs to be taken over, > but so that we support multiple protocols (HTTP, SSH, etc.). In any > case, this detail is probably not relevant. > > > Different protocols > > require different setups for making requests. > > This is true, but I don't see the relevance. > > > [1] https://lore.kernel.org/git/20210420233830.2181153-1-bga@google.com/ > > [2] https://lore.kernel.org/git/26f276956001a120cc9105b0071762c2fd4a45c5.15= > > 13287544.git.jonathantanmy@google.com/ > > For merged code, quote the commit, not the email. > > > @@ -220,6 +225,8 @@ static struct option builtin_fetch_options[] = { > > N_("write the commit-graph after fetching")), > > OPT_BOOL(0, "stdin", &stdin_refspecs, > > N_("accept refspecs from stdin")), > > + OPT_STRING_LIST(0, "object-info-format", &object_info_format, N_("option"), > > + N_("command request arguments")), > > I would have expected a parameter named "format" to take a format > string, but taking a string list of the fields we need might work too. > In any case, maybe rename it to "--object-info" or similar. > > > @@ -2000,6 +2007,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > > struct remote *remote = NULL; > > int result = 0; > > int prune_tags_ok = 1; > > + struct oid_array oids = OID_ARRAY_INIT; > > + struct object_id oid; > > The "oids" at function level needs a more descriptive name (e.g. > "oids_for_object_info"). The name of "oid" is fine, since it's just used > as a temporary variable, but since it is temporary, it should be > declared in the block where it's used. (Same for "oids", actually: > declare it in the block it's used, and in that case you can keep the > name since it's more tightly scoped.) > > > @@ -2057,6 +2066,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) > > if (dry_run) > > write_fetch_head = 0; > > > > + if (object_info_format.nr > 0) { > > + if (argc == 0 || argc == 1) { > > + die(_("must supply remote and object ids when using --object-info-format")); > > + } else { > > + remote = remote_get(argv[0]); > > + for (i = 1; i < argc; i++) { > > + if (get_oid(argv[i], &oid)) > > + return error(_("malformed object name '%s'"), argv[i]); > > + oid_array_append(&oids, &oid); > > + } > > + } > > + gtransport = prepare_transport(remote, 0); > > + gtransport->server_options = &object_info_format; > > + result = transport_fetch_object_info(gtransport, &oids); > > + return result; > > + } > > I was thinking that this should reuse the refspec parsing mechanism > (which also supports stdin), but upon more thought, using the refspec > parser means that we would also need to check that all refspecs are > exact OIDs (because we wouldn't know what to do with them otherwise). > OK, parsing the objects by ourselves looks reasonable. > > The assignment of object_info_format to server_options is probably not a > good idea, though, since readers of server_options would expect server > options, not what you're assigning. The best place to put this > information is in smart_options. (See the negotiate_only code.) > > > +static void write_object_info_command_and_capabilities(struct strbuf *req_buf, > > + const struct string_list *server_options) > > +{ > > [snip contents] > > This code is very similar to code in fetch-pack.c. If you stick to > crafting the request in builtin/fetch.c, you should refactor > fetch-pack.{c,h} to expose this functionality (in a preparatory commit) > and then use that function from here. > > > +void send_object_info_request(int fd_out, struct object_info_args *args) > > +{ > > + struct strbuf req_buf = STRBUF_INIT; > > + int i; > > + > > + write_object_info_command_and_capabilities(&req_buf, args->server_options); > > + > > + if (string_list_has_string(args->server_options, "size")) > > + packet_buf_write(&req_buf, "size"); > > What happens if "size" is not in the list? > > > + printf "%s %d\n" "$object_id" "$length" >expect && > > You can just write "echo $object_id $length >expect". Also, test the > following: > - more than one OID > - an OID that's not on the remote > - a malformed OID > - a server that doesn't support protocol v2 > - a server that supports protocol v2 but not object-format > > (You don't have to do this for all protocols; just pick one. I prefer > HTTP, since that's the most complex.) > > Other than that, the tests look good. Thanks for testing the different > protocols. > > > @@ -1269,6 +1280,7 @@ static struct transport_vtable vtable = { > > .get_refs_list = get_refs_list, > > .fetch_refs = fetch_refs, > > .push_refs = push_refs, > > + .fetch_object_info = fetch_object_info, > > .connect = connect_helper, > > .disconnect = release_helper > > }; > > Adding a function to the transport vtable is not so disruptive since we > don't have many transport vtables, but better if we can avoid this > disruption. In this case, I think it's better to reuse fetch_refs. > Mainly, the plumbing from transport_fetch_refs() to all the helper > functions in fetch-pack.c already exists, so reusing fetch_refs would > allow us to reuse that plumbing. > > This also means that we don't have to expose the protocol functionality > in fetch-pack.c that you copied over to builtin/fetch.c in this patch, > which is an added bonus. > > > +static int fetch_object_info(struct transport *transport, struct oid_array *oids) > > +{ > > [snip contents] > > And reusing the plumbing might mean that we don't need this function > too. > > Taking a step back, there also needs to be a fallback mechanism for when > the server doesn't support object-info. > > If you generally agree with my review comments, I would say that your > next steps are: > - investigate if we can reuse the transport_fetch_pack -> fetch-pack.c > machinery > - make a fallback for when the server doesn't support object-info > (might be easier when we use the machinery, so I would start with > that first)
diff --git a/builtin/fetch.c b/builtin/fetch.c index 5f06b21f8e..b48d9e93d0 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -29,6 +29,9 @@ #include "commit-graph.h" #include "shallow.h" #include "worktree.h" +#include "protocol.h" +#include "pkt-line.h" +#include "connect.h" #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000) @@ -37,6 +40,7 @@ static const char * const builtin_fetch_usage[] = { N_("git fetch [<options>] <group>"), N_("git fetch --multiple [<options>] [(<repository> | <group>)...]"), N_("git fetch --all [<options>]"), + N_("git fetch --object-info-format=[<arguments>] <remote> [<object-ids>]"), NULL }; @@ -85,6 +89,7 @@ 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 struct string_list object_info_format = STRING_LIST_INIT_NODUP; static int git_fetch_config(const char *k, const char *v, void *cb) { @@ -220,6 +225,8 @@ static struct option builtin_fetch_options[] = { N_("write the commit-graph after fetching")), OPT_BOOL(0, "stdin", &stdin_refspecs, N_("accept refspecs from stdin")), + OPT_STRING_LIST(0, "object-info-format", &object_info_format, N_("option"), + N_("command request arguments")), OPT_END() }; @@ -2000,6 +2007,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) struct remote *remote = NULL; int result = 0; int prune_tags_ok = 1; + struct oid_array oids = OID_ARRAY_INIT; + struct object_id oid; packet_trace_identity("fetch"); @@ -2057,6 +2066,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) if (dry_run) write_fetch_head = 0; + if (object_info_format.nr > 0) { + if (argc == 0 || argc == 1) { + die(_("must supply remote and object ids when using --object-info-format")); + } else { + remote = remote_get(argv[0]); + for (i = 1; i < argc; i++) { + if (get_oid(argv[i], &oid)) + return error(_("malformed object name '%s'"), argv[i]); + oid_array_append(&oids, &oid); + } + } + gtransport = prepare_transport(remote, 0); + gtransport->server_options = &object_info_format; + result = transport_fetch_object_info(gtransport, &oids); + return result; + } + if (all) { if (argc == 1) die(_("fetch --all does not take a repository argument")); diff --git a/fetch-pack.c b/fetch-pack.c index dd6ec449f2..d1c5254aa8 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1265,6 +1265,59 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf, packet_buf_delim(req_buf); } +static void write_object_info_command_and_capabilities(struct strbuf *req_buf, + const struct string_list *server_options) +{ + const char *hash_name; + + if (server_supports_v2("object-info", 1)) + packet_buf_write(req_buf, "command=object-info"); + if (server_supports_v2("agent", 0)) + packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized()); + if (advertise_sid && server_supports_v2("session-id", 0)) + packet_buf_write(req_buf, "session-id=%s", trace2_session_id()); + if (server_options && server_options->nr && + server_supports_v2("server-option", 1)) { + int i; + for (i = 0; i < server_options->nr; i++) + packet_buf_write(req_buf, "server-option=%s", + server_options->items[i].string); + } + + if (server_feature_v2("object-format", &hash_name)) { + int hash_algo = hash_algo_by_name(hash_name); + if (hash_algo_by_ptr(the_hash_algo) != hash_algo) + die(_("mismatched algorithms: client %s; server %s"), + the_hash_algo->name, hash_name); + packet_buf_write(req_buf, "object-format=%s", the_hash_algo->name); + } else if (hash_algo_by_ptr(the_hash_algo) != GIT_HASH_SHA1) { + die(_("the server does not support algorithm '%s'"), + the_hash_algo->name); + } + packet_buf_delim(req_buf); +} + +void send_object_info_request(int fd_out, struct object_info_args *args) +{ + struct strbuf req_buf = STRBUF_INIT; + int i; + + write_object_info_command_and_capabilities(&req_buf, args->server_options); + + if (string_list_has_string(args->server_options, "size")) + packet_buf_write(&req_buf, "size"); + + for (i = 0; i < args->oids->nr; i++) { + packet_buf_write(&req_buf, "oid %s\n", oid_to_hex(&args->oids->oid[i])); + } + + packet_buf_flush(&req_buf); + if (write_in_full(fd_out, req_buf.buf, req_buf.len) < 0) + die_errno(_("unable to write request to remote")); + + strbuf_release(&req_buf); +} + static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, struct fetch_pack_args *args, const struct ref *wants, struct oidset *common, diff --git a/fetch-pack.h b/fetch-pack.h index 7f94a2a583..2ad5ec5c64 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -68,6 +68,11 @@ struct fetch_pack_args { unsigned connectivity_checked:1; }; +struct object_info_args { + const struct string_list *server_options; + const struct oid_array *oids; +}; + /* * sought represents remote references that should be updated from. * On return, the names that were found on the remote will have been @@ -101,4 +106,6 @@ void negotiate_using_fetch(const struct oid_array *negotiation_tips, */ int report_unmatched_refs(struct ref **sought, int nr_sought); +void send_object_info_request(int fd_out, struct object_info_args *args); + #endif diff --git a/t/t5583-fetch-object-info.sh b/t/t5583-fetch-object-info.sh new file mode 100755 index 0000000000..942426b3ca --- /dev/null +++ b/t/t5583-fetch-object-info.sh @@ -0,0 +1,81 @@ +#!/bin/sh + +test_description='test git fetch object-info version 2' + +TEST_NO_CREATE_REPO=1 + +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + +. ./test-lib.sh + +# Test fetch object-info with 'git://' transport + +. "$TEST_DIRECTORY"/lib-git-daemon.sh +start_git_daemon --export-all --enable=receive-pack +daemon_parent=$GIT_DAEMON_DOCUMENT_ROOT_PATH/parent + + +test_expect_success 'create repo to be served by git-daemon' ' + git init "$daemon_parent" && + test_commit -C "$daemon_parent" message1 a.txt +' + +test_expect_success 'fetch object-info with git:// using protocol v2' ' + ( + cd "$daemon_parent" && + object_id=$(git rev-parse message1:a.txt) && + length=$(wc -c <a.txt) && + + printf "%s %d\n" "$object_id" "$length" >expect && + git -c protocol.version=2 fetch --object-info-format=size "$GIT_DAEMON_URL/parent" "$object_id" >actual && + test_cmp expect actual + ) +' +stop_git_daemon + +# Test protocol v2 with 'http://' transport +# +. "$TEST_DIRECTORY"/lib-httpd.sh +start_httpd + +test_expect_success 'create repo to be served by http:// transport' ' + git init "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" config http.receivepack true && + test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" message1 a.txt +' + +test_expect_success 'fetch object-info with http:// using protocol v2' ' + ( + cd "$HTTPD_DOCUMENT_ROOT_PATH/http_parent" && + object_id=$(git rev-parse message1:a.txt) && + length=$(wc -c <a.txt) && + + printf "%s %d\n" "$object_id" "$length" >expect && + git -c protocol.version=2 fetch --object-info-format=size "$HTTPD_URL/smart/http_parent" "$object_id" >actual && + test_cmp expect actual + ) +' + +# Test fetch object-info with 'file://' transport +# + +test_expect_success 'create repo to be served by file:// transport' ' + git init server && + test_commit -C server message1 a.txt && + git -C server config protocol.version 2 +' + +test_expect_success 'fetch object-info with file:// using protocol v2' ' + ( + cd server && + object_id=$(git rev-parse message1:a.txt) && + length=$(wc -c <a.txt) && + + printf "%s %d\n" "$object_id" "$length" >expect && + git fetch --object-info-format=size "file://$(pwd)" "$object_id" >actual && + test_cmp expect actual + ) +' + +test_done diff --git a/transport-helper.c b/transport-helper.c index a0297b0986..9ecda03dde 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -671,6 +671,17 @@ static int connect_helper(struct transport *transport, const char *name, static struct ref *get_refs_list_using_list(struct transport *transport, int for_push); +static int fetch_object_info(struct transport *transport, struct oid_array *oids) +{ + get_helper(transport); + + if (process_connect(transport, 0)) { + do_take_over(transport); + return transport->vtable->fetch_object_info(transport, oids); + } + return -1; +} + static int fetch_refs(struct transport *transport, int nr_heads, struct ref **to_fetch) { @@ -1269,6 +1280,7 @@ static struct transport_vtable vtable = { .get_refs_list = get_refs_list, .fetch_refs = fetch_refs, .push_refs = push_refs, + .fetch_object_info = fetch_object_info, .connect = connect_helper, .disconnect = release_helper }; diff --git a/transport-internal.h b/transport-internal.h index c4ca0b733a..04fa015011 100644 --- a/transport-internal.h +++ b/transport-internal.h @@ -59,6 +59,7 @@ struct transport_vtable { * use. disconnect() releases these resources. **/ int (*disconnect)(struct transport *connection); + int (*fetch_object_info)(struct transport *transport, struct oid_array *oids); }; #endif diff --git a/transport.c b/transport.c index 2a3e324154..87cd14c99f 100644 --- a/transport.c +++ b/transport.c @@ -445,6 +445,59 @@ static int fetch_refs_via_pack(struct transport *transport, return ret; } +static int fetch_object_info(struct transport *transport, struct oid_array *oids) +{ + int ret = 0; + struct git_transport_data *data = transport->data; + struct object_info_args args; + struct packet_reader reader; + + memset(&args, 0, sizeof(args)); + args.server_options = transport->server_options; + args.oids = oids; + + connect_setup(transport, 0); + packet_reader_init(&reader, data->fd[0], NULL, 0, + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); + data->version = discover_version(&reader); + + if (data->version == protocol_unknown_version) + BUG("unknown protocol version"); + else if (data->version <= protocol_v1) + die_if_server_options(transport); + + switch (data->version) { + case protocol_v2: + send_object_info_request(data->fd[1], &args); + break; + case protocol_v1: + case protocol_v0: + die(_("wrong protocol version. expected v2")); + case protocol_unknown_version: + BUG("unknown protocol version"); + } + + if (packet_reader_read(&reader) != PACKET_READ_NORMAL) { + die(_("error reading object info header")); + } + if (strcmp(reader.line, "size")) { + die(_("expected 'size', received '%s'"), reader.line); + } + while (packet_reader_read(&reader) == PACKET_READ_NORMAL) { + printf("%s\n", reader.line); + } + check_stateless_delimiter(transport->stateless_rpc, &reader, "stateless delimiter expected"); + close(data->fd[0]); + if (data->fd[1] >= 0) + close(data->fd[1]); + if (finish_connect(data->conn)) + ret = -1; + data->conn = NULL; + + return ret; +} + static int push_had_errors(struct ref *ref) { for (; ref; ref = ref->next) { @@ -890,6 +943,7 @@ static struct transport_vtable taken_over_vtable = { .get_refs_list = get_refs_via_connect, .fetch_refs = fetch_refs_via_pack, .push_refs = git_transport_push, + .fetch_object_info = fetch_object_info, .disconnect = disconnect_git }; @@ -1043,6 +1097,7 @@ static struct transport_vtable builtin_smart_vtable = { .get_refs_list = get_refs_via_connect, .fetch_refs = fetch_refs_via_pack, .push_refs = git_transport_push, + .fetch_object_info = fetch_object_info, .connect = connect_git, .disconnect = disconnect_git }; @@ -1420,6 +1475,10 @@ const struct ref *transport_get_remote_refs(struct transport *transport, return transport->remote_refs; } +int transport_fetch_object_info(struct transport *transport, struct oid_array *oids) { + return transport->vtable->fetch_object_info(transport, oids); +} + int transport_fetch_refs(struct transport *transport, struct ref *refs) { int rc; diff --git a/transport.h b/transport.h index 3f16e50c19..1c807591de 100644 --- a/transport.h +++ b/transport.h @@ -278,6 +278,7 @@ const struct ref *transport_get_remote_refs(struct transport *transport, * This can only be called after fetching the remote refs. */ const struct git_hash_algo *transport_get_hash_algo(struct transport *transport); +int transport_fetch_object_info(struct transport *transport, struct oid_array *oids); int transport_fetch_refs(struct transport *transport, struct ref *refs); /*
Add ‘—object-info-format’ option to fetch. This option allows the client to make an object-info [1] command request to a server that supports protocol v2. The transport implementation uses vtables [2], similar to how Git fetches refs, to determine whether a process needs to be taken over before sending the object-info request. Different protocols require different setups for making requests. [1] https://lore.kernel.org/git/20210420233830.2181153-1-bga@google.com/ [2] https://lore.kernel.org/git/26f276956001a120cc9105b0071762c2fd4a45c5.15= 13287544.git.jonathantanmy@google.com/ Helped-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- Please ignore the patch above. It was sent with a stale patch message. builtin/fetch.c | 26 ++++++++++++ fetch-pack.c | 53 +++++++++++++++++++++++ fetch-pack.h | 7 ++++ t/t5583-fetch-object-info.sh | 81 ++++++++++++++++++++++++++++++++++++ transport-helper.c | 12 ++++++ transport-internal.h | 1 + transport.c | 59 ++++++++++++++++++++++++++ transport.h | 1 + 8 files changed, 240 insertions(+) create mode 100755 t/t5583-fetch-object-info.sh base-commit: b23dac905bde28da47543484320db16312c87551