Re: [PATCH] sub-process: refactor handshake to common function

> Christian Couder has been working on a similar refactoring for the perl 
> versions of very similar helper functions.  They can be found in the 
> following patch series:
> 
> https://public-inbox.org/git/20170620075523.26961-1-chriscool@tuxfamily.org/
> 
> In particular:
> 
>      - Patches 02/49 to 08/49 create a Git/Packet.pm module by
>        refactoring "t0021/rot13-filter.pl". Functions from this new
>        module will be used later in test scripts.
> 
> Some differences I noticed: this patch does both the version and 
> capability negotiation and it lives in "sub-process.h/c" files.  In the 
> perl script patch series, the version negotiation is in the new 
> "packet.pm" module but it does not include the capability negotiation.
> 
> It would make sense to me for these to have the same API and usage 
> behaviors.  Perhaps pull them together into a single patch series so 
> that we can review and keep them in sync?

> > It is unfortunate that the code grew larger because it had to be more
> > generic, but I think this is better than having (in the future) 2 or
> > more separate handshake functions.
> 
> I'm always happy to see an effort to refactor common code into reusable 
> APIs.  Its a good engineering practice that makes it easier to 
> stabilize, extend and maintain the code. The challenge is making sure 
> the common function supports all the requirements of the various callers 
> and that the increase in complexity doesn't outweigh the benefit of 
> centralizing the logic.

> > +int subprocess_handshake(struct subprocess_entry *entry,
> > +			 const char *welcome_prefix,
> > +			 int *versions,
> > +			 int *chosen_version,
> > +			 struct subprocess_capability *capabilities,
> > +			 unsigned int *supported_capabilities) {
> > +	int version_scratch;
> > +	unsigned int capabilities_scratch;
> > +	struct child_process *process = &entry->process;
> > +	int i;
> > +	char *line;
> > +	const char *p;
> > +
> > +	if (!chosen_version)
> > +		chosen_version = &version_scratch;
> > +	if (!supported_capabilities)
> > +		supported_capabilities = &capabilities_scratch;
> > +
> > +	sigchain_push(SIGPIPE, SIG_IGN);
> > +
> > +	if (packet_write_fmt_gently(process->in, "%sclient\n",
> > +				    welcome_prefix)) {
> > +		error("Could not write client identification");
> > +		goto error;
> > +	}
> > +	for (i = 0; versions[i]; i++) {
> > +		if (packet_write_fmt_gently(process->in, "version=%d\n",
> > +					    versions[i])) {
> > +			error("Could not write requested version");
> > +			goto error;
> > +		}
> > +	}
> > +	if (packet_flush_gently(process->in))
> > +		goto error;
> > +
> > +	if (!(line = packet_read_line(process->out, NULL)) ||
> > +	    !skip_prefix(line, welcome_prefix, &p) ||
> > +	    strcmp(p, "server")) {
> > +		error("Unexpected line '%s', expected %sserver",
> > +		      line ? line : "<flush packet>", welcome_prefix);
> > +		goto error;
> > +	}
> > +	if (!(line = packet_read_line(process->out, NULL)) ||
> > +	    !skip_prefix(line, "version=", &p) ||
> > +	    strtol_i(p, 10, chosen_version)) {
> > +		error("Unexpected line '%s', expected version",
> > +		      line ? line : "<flush packet>");
> > +		goto error;
> > +	}
> > +	for (i = 0; versions[i]; i++) {
> > +		if (versions[i] == *chosen_version)
> > +			goto version_found;
> > +	}
> 
> This doesn't look like it supports negotiating a common version between 
> the client and server.  If a client supports version 1, 2, and 3 and the 
> server only supports version 1 and 2, which version and capabilities 
> will be used?