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

On 8/7/2017 2:17 PM, Jonathan Tan wrote:

On Mon, 7 Aug 2017 19:51:04 +0200
Lars Schneider [off-list ref] wrote:

quoted

quoted

On 07 Aug 2017, at 19:21, Jonathan Tan [off-list ref] wrote:

On Sun, 6 Aug 2017 21:58:24 +0200
Lars Schneider [off-list ref] wrote:

quoted

quoted

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

quoted

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

quoted

quoted

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.