Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function
- From
Ben Peart <peartben@gmail.com>
- Date
- Aug 7, 2017, 18:29 UTC
- Message-ID
- <142c1757-d366-61fe-ffa8-a398bdfd8c03@gmail.com>
- In-Reply-To
- <20170807111708.2d745df2@twelve2.svl.corp.google.com>
On 8/7/2017 2:17 PM, Jonathan Tan wrote:
Show 25 quoted lines
> 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.
Show 25 quoted lines
>> @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. >