Re: [PATCH for NEXT v3 2/2] sub-process: refactor handshake to common function

On 07 Aug 2017, at 19:21, Jonathan Tan [off-list ref] wrote:

On Sun, 6 Aug 2017 21:58:24 +0200
Lars Schneider [off-list ref] wrote:

quoted

quoted

+	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"

quoted

quoted

+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 ;-)

quoted

quoted

+			     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.

quoted

quoted

+	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 :-(

quoted

quoted

+	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.

quoted

quoted

+	}
+	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.

quoted

quoted

+
+	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 :-)

quoted

quoted

+		return error("Unexpected line '%s', expected version",

Maybe "... expected version number" ?

I'm fine either way.

quoted

quoted

+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.

quoted

quoted

+	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.

quoted

quoted

+	}
+	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.

quoted

quoted

+	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