Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Aug 7, 2017, 17:21 UTC
- Message-ID
- <20170807102136.30b23023@twelve2.svl.corp.google.com>
- In-Reply-To
- <323E470B-994B-4AD8-9F30-588C2B97A845@gmail.com>
On Sun, 6 Aug 2017 21:58:24 +0200 Lars Schneider <larsxschneider@gmail.com> wrote:
Show 6 quoted lines
> > + 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.
Show 5 quoted lines
> > + > > +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.
Show 12 quoted lines
> > + 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.
Show 6 quoted lines
> > + 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.
Show 6 quoted lines
> > + 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.
Show 6 quoted lines
> > + }
> > + 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.
Show 11 quoted lines
> > +
> > + 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.
Show 8 quoted lines
> > +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.
Show 6 quoted lines
> > + 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.
Show 5 quoted lines
> > + }
> > + 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.
Show 6 quoted lines
> > + 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.
Show 9 quoted lines
> > +/* > > + * Perform the version and capability negotiation as described in the "Long > > + * Running Filter Process" section of the gitattributes documentation using the > > + * given requested versions and capabilities. The "versions" and "capabilities" > > + * parameters are arrays terminated by a 0 or blank struct. > > Should we reference the "gitattributes docs" if we want to make this generic? > I thought "technical/api-sub-process.txt" would explain this kind of stuff > and I was surprised that you deleted it in an earlier patch.
I think this should be done only when we have two users of this, for example, when a patch like [1] (which does contain the change to move away from the gitattributes doc) lands.
[1] https://public-inbox.org/git/eadce97b6a1e80345a2621e71ce187e9e6bc05bf.1501532294.git.jonathantanmy@google.com/
> The generalization of this protocol is nice to see. > > Thanks for working on it, > Lars
Thanks for your comments.