> 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@xxxxxxxxxx> wrote: > > Calvin Wan <calvinwan@xxxxxxxxxx> 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@xxxxxxxxxx/ > > [2] https://lore.kernel.org/git/26f276956001a120cc9105b0071762c2fd4a45c5.15= > > 13287544.git.jonathantanmy@xxxxxxxxxx/ > > 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)
