Re: [PATCH 4/4] Add 'filter' attribute and external filter driver definition.

["Shawn O. Pearce" <spearce@spearce.org>]

Junio C Hamano <junkio@cox.net> wrote:
> The interface is similar to the custom low-level merge drivers.
... 
> +static int filter_buffer(const char *path, const char *src,
> +			 unsigned long size, const char *cmd)
> +{
> +	/*
> +	 * Spawn cmd and feed the buffer contents through its stdin.
> +	 */
...

ick.  What about something like this on top?  I moved the extra child
process for the input pipe down into the start_command routine,
where we can do something a little smarter on some systems, like
using a thread rather than a full process.  Its also a shorter
patch and uses more of the run-command API.

Its not documented very well, but if you set child_process.{in,out}
to <0 we open the pipe for you, and close your side of the pipe
for you.  That really simplifies the logic in the callers.

I did consider rewriting this as a select() based loop to feed the
input and read the output, but that might not port well onto a more
native Win32 based set of APIs.

---
diff --git a/convert.c b/convert.c
index 62d8cee..2ba7ea3 100644
--- a/convert.c
+++ b/convert.c
@@ -201,99 +201,37 @@ static char *crlf_to_worktree(const char *path, const char *src, unsigned long *
 	return buffer;
 }
 
-static int filter_buffer(const char *path, const char *src,
-			 unsigned long size, const char *cmd)
-{
-	/*
-	 * Spawn cmd and feed the buffer contents through its stdin.
-	 */
-	struct child_process child_process;
-	int pipe_feed[2];
-	int write_err, status;
-
-	memset(&child_process, 0, sizeof(child_process));
-
-	if (pipe(pipe_feed) < 0) {
-		error("cannot create pipe to run external filter %s", cmd);
-		return 1;
-	}
-
-	child_process.pid = fork();
-	if (child_process.pid < 0) {
-		error("cannot fork to run external filter %s", cmd);
-		close(pipe_feed[0]);
-		close(pipe_feed[1]);
-		return 1;
-	}
-	if (!child_process.pid) {
-		dup2(pipe_feed[0], 0);
-		close(pipe_feed[0]);
-		close(pipe_feed[1]);
-		execlp(cmd, cmd, NULL);
-		return 1;
-	}
-	close(pipe_feed[0]);
-
-	write_err = (write_in_full(pipe_feed[1], src, size) < 0);
-	if (close(pipe_feed[1]))
-		write_err = 1;
-	if (write_err)
-		error("cannot feed the input to external filter %s", cmd);
-
-	status = finish_command(&child_process);
-	if (status)
-		error("external filter %s failed %d", cmd, -status);
-	return (write_err || status);
-}
-
 static char *apply_filter(const char *path, const char *src,
 			  unsigned long *sizep, const char *cmd)
 {
-	/*
-	 * Create a pipeline to have the command filter the buffer's
-	 * contents.
-	 *
-	 * (child --> cmd) --> us
-	 */
 	const int SLOP = 4096;
-	int pipe_feed[2];
+	struct child_process filter_process;
+	const char *filter_argv[] = {cmd, NULL};
 	int status;
 	char *dst;
 	unsigned long dstsize, dstalloc;
-	struct child_process child_process;
 
 	if (!cmd)
 		return NULL;
 
-	memset(&child_process, 0, sizeof(child_process));
-
-	if (pipe(pipe_feed) < 0) {
-		error("cannot create pipe to run external filter %s", cmd);
-		return NULL;
-	}
-
-	child_process.pid = fork();
-	if (child_process.pid < 0) {
-		error("cannot fork to run external filter %s", cmd);
-		close(pipe_feed[0]);
-		close(pipe_feed[1]);
+	memset(&filter_process, 0, sizeof(filter_process));
+	filter_process.in_bufptr = src;
+	filter_process.in_buflen = *sizep;
+	filter_process.out = -1;
+	filter_process.argv = filter_argv;
+	status = start_command(&filter_process);
+	if (status) {
+		error("external filter %s failed %d", cmd, -status);
 		return NULL;
 	}
-	if (!child_process.pid) {
-		dup2(pipe_feed[1], 1);
-		close(pipe_feed[0]);
-		close(pipe_feed[1]);
-		exit(filter_buffer(path, src, *sizep, cmd));
-	}
-	close(pipe_feed[1]);
 
 	dstalloc = *sizep;
 	dst = xmalloc(dstalloc);
 	dstsize = 0;
 
 	while (1) {
-		ssize_t numread = xread(pipe_feed[0], dst + dstsize,
-					dstalloc - dstsize);
+		ssize_t numread = xread(filter_process.out,
+			dst + dstsize, dstalloc - dstsize);
 
 		if (numread <= 0) {
 			if (!numread)
@@ -310,7 +248,7 @@ static char *apply_filter(const char *path, const char *src,
 		}
 	}
 
-	status = finish_command(&child_process);
+	status = finish_command(&filter_process);
 	if (status) {
 		error("external filter %s failed %d", cmd, -status);
 		free(dst);
diff --git a/run-command.c b/run-command.c
index eff523e..72887f8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -20,12 +20,29 @@ int start_command(struct child_process *cmd)
 	int need_in, need_out;
 	int fdin[2], fdout[2];
 
-	need_in = !cmd->no_stdin && cmd->in < 0;
+	need_in = !cmd->no_stdin && (cmd->in_bufptr || cmd->in < 0);
 	if (need_in) {
 		if (pipe(fdin) < 0)
 			return -ERR_RUN_COMMAND_PIPE;
-		cmd->in = fdin[1];
-		cmd->close_in = 1;
+		if (cmd->in_bufptr) {
+			pid_t in_feeder = fork();
+			if (in_feeder < 0) {
+				close_pair(fdin);
+				return -ERR_RUN_COMMAND_PIPE;
+			}
+			if (!in_feeder) {
+				close(fdin[0]);
+				if (write_in_full(fdin[1],
+					cmd->in_bufptr,
+					cmd->in_buflen) != cmd->in_buflen)
+					die("'%s' did not read all input", cmd->argv[0]);
+				exit(0);
+			}
+			close(fdin[1]);
+		} else {
+			cmd->in = fdin[1];
+			cmd->close_in = 1;
+		}
 	}
 
 	need_out = !cmd->no_stdout
diff --git a/run-command.h b/run-command.h
index 3680ef9..7632843 100644
--- a/run-command.h
+++ b/run-command.h
@@ -13,6 +13,8 @@ enum {
 
 struct child_process {
 	const char **argv;
+	const char *in_bufptr;
+	size_t in_buflen;
 	pid_t pid;
 	int in;
 	int out;
-- 
1.5.1.1.135.gf948


-- 
Shawn.