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

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

>>> 
>>> +static int handshake_version(struct child_process *process,
>>> +			     const char *welcome_prefix, int *versions,
>> 
>> Maybe it would be less ambiguous if we call it `supported_versions` ? 
> 
> I thought of that, but I think "supported_versions" is actually more
> ambiguous, since we don't know if these are versions supported by the
> server or client or both.

>>> +			     int *chosen_version)
>>> +{
>>> +	int version_scratch;
>>> +	int i;
>>> +	char *line;
>>> +	const char *p;
>>> +
>>> +	if (!chosen_version)
>>> +		chosen_version = &version_scratch;
>> 
>> I am not an C expert but wouldn't 'version_scratch' go out of scope as soon
>> as the function returns? Why don't you error here right away?
> 
> It does, but so does chosen_version. This is meant to allow the caller
> to pass NULL to this function.

>>> +	if (packet_write_fmt_gently(process->in, "%s-client\n",
>>> +				    welcome_prefix))
>>> +		return error("Could not write client identification");
>> 
>> Nit: Would it make sense to rename `welcome_prefix` to `client_id`?
>> Alternatively, could we rename the error messages to "welcome prefix"?
> 
> I was retaining the existing terminology, but your suggestions seem
> reasonable. This might be best done in another patch once this series
> lands in master, though.

>>> +	for (i = 0; versions[i]; i++) {
>>> +		if (packet_write_fmt_gently(process->in, "version=%d\n",
>>> +					    versions[i]))
>>> +			return error("Could not write requested version");
>> 
>> Maybe: "Could not write supported versions"?
> 
> Same as above - "supported" is ambiguous.
> 
>>> +	}
>>> +	if (packet_flush_gently(process->in))
>>> +		return error("Could not write flush packet");
>> 
>> I feel this error is too generic.
>> Maybe: "Could not finish writing supported versions"?
> 
> That's reasonable. This is a rare error, though, and if it does occur, I
> think this message is more informative. But I'm OK either way.

> 
>>> +
>>> +	if (!(line = packet_read_line(process->out, NULL)) ||
>>> +	    !skip_prefix(line, welcome_prefix, &p) ||
>>> +	    strcmp(p, "-server"))
>>> +		return error("Unexpected line '%s', expected %s-server",
>>> +			     line ? line : "<flush packet>", welcome_prefix);
>>> +	if (!(line = packet_read_line(process->out, NULL)) ||
>>> +	    !skip_prefix(line, "version=", &p) ||
>>> +	    strtol_i(p, 10, chosen_version))
>> 
>> Maybe `strlen("version=")` would be more clear than 10?
> 
> The 10 here is the base, not the length. If there's a better way to
> convert strings to integers, let me know.

> 
>>> +		return error("Unexpected line '%s', expected version",
>> 
>> Maybe "... expected version number" ?
> 
> I'm fine either way.
> 
>>> +static int handshake_capabilities(struct child_process *process,
>>> +				  struct subprocess_capability *capabilities,
>>> +				  unsigned int *supported_capabilities)
>> 
>> I feel the naming could be misleading. I think ...
>> `capabilities` is really `supported_capabilities` 
>> and 
>> `supported_capabilities` is really `negiotated_capabilties` or `agreed_capabilites`
> 
> These "supported capabilities" are those supported by both the client
> (Git) and the server (the process Git is invoking). I think it's better
> to use this term for the intersection of capabilities, rather than
> exclusively for the client or server.
> 
>>> +	for (i = 0; capabilities[i].name; i++) {
>>> +		if (packet_write_fmt_gently(process->in, "capability=%s\n",
>>> +					    capabilities[i].name))
>>> +			return error("Could not write requested capability");
>> 
>> I think this should be "Could not write supported capability", no?
> 
> Same comment as above.
> 
>>> +	}
>>> +	if (packet_flush_gently(process->in))
>>> +		return error("Could not write flush packet");
>> 
>> Maybe " "Could not finish writing supported capability" ?
> 
> Same comment as the one about writing flush packets above.
> 
>>> +	while ((line = packet_read_line(process->out, NULL))) {
>>> +		const char *p;
>>> +		if (!skip_prefix(line, "capability=", &p))
>>> +			continue;
>> 
>> Shouldn't we write an error in this case?
> 
> I'm preserving the existing behavior.