Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function

[Ben Peart <peartben@gmail.com>]

On 8/7/2017 2:17 PM, Jonathan Tan wrote:
> On Mon, 7 Aug 2017 19:51:04 +0200
> Lars Schneider <larsxschneider@gmail.com> wrote:
> 
>>
>>> On 07 Aug 2017, at 19:21, Jonathan Tan <jonathantanmy@google.com> wrote:
>>>
>>> On Sun, 6 Aug 2017 21:58:24 +0200
>>> Lars Schneider <larsxschneider@gmail.com> wrote:
>>>
>>>>> +	struct cmd2process *entry = (struct cmd2process *)subprocess;
>>>>> +	return subprocess_handshake(subprocess, "git-filter", versions, NULL,
>>>>> +				    capabilities,
>>>>> +				    &entry->supported_capabilities);
>>>>
>>>> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?
>>>
>>> The members of "struct subprocess_entry" are not supposed to be accessed
>>> directly, according to the documentation. If we relaxed that, then we
>>> could do this, but before that I think it's better to let the caller
>>> handle it.
>>
>> @Ben: You wrote that " Members should not be accessed directly.":
>> https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22
>> Can you give me a hint why?
>>

It's just good object oriented design of providing a layer of 
abstraction between the implementation details and the use of the 
class/object/API.  I was following the model in api-hashmap.txt but 
there are many other examples of where we don't do this.

Perhaps providing a function that returns the property you want to 
access (similar to subprocess_get_child_process) would work.

>> @Jonathan: What do you mean by "it's better to let the caller handle it"
> 
> Let the caller provide their own place to store the capabilities, I
> mean, instead of (say) using a field as you describe and an accessor
> method.
> 
> I don't feel strongly about this, though.
> 
>>> It does, but so does chosen_version. This is meant to allow the caller
>>> to pass NULL to this function.
>>
>> Hm. I think every protocol should be versioned otherwise we could run
>> into trouble in the long run.
>>
>> TBH I wouldn't support NULL in that case in the first place. If you
>> want to support it then I think we should document it.
> 
> Note that this NULL is for the chosen version as chosen by the server,
> not the versions declared as supported by the client.
> 
> The protocol is versioned. Some users (e.g. the filter mechanism) of
> this subprocess thing would want to pass NULL because they only support
> one version and the subprocess thing already ensures that the server
> report that it supports one of the versions sent.
>