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

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

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.

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.

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.

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.

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

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

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.

quoted

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

Maybe "... expected version number" ?

I'm fine either way.

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

+	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

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

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

quoted

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