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

From: Peijian Ju <eric.peijian@gmail.com>
Date: Jul 13, 2024, 02:30 UTC
Message-ID: <CAN2LT1D_L39EH53K=xAGx3PdK90-Wir52+mOpwwK42cc8nnHTQ@mail.gmail.com>
In-Reply-To: <87ikxfqbm9.fsf@iotcl.com>

On Tue, Jul 9, 2024 at 3:16 AM Toon claes <toon@iotcl.com> wrote:

Show 33 quoted lines

>
> Eric Ju <eric.peijian@gmail.com> writes:
>
> > diff --git a/transport.c b/transport.c
> > index 83ddea8fbc..2847aa3f3c 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -436,11 +504,27 @@ 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;
> > -
> > -     if (!data->finished_handshake) {
> > -             int i;
> > +     args.object_info = transport->smart_options->object_info;
> > +
> > +     if (transport->smart_options && transport->smart_options->object_info) {
> > +             struct ref *ref = object_info_refs;
> > +
> > +             if (!fetch_object_info(transport, data->options.object_info_data))
> > +                     goto cleanup;
> > +             args.object_info_data = data->options.object_info_data;
> > +             args.quiet = 1;
> > +             args.no_progress = 1;
> > +             for (size_t i = 0; i < transport->smart_options->object_info_oids->nr; i++) {
> > +                     struct ref *temp_ref = xcalloc(1, sizeof (struct ref));
> > +                     temp_ref->old_oid = *(transport->smart_options->object_info_oids->oid + i);
>
> Any reason why you're not using the subscript operator (square brackets)
> like this:
>
> +                       temp_ref->old_oid = transport->smart_options->object_info_oids->oid[I];
>

Thank you. Fixed in V2.

Show 9 quoted lines

> > +                     temp_ref->exact_oid = 1;
> > +                     ref->next = temp_ref;
> > +                     ref = ref->next;
> > +             }
> > +             transport->remote_refs = object_info_refs->next;
>
> I find it a bit weird you're allocating object_info_refs, only to use it
> to point to the next. Can I suggest a little refactor:
>

Thank you. I have to agree that the old implementation of iterating on the object_info_refs linked list is a bit obscure. Your suggestion is easier to follow. I am replacing the old logic in V2.

Show 56 quoted lines

> ----8<-----8<----
> diff --git a/transport.c b/transport.c
> index 662faa004e..56cb3a1693 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -479,7 +479,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>         struct ref *refs = NULL;
>         struct fetch_pack_args args;
>         struct ref *refs_tmp = NULL;
> -       struct ref *object_info_refs = xcalloc(1, sizeof (struct ref));
> +       struct ref *object_info_refs = NULL;
>
>         memset(&args, 0, sizeof(args));
>         args.uploadpack = data->options.uploadpack;
> @@ -509,7 +509,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>         args.object_info = transport->smart_options->object_info;
>
>         if (transport->smart_options && transport->smart_options->object_info) {
> -               struct ref *ref = object_info_refs;
> +               struct ref *ref = object_info_refs = xcalloc(1, sizeof (struct ref));
>
>                 if (!fetch_object_info(transport, data->options.object_info_data))
>                         goto cleanup;
> @@ -517,13 +517,12 @@ static int fetch_refs_via_pack(struct transport *transport,
>                 args.quiet = 1;
>                 args.no_progress = 1;
>                 for (size_t i = 0; i < transport->smart_options->object_info_oids->nr; i++) {
> -                       struct ref *temp_ref = xcalloc(1, sizeof (struct ref));
> -                       temp_ref->old_oid = *(transport->smart_options->object_info_oids->oid + i);
> -                       temp_ref->exact_oid = 1;
> -                       ref->next = temp_ref;
> +                       ref->old_oid = transport->smart_options->object_info_oids->oid[i];
> +                       ref->exact_oid = 1;
> +                       ref->next = xcalloc(1, sizeof (struct ref));
>                         ref = ref->next;
>                 }
> -               transport->remote_refs = object_info_refs->next;
> +               transport->remote_refs = object_info_refs;
>         } else if (!data->finished_handshake) {
>                 int must_list_refs = 0;
>                 for (int i = 0; i < nr_heads; i++) {
> @@ -565,7 +564,7 @@ static int fetch_refs_via_pack(struct transport *transport,
>
>         data->finished_handshake = 0;
>         if (args.object_info) {
> -               struct ref *ref_cpy_reader = object_info_refs->next;
> +               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;
> ----8<-----8<----
>
> To be honest, I'm not sure it works, because fetch_object_info() always
> seem to return a non-zero value. I'm not sure this is due to missing
> code coverage, or a bug. I guess it's worth looking into.
>

Thank you. I tested your suggestion and it is working. I can confirm
it when I did the following with my debugger
    1. pause on a test case of t/t1017-cat-file-remote-object-info.sh
    2. git cat-file "--batch-command=%(objectname) %(objectsize)"
    3. remote-object-info http://127.0.0.1:11017/smart/http_parent
5e1c309dae7f45e0f39b1bf3ac3cd9db12e7d689
I set breakpoints all along and see that fetch_object_info() returned zero

Would you mind sharing your test steps with me? I would love to dig deeper.

> --
> Toon