On Fri, Oct 25, 2024 at 6:13 AM karthik nayak <karthik.188@xxxxxxxxx> wrote: > > Eric Ju <eric.peijian@xxxxxxxxx> writes: > > [snip] > > > diff --git a/fetch-pack.c b/fetch-pack.c > > index 800505f25f..1a9facc1c0 100644 > > --- a/fetch-pack.c > > +++ b/fetch-pack.c > > @@ -1347,7 +1347,6 @@ static void write_command_and_capabilities(struct strbuf *req_buf, > > packet_buf_delim(req_buf); > > } > > > > - > > Seems like this was introduced in Patch 1/6, including the function > below which is not used in that patch. > Thank you. As explained at https://lore.kernel.org/git/CAN2LT1CEPdTAxCEpKtd+8-5zKYSnh0PMqEXgAZ++TTMPPKrD1g@xxxxxxxxxxxxxx/. In Patch 1/6, I am moving `write_command_and_capabilities()` to connect.c. And I am moving `send_object_info_request()` to a new file fetch-object-info.c in patch 4/6 where it is used. > > void send_object_info_request(int fd_out, struct object_info_args *args) > > { > > struct strbuf req_buf = STRBUF_INIT; > > @@ -1706,6 +1705,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, > > if (args->depth > 0 || args->deepen_since || args->deepen_not) > > args->deepen = 1; > > > > + if (args->object_info) > > + state = FETCH_SEND_REQUEST; > > + > > while (state != FETCH_DONE) { > > switch (state) { > > case FETCH_CHECK_LOCAL: > > [snip] > > > /* > > * sought represents remote references that should be updated from. > > * On return, the names that were found on the remote will have been > > @@ -106,4 +114,6 @@ int report_unmatched_refs(struct ref **sought, int nr_sought); > > */ > > int fetch_pack_fsck_objects(void); > > > > +void send_object_info_request(int fd_out, struct object_info_args *args); > > + > > > > Nit: Would be nice to have a comment here explaining what the function does. > Thank you. Added in v5. > > #endif > > diff --git a/transport-helper.c b/transport-helper.c > > index 013ec79dc9..2ff9675984 100644 > > --- a/transport-helper.c > > +++ b/transport-helper.c > > @@ -709,8 +709,8 @@ static int fetch_refs(struct transport *transport, > > > > /* > > * If we reach here, then the server, the client, and/or the transport > > - * helper does not support protocol v2. --negotiate-only requires > > - * protocol v2. > > + * helper does not support protocol v2. --negotiate-only and cat-file remote-object-info > > Nit: could we wrap this comment? > Thank you, Fixed in v5. > > + * require protocol v2. > > */ > > if (data->transport_options.acked_commits) { > > warning(_("--negotiate-only requires protocol v2")); > > [snip] > > > static struct ref *get_refs_via_connect(struct transport *transport, int for_push, > > struct transport_ls_refs_options *options) > > { > > @@ -418,6 +489,7 @@ static int fetch_refs_via_pack(struct transport *transport, > > struct ref *refs = NULL; > > struct fetch_pack_args args; > > struct ref *refs_tmp = NULL, **to_fetch_dup = NULL; > > + struct ref *object_info_refs = NULL; > > > > memset(&args, 0, sizeof(args)); > > args.uploadpack = data->options.uploadpack; > > @@ -444,11 +516,36 @@ static int fetch_refs_via_pack(struct transport *transport, > > args.server_options = transport->server_options; > > args.negotiation_tips = data->options.negotiation_tips; > > args.reject_shallow_remote = transport->smart_options->reject_shallow; > > + args.object_info = transport->smart_options->object_info; > > + > > + if (transport->smart_options > > + && transport->smart_options->object_info > > + && transport->smart_options->object_info_oids->nr > 0) { > > + struct ref *ref_itr = object_info_refs = alloc_ref(""); > > + > > + if (!fetch_object_info(transport, data->options.object_info_data)) > > + goto cleanup; > > So if we were successful, we skip to the cleanup. Okay. > Yes, that is right. > > + args.object_info_data = data->options.object_info_data; > > + args.quiet = 1; > > + args.no_progress = 1; > > Not sure why we set quiet and no_progress here. > Thank you. If the code reaches here, it means we fall back to downloading the pack file with fetch_pack(). Setting quiet and no_progress just wants to make fetch_pack less verbose and do its job quietly in the background. It is like calling `git fetch-pack -q ...`. I see setting quiet and no_progress is necessary here because: 1. If the call git fetch-pack is from an internal command, we would better keep the call lean and efficient. 2. If the user wants to do a verbose call, they have the choice to call git fetch-pack directly from the client. I add a comment in v5 to explain it, like this: " we can't retrieve object info in packets, so we will fall back to downland pack files. We set quiet and no_progress to be true, so that the internal call of fetch-pack is less verbose." > > + for (size_t i = 0; i < transport->smart_options->object_info_oids->nr; i++) { > > + ref_itr->old_oid = transport->smart_options->object_info_oids->oid[i]; > > + ref_itr->exact_oid = 1; > > + if (i == transport->smart_options->object_info_oids->nr - 1) > > + /* last element, no need to allocate to next */ > > + ref_itr->next = NULL; > > + else > > + ref_itr->next = alloc_ref(""); > > > > - if (!data->finished_handshake) { > > - int i; > > + ref_itr = ref_itr->next; > > + } > > + > > + transport->remote_refs = object_info_refs; > > + > > + } else if (!data->finished_handshake) { > > int must_list_refs = 0; > > - for (i = 0; i < nr_heads; i++) { > > + for (int i = 0; i < nr_heads; i++) { > > if (!to_fetch[i]->exact_oid) { > > must_list_refs = 1; > > break; > > @@ -494,16 +591,26 @@ static int fetch_refs_via_pack(struct transport *transport, > > &transport->pack_lockfiles, data->version); > > > > data->finished_handshake = 0; > > + if (args.object_info) { > > + struct ref *ref_cpy_reader = object_info_refs; > > + for (int i = 0; ref_cpy_reader; i++) { > > + oid_object_info_extended(the_repository, &ref_cpy_reader->old_oid, > > + &args.object_info_data[i], OBJECT_INFO_LOOKUP_REPLACE); > > + ref_cpy_reader = ref_cpy_reader->next; > > + } > > + } > > + > > data->options.self_contained_and_connected = > > args.self_contained_and_connected; > > data->options.connectivity_checked = args.connectivity_checked; > > > > - if (!refs) > > + if (!refs && !args.object_info) > > ret = -1; > > This is because, now we don't necessary always fetch the refs, since > sometimes we're just happy fetching the object info. Would be nice to > have a comment here. > Thank you. Acutally, if the code reaches here, it means we fall back to downloading the pack file. I would expect there is no difference from the old logic, so `!args.object_info` might not be needed here. I am removing `!args.object_info` in v5. > > if (report_unmatched_refs(to_fetch, nr_heads)) > > ret = -1; > > > > cleanup: > > + free_refs(object_info_refs); > > close(data->fd[0]); > > if (data->fd[1] >= 0) > > close(data->fd[1]); > > > [snip]
