On Sun, 6 Aug 2017 21:58:24 +0200
Lars Schneider <larsxschneider@xxxxxxxxx> 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.
> > +/*
> > + * 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@xxxxxxxxxx/
> The generalization of this protocol is nice to see.
>
> Thanks for working on it,
> Lars
Thanks for your comments.
