Re: [PATCH v2] fetch object-info-format: client option for object-info
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Feb 9, 2022, 20:41 UTC
- Message-ID
- <20220209204153.481122-1-jonathantanmy@google.com>
- In-Reply-To
- <20220208235631.732466-1-calvinwan@google.com>
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.
Show 6 quoted lines
> @@ -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.
Show 6 quoted lines
> @@ -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.)
Show 20 quoted lines
> @@ -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.
Show 9 quoted lines
> +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.
Show 8 quoted lines
> @@ -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)