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

From: Toon claes <[email-masked]>
Date: Tue, 09 Jul 2024 09:15:58 +0200

Eric Ju <[email-masked]> 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];

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

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

-- 
Toon