From: Chandra Pratap Date: Tue, 09 Jun 2026 15:32:13 GMT Subject: Re: [PATCH GSoC RFC v12 12/12] cat-file: make remote-object-info allow-list dynamic Message-ID: In-Reply-To: <20260608-ps-eric-work-rebase-v12-12-5338b766e658@gmail.com> 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(-) > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index 1166a046b4..055991b5af 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -341,13 +341,10 @@ struct expand_data { > * Flags about when an object info is being fetched from remote. > */ > unsigned is_remote:1; > -}; > -#define EXPAND_DATA_INIT { .mode = S_IFINVALID, .type = OBJ_BAD } > > -static const char *remote_object_info_atoms[] = { > - "objectname", > - "objectsize", > + struct string_list remote_allowed_atoms; > }; > +#define EXPAND_DATA_INIT { .mode = S_IFINVALID, .type = OBJ_BAD, .remote_allowed_atoms = STRING_LIST_INIT_NODUP } > > static int is_atom(const char *atom, const char *s, int slen) > { > @@ -359,17 +356,11 @@ static int expand_atom(struct strbuf *sb, const char *atom, int len, > struct expand_data *data) > { > if (data->is_remote) { > - size_t i, allowed_nr = ARRAY_SIZE(remote_object_info_atoms); > - for (i = 0; i < allowed_nr; i++) > - if (is_atom(remote_object_info_atoms[i], atom, len)) > + size_t i; > + for (i = 0; i < data->remote_allowed_atoms.nr; i++) > + if (is_atom(data->remote_allowed_atoms.items[i].string, atom, len)) > break; > - > - /* > - * On remote, skip unsupported atoms returning an empty sb, > - * honoring how for-each-ref handles known but inapplicable > - * atoms (e.g. %(tagger)). > - */ > - if (i == allowed_nr) > + if (i == data->remote_allowed_atoms.nr) > return 1; > } > > @@ -686,12 +677,12 @@ static int get_remote_info(struct batch_options *opt, > int argc, > const char **argv, > struct object_info **remote_object_info, > - struct oid_array *object_info_oids) > + struct oid_array *object_info_oids, > + struct string_list *object_info_options) > { > int retval = 0; > struct remote *remote = NULL; > struct object_id oid; > - struct string_list object_info_options = STRING_LIST_INIT_NODUP; > static struct transport *gtransport; > > /* > @@ -726,15 +717,12 @@ static int get_remote_info(struct batch_options *opt, > gtransport->smart_options->object_info = 1; > gtransport->smart_options->object_info_oids = object_info_oids; > > - string_list_append(&object_info_options, "size"); > - > - if (object_info_options.nr > 0) { > - gtransport->smart_options->object_info_options = &object_info_options; > + if (object_info_options->nr > 0) { > + gtransport->smart_options->object_info_options = object_info_options; > gtransport->smart_options->object_info_data = *remote_object_info; > retval = transport_fetch_refs(gtransport, NULL); > } > cleanup: > - string_list_clear(&object_info_options, 0); > transport_disconnect(gtransport); > return retval; > } > @@ -820,6 +808,21 @@ static void parse_cmd_mailmap(struct batch_options *opt UNUSED, > load_mailmap(); > } > > +struct protocol_placeholder_entry { > + const char *option; > + const char *atom; > +}; > + > +static const struct protocol_placeholder_entry remote_atom_map[] = { > + {"size", "objectsize"}, > + {"type", "objecttype"}, > + /* > + * Add new protocol options here. Even if the server doesn't support > + * them the allow_list will drop them if the server doesn't advertise > + * them. > + */ > +}; > + > static void parse_cmd_remote_object_info(struct batch_options *opt, > const char *line, struct strbuf *output, > struct expand_data *data) > @@ -829,6 +832,7 @@ static void parse_cmd_remote_object_info(struct batch_options *opt, > char *line_to_split; > static struct object_info *remote_object_info; > static struct oid_array object_info_oids = OID_ARRAY_INIT; > + struct string_list object_info_options = STRING_LIST_INIT_NODUP; > > if (strlen(line) >= MAX_REMOTE_OBJ_INFO_LINE) > die(_("remote-object-info command too long")); > @@ -841,30 +845,44 @@ static void parse_cmd_remote_object_info(struct batch_options *opt, > die(_("remote-object-info supports at most %d objects"), > MAX_ALLOWED_OBJ_LIMIT); > > + if (data->info.sizep) > + string_list_append(&object_info_options, "size"); > + if (data->info.typep) > + string_list_append(&object_info_options, "type"); > + > if (get_remote_info(opt, count, argv, &remote_object_info, > - &object_info_oids)) > + &object_info_oids, &object_info_options)) > goto cleanup; > > + string_list_clear(&data->remote_allowed_atoms, 0); > + string_list_append(&data->remote_allowed_atoms, "objectname"); > + for (size_t i = 0; i < ARRAY_SIZE(remote_atom_map); i++) > + if (unsorted_string_list_has_string(&object_info_options, remote_atom_map[i].option)) > + string_list_append(&data->remote_allowed_atoms, > + remote_atom_map[i].atom); > + > data->skip_object_info = 1; > for (size_t i = 0; i < object_info_oids.nr; i++) { > data->oid = object_info_oids.oid[i]; > - if (remote_object_info[i].sizep) { > - /* > - * When reaching here, it means remote-object-info can retrieve > - * information from server without downloading them. > - */ > + /* > + * When reaching here, it means remote-object-info can retrieve > + * information from server without downloading them. > + */ > + if (remote_object_info[i].sizep) > data->size = *remote_object_info[i].sizep; > - opt->batch_mode = BATCH_MODE_INFO; > - data->is_remote = 1; > - batch_object_write(argv[i + 1], output, opt, data, NULL, 0); > - data->is_remote = 0; > - } > + if (remote_object_info[i].typep) > + data->type = *remote_object_info[i].typep; > + opt->batch_mode = BATCH_MODE_INFO; > + data->is_remote = 1; > + batch_object_write(argv[i + 1], output, opt, data, NULL, 0); > + data->is_remote = 0; > } > data->skip_object_info = 0; > > cleanup: > for (size_t i = 0; i < object_info_oids.nr; i++) > free_object_info_contents(&remote_object_info[i]); > + string_list_clear(&object_info_options, 0); > free(line_to_split); > free(argv); > free(remote_object_info); > @@ -1177,6 +1195,7 @@ static int batch_objects(struct batch_options *opt) > cleanup: > strbuf_release(&input); > strbuf_release(&output); > + string_list_clear(&data.remote_allowed_atoms, 0); > warn_on_object_refname_ambiguity = save_warning; > return retval; > } > 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. > + 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.