From: Pablo Sabater Date: Tue, 09 Jun 2026 17:34:29 GMT Subject: Re: [PATCH GSoC RFC v12 12/12] cat-file: make remote-object-info allow-list dynamic Message-ID: In-Reply-To: El mar, 9 jun 2026 a las 17:32, Chandra Pratap () escribió: > > On Mon, 8 Jun 2026 at 15:45, Pablo Sabater wrote: > > > > The static allow-list in expand_atom() is hardcoded to only allow > > "objectname" and "objectsize" for remote queries. This works because > > up to this point all servers will either support object-info with name > > and size or they do not support them at all, but we cannot expect that > > in a future different servers with different git versions to have the > > same object-info capabilities. Therefore, the allow_list needs to be > > dynamic depending on what does the server advertise. > > > > The client will now: > > > > 1. Request the protocol option that the placeholder refers to (i.e. > > "size" when "%(objectsize)"). > > > > 2. Filters the request in fetch_object_info() dropping any option that > > the server does not advertise. > > > > 3. After the fetching, the options that haven't been dropped are the ones > > fetched and supported by the server, these supported options are > > mapped and remote_allowed_atoms is populated with the placeholders. > > > > 4. expand_atom() checks remote_allowed_atoms with the same behaviour as > > the static allow_list had. > > > > Move object_info_options out of get_remote_info so the caller which has > > data can select what options will be requested instead of requesting > > always size. > > Move batch_object_write() out so there will always be an output even if > > all the placeholders are not supported by the server (returns an empty > > line). > > > > Include "type" in the object_info_options so once the server supports > > it, the clients know already how to request it. > > > > Mentored-by: Karthik Nayak > > Mentored-by: Chandra Pratap > > Signed-off-by: Pablo Sabater > > --- > > builtin/cat-file.c | 85 ++++++++++++++++++++++++++++++++--------------------- > > fetch-object-info.c | 6 ++++ > > 2 files changed, 58 insertions(+), 33 deletions(-) > > [snip] > > diff --git a/fetch-object-info.c b/fetch-object-info.c > > index 51a898430d..425929a269 100644 > > --- a/fetch-object-info.c > > +++ b/fetch-object-info.c > > @@ -39,6 +39,12 @@ int fetch_object_info(const enum protocol_version version, struct object_info_ar > > case protocol_v2: > > if (!server_supports_v2("object-info")) > > die(_("object-info capability is not enabled on the server")); > > + > > + for (int i = args->object_info_options->nr - 1; i >= 0; i--) > > Isn't args->object_info_options->nr of type size_t? We should probably > do something > like: > > for (size_t i = 0; i < args->args->object_info_options->nr; i++) > > instead. Hi! void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util) { if (list->strdup_strings) free(list->items[i].string); if (free_util) free(list->items[i].util); list->items[i] = list->items[list->nr-1]; list->nr--; } I made it backwards because of "list->items[i] = list->items[list->nr - 1];" If we made it from 0..nr and we delete the first element, for the next iteration, the last element is at [0] but we are on [1] and that swapped element never gets evaluated. About size_t, yes, it is size_t but because we go backwards 0 - 1 would fail, also unsorted_string_list_delete_item() signature has "int i". The options that can be on that list will be a small number so there should be no problem, should I cast it explicitly? > > > + if (!server_supports_feature("object-info", > > + args->object_info_options->items[i].string, 0)) > > + unsorted_string_list_delete_item(args->object_info_options, i, 0); > > + > > send_object_info_request(fd_out, args); > > break; > > case protocol_v1: > > > > -- > > 2.54.0 > > Other than these, the patch series LGTM for now. > > Thanks, > Chandra. Thanks, Pablo.