Re: [PATCH GSoC v18 13/13] cat-file: make remote-object-info allow-list dynamic

["Pablo Sabater" <pabloosabaterr@gmail.com>]

On Wed Jul 15, 2026 at 7:56 PM CEST, Junio C Hamano wrote:
> Pablo Sabater <pabloosabaterr@gmail.com> writes:
>
>> 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 the server advertises.
>>
>> The client will now:
>>
>> 1. Request the protocol option that the placeholder refers to (i.e.
>>    "size" when "%(objectsize)").
>
> "when" -> "for"?

Will change.

>
>>
>> 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.
>
> I am not sure I follow the above entirely.  Could you add a
> concrete example to the commit message?
>
> For instance, if the client wants "%(objectsize) %(objectcolor)" and
> the server only supports 'size' but not 'color', the filtering in
> step (2) prevents the client from asking about the color, requesting
> only the size instead.  When the server says the size is 42, step (3)
> uses that to substitute '%(objectsize)'.  Would the end result then
> be "42 %(objectcolor)"?

You've gotten everything right until the last step, because we have only
size from the server there is no data to match %(objectcolor) and the
end result is an empty string for %(objeccolor):

"42 "

Note that %(objectcolor) doesn't exists and it would have die(), the
empty string is only for known but unsupported placeholders.

This is what for-each-ref does for known but unaplicable placeholders (atoms).

I'll add an example to the commmit log so it is clearer.

>
>> -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 }
>
> Hmph, is this list expected to change over time?  One-line-per-item
> format would be more suited for updates if it is the case.

Will format like so.

>
>> @@ -683,12 +675,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;
>>  	struct transport *gtransport;
>>
>>  	/*
>> @@ -736,15 +728,12 @@ static int get_remote_info(struct batch_options *opt,
>>  	CALLOC_ARRAY(*remote_object_info, object_info_oids->nr);
>>  	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_object_info(gtransport);
>>  	}
>
> This is not a new issue, but if the caller does not ask for
> anything in object_info_options, no call to
> transport_fetch_object_info() is made here.  This is so even
> though we went through quite a lot of work, including the
> connection establishment and teardown below.
>
> By failing to contact the remote side, we wouldn't even know if
> the objects being queried actually exist there, which is
> probably even worse.

[Answered below]

>
>>  static void parse_cmd_remote_object_info(struct batch_options *opt,
>>  					 const char *line, struct strbuf *output,
>>  					 struct expand_data *data)
>> @@ -839,6 +843,7 @@ static void parse_cmd_remote_object_info(struct batch_options *opt,
>>  	char *line_to_split;
>>  	struct object_info *remote_object_info = NULL;
>>  	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"));
>> @@ -851,32 +856,57 @@ 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");
>
> And if the request is only for "%(objectname)", an empty
> object_info_options is given to get_remote_info().

Right now 'name' is not part of the protocol as 'type' or 'size' are,
'objectname' is always allowed but only shown if it's present on the
format.
If the format is only "%(objectname)" then there's nothing to ask the
server for.

The current code avoids making the request if there's only objectname or
nothing supported, but still goes through the connection work. I will
add an early return to just output the oid back without any connection.

Returning the oid is what the client expects for which we don't need a
connection, but this means that we skip the existence check that
we would get if we asked for size.

The server is not ready to support a bare oid for existence check, it
could be done but that work belongs to a future series.

>
>>  	if (get_remote_info(opt, count, argv, &remote_object_info,
>> -			    &object_info_oids))
>> +			    &object_info_oids, &object_info_options))
>>  		goto cleanup;

Regards,
Pablo