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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 7, 2017, 18:17 UTC
Message-ID: <20170807111708.2d745df2@twelve2.svl.corp.google.com>
In-Reply-To: <2BEA5CEF-44C9-45DC-A462-579864790EDB@gmail.com>

On Mon, 7 Aug 2017 19:51:04 +0200 Lars Schneider <larsxschneider@gmail.com> wrote:

Show 23 quoted lines

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

Show 8 quoted lines

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