Re: [PATCH 1/3] connect: let write_command_and_capabilities() send a promisor-remote reply

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Christian Couder <christian.couder@gmail.com> writes:
>
>>  void write_command_and_capabilities(struct strbuf *req_buf, const char *command,
>> -				    const struct string_list *server_options)
>> +				    const struct string_list *server_options,
>> +				    const char *promisor_remote_reply)
>>  {
>>  	const char *hash_name;
>>  	int advertise_sid = 0;
>> @@ -754,6 +755,9 @@ void write_command_and_capabilities(struct strbuf *req_buf, const char *command,
>>  		die(_("the server does not support algorithm '%s'"),
>>  		    the_hash_algo->name);
>>  	}
>> +	if (promisor_remote_reply)
>> +		packet_buf_write(req_buf, "promisor-remote=%s",
>> +				 promisor_remote_reply);
>>  	packet_buf_delim(req_buf);
>>  }
>
> This function was designed to have a fairly generic "here we send a
> command and then capability exchange based on the settings stored in
> the string list" interface for extensibility.  It is highly annoying
> that we are now adding a specific option here to be sent outside the
> framework.
>
> Isn't the misdesign being corrected that the "promisor-remote=..."
> string was not sent as part of these server options list of strings?
> Can we fix it without contaminating the API like this patch does, or
> is it too late?

By this I do not mean that promisor-remote=... has to be sent as one
of the server options (i.e., making the string sent to something
silly like "server-option=promisor-remote=...").  Rather, I am
wondering if the existing server_options string list should be made
into a more generalized form so that the string list can contain
elements whose keys other than "server-option" can also be
contained (like "promisor-remote").  IOW, the caller can decide to
put "promisor-remote=...promisor remote reply..." in the list,
instead of sending it as a totally separate parameter, whose
semantics is hardcoded (i.e. the fourth parameter to this function
will be sent as a value to "promistor-remote=...").