On Tue, 25 Jul 2017 10:38:51 -0400 Ben Peart <peartben@xxxxxxxxx> wrote: > 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@xxxxxxxxxxxxx/ > > 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? Thanks for the pointer. These are different languages and different use cases (mine for production, his for tests), though, so I don't think consistency in API and behavior is practical. > > 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. Agreed - thanks. > > +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? In the protocol, the client sends a list of versions it supports (see the "for" loop above), and expects the server to write the single version that the server chooses (see the part where we read one single version line). In your example, the client would write "version=1\n", "version=2\n", and "version=3\n", and the server would write "version=2\n" (well, it could write "version=1\n" too). So version 2 (or 1) will be used.
