Re: [PATCH 4/6] transport: add client support for object-info
From: Peijian Ju <hidden>
Date: 2024-07-13 02:30:12
On Tue, Jul 9, 2024 at 3:16 AM Toon claes [off-list ref] wrote:
Eric Ju [off-list ref] writes:quoted
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.
quoted
+ 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.
quoted hunk ↗ jump to hunk
----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