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

> 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"?

>
>>
>> 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)"?

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

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

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

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