Re: [PATCH v4 4/6] transport: add client support for object-info

From: Peijian Ju <eric.peijian@gmail.com>
Date: Oct 28, 2024, 05:39 UTC
Message-ID: <CAN2LT1C1dh4oSO-3q71TX-BCy-JMoDXNXBSHA3P66_EjQ+0EKg@mail.gmail.com>
In-Reply-To: <CAOLa=ZS4XLNBeXewQ8O7tpobQfF9C9LKP--MVYghVR52hcqgWA@mail.gmail.com>

On Fri, Oct 25, 2024 at 6:13 AM karthik nayak <karthik.188@gmail.com> wrote:

Show 18 quoted lines

>
> Eric Ju <eric.peijian@gmail.com> 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@mail.gmail.com/.

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.

Show 29 quoted lines

> >  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.

Show 15 quoted lines

> >  #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.

Show 34 quoted lines

> > +      * 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.

Show 6 quoted lines

> > +             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."

Show 48 quoted lines

> > +             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.

Show 11 quoted lines

> >       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]