Merge branch 'jt/subprocess-handshake'

Code cleanup.

* jt/subprocess-handshake:
  sub-process: refactor handshake to common function
  Documentation: migrate sub-process docs to header

Documentation/technical/api-sub-process.txt

@@ -1,59 +0,0 @@
-sub-process API
-===============
-
-The sub-process API makes it possible to run background sub-processes
-for the entire lifetime of a Git invocation. If Git needs to communicate
-with an external process multiple times, then this can reduces the process
-invocation overhead. Git and the sub-process communicate through stdin and
-stdout.
-
-The sub-processes are kept in a hashmap by command name and looked up
-via the subprocess_find_entry function.  If an existing instance can not
-be found then a new process should be created and started.  When the
-parent git command terminates, all sub-processes are also terminated.
-
-This API is based on the run-command API.
-
-Data structures
----------------
-
-* `struct subprocess_entry`
-
-The sub-process structure.  Members should not be accessed directly.
-
-Types
------
-
-'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::
-
-	User-supplied function to initialize the sub-process.  This is
-	typically used to negotiate the interface version and capabilities.
-
-
-Functions
----------
-
-`cmd2process_cmp`::
-
-	Function to test two subprocess hashmap entries for equality.
-
-`subprocess_start`::
-
-	Start a subprocess and add it to the subprocess hashmap.
-
-`subprocess_stop`::
-
-	Kill a subprocess and remove it from the subprocess hashmap.
-
-`subprocess_find_entry`::
-
-	Find a subprocess in the subprocess hashmap.
-
-`subprocess_get_child_process`::
-
-	Get the underlying `struct child_process` from a subprocess.
-
-`subprocess_read_status`::
-
-	Helper function to read packets looking for the last "status=<foo>"
-	key/value pair.

convert.c

@@ -513,78 +513,17 @@ static struct hashmap subprocess_map;
 
 static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
 {
-	int err, i;
+	static int versions[] = {2, 0};
-	struct cmd2process *entry = (struct cmd2process *)subprocess;
+	static struct subprocess_capability capabilities[] = {
-	struct string_list cap_list = STRING_LIST_INIT_NODUP;
-	char *cap_buf;
-	const char *cap_name;
-	struct child_process *process = &subprocess->process;
-	const char *cmd = subprocess->cmd;
-
-	static const struct {
-		const char *name;
-		unsigned int cap;
-	} known_caps[] = {
 		{ "clean",  CAP_CLEAN  },
 		{ "smudge", CAP_SMUDGE },
 		{ "delay",  CAP_DELAY  },
+		{ NULL, 0 }
 	};
-
+	struct cmd2process *entry = (struct cmd2process *)subprocess;
-	sigchain_push(SIGPIPE, SIG_IGN);
+	return subprocess_handshake(subprocess, "git-filter", versions, NULL,
-
+				    capabilities,
-	err = packet_writel(process->in, "git-filter-client", "version=2", NULL);
+				    &entry->supported_capabilities);
-	if (err)
-		goto done;
-
-	err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
-	if (err) {
-		error("external filter '%s' does not support filter protocol version 2", cmd);
-		goto done;
-	}
-	err = strcmp(packet_read_line(process->out, NULL), "version=2");
-	if (err)
-		goto done;
-	err = packet_read_line(process->out, NULL) != NULL;
-	if (err)
-		goto done;
-
-	for (i = 0; i < ARRAY_SIZE(known_caps); ++i) {
-		err = packet_write_fmt_gently(
-			process->in, "capability=%s\n", known_caps[i].name);
-		if (err)
-			goto done;
-	}
-	err = packet_flush_gently(process->in);
-	if (err)
-		goto done;
-
-	for (;;) {
-		cap_buf = packet_read_line(process->out, NULL);
-		if (!cap_buf)
-			break;
-		string_list_split_in_place(&cap_list, cap_buf, '=', 1);
-
-		if (cap_list.nr != 2 || strcmp(cap_list.items[0].string, "capability"))
-			continue;
-
-		cap_name = cap_list.items[1].string;
-		i = ARRAY_SIZE(known_caps) - 1;
-		while (i >= 0 && strcmp(cap_name, known_caps[i].name))
-			i--;
-
-		if (i >= 0)
-			entry->supported_capabilities |= known_caps[i].cap;
-		else
-			warning("external filter '%s' requested unsupported filter capability '%s'",
-			cmd, cap_name);
-
-		string_list_clear(&cap_list, 0);
-	}
-
-done:
-	sigchain_pop(SIGPIPE);
-
-	return err;
 }
 
 static void handle_filter_error(const struct strbuf *filter_status,

pkt-line.c

@@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
 	return status;
 }
 
-int packet_writel(int fd, const char *line, ...)
-{
-	va_list args;
-	int err;
-	va_start(args, line);
-	for (;;) {
-		if (!line)
-			break;
-		if (strlen(line) > LARGE_PACKET_DATA_MAX)
-			return -1;
-		err = packet_write_fmt_gently(fd, "%s\n", line);
-		if (err)
-			return err;
-		line = va_arg(args, const char*);
-	}
-	va_end(args);
-	return packet_flush_gently(fd);
-}
-
 static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 {
 	static char packet_write_buffer[LARGE_PACKET_MAX];

pkt-line.h

@@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);
 void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
-LAST_ARG_MUST_BE_NULL
-int packet_writel(int fd, const char *line, ...);
 int write_packetized_from_fd(int fd_in, int fd_out);
 int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
 

sub-process.c

@@ -108,3 +108,107 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
 	hashmap_add(hashmap, entry);
 	return 0;
 }
+
+static int handshake_version(struct child_process *process,
+			     const char *welcome_prefix, int *versions,
+			     int *chosen_version)
+{
+	int version_scratch;
+	int i;
+	char *line;
+	const char *p;
+
+	if (!chosen_version)
+		chosen_version = &version_scratch;
+
+	if (packet_write_fmt_gently(process->in, "%s-client\n",
+				    welcome_prefix))
+		return error("Could not write client identification");
+	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");
+	}
+	if (packet_flush_gently(process->in))
+		return error("Could not write flush packet");
+
+	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))
+		return error("Unexpected line '%s', expected version",
+			     line ? line : "<flush packet>");
+	if ((line = packet_read_line(process->out, NULL)))
+		return error("Unexpected line '%s', expected flush", line);
+
+	/* Check to make sure that the version received is supported */
+	for (i = 0; versions[i]; i++) {
+		if (versions[i] == *chosen_version)
+			break;
+	}
+	if (!versions[i])
+		return error("Version %d not supported", *chosen_version);
+
+	return 0;
+}
+
+static int handshake_capabilities(struct child_process *process,
+				  struct subprocess_capability *capabilities,
+				  unsigned int *supported_capabilities)
+{
+	int i;
+	char *line;
+
+	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");
+	}
+	if (packet_flush_gently(process->in))
+		return error("Could not write flush packet");
+
+	while ((line = packet_read_line(process->out, NULL))) {
+		const char *p;
+		if (!skip_prefix(line, "capability=", &p))
+			continue;
+
+		for (i = 0;
+		     capabilities[i].name && strcmp(p, capabilities[i].name);
+		     i++)
+			;
+		if (capabilities[i].name) {
+			if (supported_capabilities)
+				*supported_capabilities |= capabilities[i].flag;
+		} else {
+			warning("external filter requested unsupported filter capability '%s'",
+				p);
+		}
+	}
+
+	return 0;
+}
+
+int subprocess_handshake(struct subprocess_entry *entry,
+			 const char *welcome_prefix,
+			 int *versions,
+			 int *chosen_version,
+			 struct subprocess_capability *capabilities,
+			 unsigned int *supported_capabilities)
+{
+	int retval;
+	struct child_process *process = &entry->process;
+
+	sigchain_push(SIGPIPE, SIG_IGN);
+
+	retval = handshake_version(process, welcome_prefix, versions,
+				   chosen_version) ||
+		 handshake_capabilities(process, capabilities,
+					supported_capabilities);
+
+	sigchain_pop(SIGPIPE);
+	return retval;
+}

sub-process.h

@@ -6,41 +6,88 @@
 #include "run-command.h"
 
 /*
- * Generic implementation of background process infrastructure.
+ * The sub-process API makes it possible to run background sub-processes
- * See: Documentation/technical/api-sub-process.txt
+ * for the entire lifetime of a Git invocation. If Git needs to communicate
+ * with an external process multiple times, then this can reduces the process
+ * invocation overhead. Git and the sub-process communicate through stdin and
+ * stdout.
+ *
+ * The sub-processes are kept in a hashmap by command name and looked up
+ * via the subprocess_find_entry function.  If an existing instance can not
+ * be found then a new process should be created and started.  When the
+ * parent git command terminates, all sub-processes are also terminated.
+ *
+ * This API is based on the run-command API.
  */
 
  /* data structures */
 
+/* Members should not be accessed directly. */
 struct subprocess_entry {
 	struct hashmap_entry ent; /* must be the first member! */
 	const char *cmd;
 	struct child_process process;
 };
 
+struct subprocess_capability {
+	const char *name;
+
+	/*
+	 * subprocess_handshake will "|=" this value to supported_capabilities
+	 * if the server reports that it supports this capability.
+	 */
+	unsigned int flag;
+};
+
 /* subprocess functions */
 
+/* Function to test two subprocess hashmap entries for equality. */
 extern int cmd2process_cmp(const void *unused_cmp_data,
 			   const void *e1,
 			   const void *e2,
 			   const void *unused_keydata);
 
+/*
+ * User-supplied function to initialize the sub-process.  This is
+ * typically used to negotiate the interface version and capabilities.
+ */
 typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
+
+/* Start a subprocess and add it to the subprocess hashmap. */
 int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,
 		subprocess_start_fn startfn);
 
+/* Kill a subprocess and remove it from the subprocess hashmap. */
 void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry);
 
+/* Find a subprocess in the subprocess hashmap. */
 struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const char *cmd);
 
 /* subprocess helper functions */
 
+/* Get the underlying `struct child_process` from a subprocess. */
 static inline struct child_process *subprocess_get_child_process(
 		struct subprocess_entry *entry)
 {
 	return &entry->process;
 }
 
+/*
+ * 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.
+ *
+ * This function is typically called when a subprocess is started (as part of
+ * the "startfn" passed to subprocess_start).
+ */
+int subprocess_handshake(struct subprocess_entry *entry,
+			 const char *welcome_prefix,
+			 int *versions,
+			 int *chosen_version,
+			 struct subprocess_capability *capabilities,
+			 unsigned int *supported_capabilities);
+
 /*
  * Helper function that will read packets looking for "status=<foo>"
  * key/value pairs and return the value from the last "status" packet

t/t0021-conversion.sh

@@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '
 
 		cp "$TEST_ROOT/test.o" test.r &&
 		test_must_fail git add . 2>git-stderr.log &&
-		grep "does not support filter protocol version" git-stderr.log
+		grep "expected git-filter-server" git-stderr.log
 	)
 '