> On 07 Aug 2017, at 19:21, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote: > > 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. @Ben: You wrote that " Members should not be accessed directly.": https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22 Can you give me a hint why? @Jonathan: What do you mean by "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. True! Maybe `versions_supported_by_git` to annoy people that hate long variable names ;-) >>> + 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. Hm. I think every protocol should be versioned otherwise we could run into trouble in the long run. TBH I wouldn't support NULL in that case in the first place. If you want to support it then I think we should document it. >>> + 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. Yeah, sorry for my late review :-( >>> + 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. My thinking is this: if I see an error then I want to roughly know what went wrong and I want to have a good chance to find the error in the source. The "Could not write flush packet" is technically correct but it makes it harder to pinpoint the error in the source as we throw it in several places. > >>> + >>> + 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. Argh, of course! Sorry! To my defense: it was late last night :-) > >>> + 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. You're right: https://github.com/git/git/blob/4384e3cde2ce8ecd194202e171ae16333d241326/convert.c#L549-L550 - Lars
