v5, 8 partsrefactor the filter process code into a reusable module

> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> Add packet_read_line_gently() to enable reading a line without dying on
> EOF.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> pkt-line.c | 12 ++++++++++++
> pkt-line.h | 10 ++++++++++
> 2 files changed, 22 insertions(+)
> 
> diff --git a/pkt-line.c b/pkt-line.c
> index d4b6bfe076..58842544b4 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -323,6 +323,18 @@ char *packet_read_line(int fd, int *len_p)
> 	return packet_read_line_generic(fd, NULL, NULL, len_p);
> }
> 
> +int packet_read_line_gently(int fd, int *dst_len, char** dst_line)
> +{
> +	int len = packet_read(fd, NULL, NULL,
> +		packet_buffer, sizeof(packet_buffer),
> +		PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);

> +	return len;
> +}
> +
> char *packet_read_line_buf(char **src, size_t *src_len, int *dst_len)
> {
> 	return packet_read_line_generic(-1, src, src_len, dst_len);
> diff --git a/pkt-line.h b/pkt-line.h
> index 18eac64830..12b18991f6 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -74,6 +74,16 @@ int packet_read(int fd, char **src_buffer, size_t *src_len, char
> char *packet_read_line(int fd, int *size);
> 
> /*
> + * Convenience wrapper for packet_read that sets the PACKET_READ_GENTLE_ON_EOF
> + * and CHOMP_NEWLINE options. The return value specifies the number of bytes
> + * read into the buffer or -1 on truncated input. if the *dst_line parameter

> + * a static buffer (that may be overwritten by subsequent calls). If the size
> + * parameter is not NULL, the length of the packet is written to it.
> + */
> +int packet_read_line_gently(int fd, int *size, char** dst_line);
> +
> +/*
>  * Same as packet_read_line, but read from a buf rather than a descriptor;
>  * see packet_read for details on how src_* is used.
>  */

> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> Add packet_writel() which writes multiple lines in a single call and
> then calls packet_flush_gently(). Update convert.c to use the new
> packet_writel() function from pkt-line.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> convert.c  | 23 ++---------------------
> pkt-line.c | 19 +++++++++++++++++++
> pkt-line.h |  1 +
> 3 files changed, 22 insertions(+), 21 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index 8d652bf27c..793c29ebfd 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -521,25 +521,6 @@ static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap,
> 	return hashmap_get(hashmap, &key, NULL);
> }
> 
> -static int packet_write_list(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 void read_multi_file_filter_status(int fd, struct strbuf *status)
> {
> 	struct strbuf **pair;
> @@ -616,7 +597,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
> 
> 	sigchain_push(SIGPIPE, SIG_IGN);
> 
> -	err = packet_write_list(process->in, "git-filter-client", "version=2", NULL);
> +	err = packet_writel(process->in, "git-filter-client", "version=2", NULL);
> 	if (err)
> 		goto done;
> 
> @@ -632,7 +613,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
> 	if (err)
> 		goto done;
> 
> -	err = packet_write_list(process->in, "capability=clean", "capability=smudge", NULL);
> +	err = packet_writel(process->in, "capability=clean", "capability=smudge", NULL);
> 
> 	for (;;) {
> 		cap_buf = packet_read_line(process->out, NULL);
> diff --git a/pkt-line.c b/pkt-line.c
> index 58842544b4..2788aa1af6 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,6 +171,25 @@ 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];
> diff --git a/pkt-line.h b/pkt-line.h
> index 12b18991f6..cb3eda9695 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,6 +25,7 @@ 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)));
> +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);
> 
> -- 
> 2.12.0.windows.1.31.g1548525701.dirty

> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> To enable future reuse of the filter.<driver>.process infrastructure,
> split start_multi_file_filter into two separate parts.
> 
> start_multi_file_filter will now only contain the generic logic to
> manage the creation and tracking of the child process in a hashmap.
> 
> start_multi_file_filter_fn is a protocol specific initialization
> function that will negotiate the multi-file-filter interface version
> and capabilities.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> convert.c | 63 ++++++++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 38 insertions(+), 25 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index 793c29ebfd..404757eac9 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -565,35 +565,14 @@ static void stop_multi_file_filter(struct child_process *process)
> 	finish_command(process);
> }
> 
> -static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)
> +static int start_multi_file_filter_fn(struct cmd2process *entry)
> {
> 	int err;
> -	struct cmd2process *entry;
> -	struct child_process *process;
> -	const char *argv[] = { cmd, NULL };
> 	struct string_list cap_list = STRING_LIST_INIT_NODUP;
> 	char *cap_buf;
> 	const char *cap_name;
> -
> -	entry = xmalloc(sizeof(*entry));
> -	entry->cmd = cmd;
> -	entry->supported_capabilities = 0;
> -	process = &entry->process;
> -
> -	child_process_init(process);
> -	process->argv = argv;
> -	process->use_shell = 1;
> -	process->in = -1;
> -	process->out = -1;
> -	process->clean_on_exit = 1;
> -	process->clean_on_exit_handler = stop_multi_file_filter;
> -
> -	if (start_command(process)) {
> -		error("cannot fork to run external filter '%s'", cmd);
> -		return NULL;
> -	}
> -
> -	hashmap_entry_init(entry, strhash(cmd));
> +	struct child_process *process = &entry->process;
> +	const char *cmd = entry->cmd;
> 
> 	sigchain_push(SIGPIPE, SIG_IGN);
> 
> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
> done:
> 	sigchain_pop(SIGPIPE);
> 
> -	if (err || errno == EPIPE) {
> +	if (err || errno == EPIPE)
> +		err = err ? err : errno;

> +
> +	return err;
> +}
> +
> +static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)
> +{
> +	int err;
> +	struct cmd2process *entry;
> +	struct child_process *process;
> +	const char *argv[] = { cmd, NULL };
> +
> +	entry = xmalloc(sizeof(*entry));
> +	entry->cmd = cmd;
> +	entry->supported_capabilities = 0;
> +	process = &entry->process;
> +
> +	child_process_init(process);
> +	process->argv = argv;
> +	process->use_shell = 1;
> +	process->in = -1;
> +	process->out = -1;
> +	process->clean_on_exit = 1;
> +	process->clean_on_exit_handler = stop_multi_file_filter;
> +
> +	if (start_command(process)) {
> +		error("cannot fork to run external filter '%s'", cmd);
> +		return NULL;
> +	}
> +
> +	hashmap_entry_init(entry, strhash(cmd));
> +
> +	err = start_multi_file_filter_fn(entry);
> +	if (err) {
> 		error("initialization for external filter '%s' failed", cmd);
> 		kill_multi_file_filter(hashmap, entry);
> 		return NULL;
> -- 
> 2.12.0.windows.1.31.g1548525701.dirty

> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
>  done:
>  	sigchain_pop(SIGPIPE);
>  
> -	if (err || errno == EPIPE) {
> +	if (err || errno == EPIPE)
> +		err = err ? err : errno;
> +
> +	return err;
> +}

> On 11 Apr 2017, at 18:16, Jeff King <peff@peff.net> wrote:
> 
> On Fri, Apr 07, 2017 at 08:03:49AM -0400, Ben Peart wrote:
> 
>> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
>> done:
>> 	sigchain_pop(SIGPIPE);
>> 
>> -	if (err || errno == EPIPE) {
>> +	if (err || errno == EPIPE)
>> +		err = err ? err : errno;
>> +
>> +	return err;
>> +}
> 
> This isn't a new problem introduced by your patch, but this use of errno
> seems funny to me. Specifically:

> >  1. Do we need to save errno before calling sigchain_pop()? It's making
> >     syscalls (though admittedly they are unlikely to fail).
> 
> What if we add the following right before sigchain_pop() ?
> 
> 	if (errno == EPIPE)
> 		err = -1;

> >  2. If err is 0, then nothing failed. Who would have set errno? Aren't
> >     we reading whatever cruft happened to be in errno before the
> >     function started?
> 
> Yeah, looks like you're right:
> https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=6619179
> 
> According to this article we shouldn't even check *only* for errno. 
> At least we should add
> 	errno = 0;
> at the beginning of the function, no?

> This means we have many areas in Git where we don't handle errno
> correctly. E.g. right in convert.c where I stole code from:
> https://github.com/git/git/commit/0c4dd67a048b39470b9b95912e4912fecc405a85#diff-7949b716ab0a83e8c422a0d6336f19d6R361
> 
> Should that be addressed?

> On 11 Apr 2017, at 21:37, Jeff King <peff@peff.net> wrote:
> 
> On Tue, Apr 11, 2017 at 09:29:36PM +0200, Lars Schneider wrote:
> 
>>> 1. Do we need to save errno before calling sigchain_pop()? It's making
>>>    syscalls (though admittedly they are unlikely to fail).
>> 
>> What if we add the following right before sigchain_pop() ?
>> 
>> 	if (errno == EPIPE)
>> 		err = -1;
> 
> Yes, that would be fine (though again, this runs against point 2 below).
> 
>>> 2. If err is 0, then nothing failed. Who would have set errno? Aren't
>>>    we reading whatever cruft happened to be in errno before the
>>>    function started?
>> 
>> Yeah, looks like you're right:
>> https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=6619179
>> 
>> According to this article we shouldn't even check *only* for errno. 
>> At least we should add
>> 	errno = 0;
>> at the beginning of the function, no?
> 
> If you initialize errno to 0 right before a syscall, then yes, you can
> trust it without checking the return value of the syscall. I wouldn't
> trust it before calling more complicated functions, though. Not even
> xwrite(), which may see EINTR and keep going (which is OK for checking
> for EPIPE, but not checking generally for errno values).

> > If you initialize errno to 0 right before a syscall, then yes, you can
> > trust it without checking the return value of the syscall. I wouldn't
> > trust it before calling more complicated functions, though. Not even
> > xwrite(), which may see EINTR and keep going (which is OK for checking
> > for EPIPE, but not checking generally for errno values).
> 
> Should we remove all the errno checks here as we don't have any direct 
> "write" etc syscalls anyways then?

> On Tue, Apr 11, 2017 at 10:01:02PM +0200, Lars Schneider wrote:
>
>>> If you initialize errno to 0 right before a syscall, then yes, you can
>>> trust it without checking the return value of the syscall. I wouldn't
>>> trust it before calling more complicated functions, though. Not even
>>> xwrite(), which may see EINTR and keep going (which is OK for checking
>>> for EPIPE, but not checking generally for errno values).
>> Should we remove all the errno checks here as we don't have any direct
>> "write" etc syscalls anyways then?
> Yeah, you should be trusting the return value from the various
> sub-functions. Usually you'd check "errno == EPIPE" only when you saw an
> error return but you want to _ignore_ EPIPE. This is what
> filter_buffer_or_fd() is doing.
>
> But the code here is the opposite case. It definitely wants to treat
> EPIPE as an error. But that should be happening already because any
> EPIPE we get would come with an error-return from one of the
> packet_write() functions.
>
> So I would say that "err || errno == EPIPE" here can just become "err",
> and ditto in apply_multi_file_filter().

> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> Do a mechanical rename of the functions that will become the reusable
> sub-process module.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> convert.c | 53 +++++++++++++++++++++++++++--------------------------
> 1 file changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index 747c0c363b..235a6a5279 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -507,8 +507,8 @@ struct cmd2process {
> 	unsigned int supported_capabilities;
> };
> 

> 
> -static void kill_multi_file_filter(struct subprocess_entry *entry)
> +static void subprocess_stop(struct subprocess_entry *entry)
> {
> 	if (!entry)
> 		return;
> @@ -561,14 +561,14 @@ static void kill_multi_file_filter(struct subprocess_entry *entry)
> 	kill(entry->process.pid, SIGTERM);
> 	finish_command(&entry->process);
> 
> -	hashmap_remove(&cmd_process_map, entry, NULL);
> +	hashmap_remove(&subprocess_map, entry, NULL);
> 	free(entry);
> }

> @@ -777,7 +777,8 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
> 			 * Force shutdown and restart if another blob requires filtering.
> 			 */
> 			error("external filter '%s' failed", cmd);
> -			kill_multi_file_filter((struct subprocess_entry *)entry);
> +			subprocess_stop((struct subprocess_entry *)entry);
> +			free(entry);

> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> Move the sub-proces functions into sub-process.h/c.  Add documentation
> for the new module in Documentation/technical/api-sub-process.txt
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> Documentation/technical/api-sub-process.txt |  54 +++++++++++++
> Makefile                                    |   1 +
> convert.c                                   | 119 +---------------------------
> sub-process.c                               | 116 +++++++++++++++++++++++++++
> sub-process.h                               |  46 +++++++++++
> 5 files changed, 218 insertions(+), 118 deletions(-)
> create mode 100644 Documentation/technical/api-sub-process.txt
> create mode 100644 sub-process.c
> create mode 100644 sub-process.h
> 
> diff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt
> new file mode 100644
> index 0000000000..eb5005aa72
> --- /dev/null
> +++ b/Documentation/technical/api-sub-process.txt
> @@ -0,0 +1,54 @@
> +sub-process API
> +===============
> +
> +The sub-process API makes it possible to run background sub-processes
> +that should run until the git command exits and communicate with it
> +through stdin and stdout.  This reduces the overhead of having to fork
> +a new process each time it needs to be communicated with.

> +
> +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 negoiate the interface version and capabilities.

> +
> +
> +Functions
> +---------
> +
> +`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.
> diff --git a/Makefile b/Makefile
> index 9f8b35ad41..add945b560 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -838,6 +838,7 @@ LIB_OBJS += streaming.o
> LIB_OBJS += string-list.o
> LIB_OBJS += submodule.o
> LIB_OBJS += submodule-config.o
> +LIB_OBJS += sub-process.o
> LIB_OBJS += symlinks.o
> LIB_OBJS += tag.o
> LIB_OBJS += tempfile.o
> diff --git a/convert.c b/convert.c
> index 235a6a5279..baa41da760 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -4,6 +4,7 @@
> #include "quote.h"
> #include "sigchain.h"
> #include "pkt-line.h"
> +#include "sub-process.h"
> 
> /*
>  * convert.c - convert a file when checking it out and checking it in.
> @@ -496,86 +497,11 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le
> #define CAP_CLEAN    (1u<<0)
> #define CAP_SMUDGE   (1u<<1)
> 
> -struct subprocess_entry {
> -	struct hashmap_entry ent; /* must be the first member! */
> -	const char *cmd;
> -	struct child_process process;
> -};
> -
> struct cmd2process {
> 	struct subprocess_entry subprocess; /* must be the first member! */
> 	unsigned int supported_capabilities;
> };
> 
> -static int subprocess_map_initialized;
> -static struct hashmap subprocess_map;
> -
> -static int cmd2process_cmp(const struct subprocess_entry *e1,
> -			   const struct subprocess_entry *e2,
> -			   const void *unused)
> -{
> -	return strcmp(e1->cmd, e2->cmd);
> -}
> -
> -static struct subprocess_entry *subprocess_find_entry(const char *cmd)
> -{
> -	struct subprocess_entry key;
> -
> -	if (!subprocess_map_initialized) {
> -		subprocess_map_initialized = 1;
> -		hashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);
> -		return NULL;
> -	}
> -
> -	hashmap_entry_init(&key, strhash(cmd));
> -	key.cmd = cmd;
> -	return hashmap_get(&subprocess_map, &key, NULL);
> -}
> -
> -static void subprocess_read_status(int fd, struct strbuf *status)
> -{
> -	struct strbuf **pair;
> -	char *line;
> -	for (;;) {
> -		line = packet_read_line(fd, NULL);
> -		if (!line)
> -			break;
> -		pair = strbuf_split_str(line, '=', 2);
> -		if (pair[0] && pair[0]->len && pair[1]) {
> -			/* the last "status=<foo>" line wins */
> -			if (!strcmp(pair[0]->buf, "status=")) {
> -				strbuf_reset(status);
> -				strbuf_addbuf(status, pair[1]);
> -			}
> -		}
> -		strbuf_list_free(pair);
> -	}
> -}
> -
> -static void subprocess_stop(struct subprocess_entry *entry)
> -{
> -	if (!entry)
> -		return;
> -
> -	entry->process.clean_on_exit = 0;
> -	kill(entry->process.pid, SIGTERM);
> -	finish_command(&entry->process);
> -
> -	hashmap_remove(&subprocess_map, entry, NULL);
> -	free(entry);
> -}
> -
> -static void subprocess_exit_handler(struct child_process *process)
> -{
> -	sigchain_push(SIGPIPE, SIG_IGN);
> -	/* Closing the pipe signals the subprocess to initiate a shutdown. */
> -	close(process->in);
> -	close(process->out);
> -	sigchain_pop(SIGPIPE);
> -	/* Finish command will wait until the shutdown is complete. */
> -	finish_command(process);
> -}
> -
> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
> {
> 	int err;
> @@ -639,49 +565,6 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
> 	return err;
> }
> 
> -typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
> -int subprocess_start(struct subprocess_entry *entry, const char *cmd,
> -	subprocess_start_fn startfn)
> -{
> -	int err;
> -	struct child_process *process;
> -	const char *argv[] = { cmd, NULL };
> -
> -	if (!subprocess_map_initialized) {
> -		subprocess_map_initialized = 1;
> -		hashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);
> -	}
> -
> -	entry->cmd = cmd;
> -	process = &entry->process;
> -
> -	child_process_init(process);
> -	process->argv = argv;
> -	process->use_shell = 1;
> -	process->in = -1;
> -	process->out = -1;
> -	process->clean_on_exit = 1;
> -	process->clean_on_exit_handler = subprocess_exit_handler;
> -
> -	err = start_command(process);
> -	if (err) {
> -		error("cannot fork to run subprocess '%s'", cmd);
> -		return err;
> -	}
> -
> -	hashmap_entry_init(entry, strhash(cmd));
> -
> -	err = startfn(entry);
> -	if (err) {
> -		error("initialization for subprocess '%s' failed", cmd);
> -		subprocess_stop(entry);
> -		return err;
> -	}
> -
> -	hashmap_add(&subprocess_map, entry);
> -	return 0;
> -}
> -
> static int apply_multi_file_filter(const char *path, const char *src, size_t len,
> 				   int fd, struct strbuf *dst, const char *cmd,
> 				   const unsigned int wanted_capability)
> diff --git a/sub-process.c b/sub-process.c
> new file mode 100644
> index 0000000000..60bb650012
> --- /dev/null
> +++ b/sub-process.c
> @@ -0,0 +1,116 @@
> +/*
> + * Generic implementation of background process infrastructure.
> + */
> +#include "sub-process.h"
> +#include "sigchain.h"
> +#include "pkt-line.h"
> +
> +static int subprocess_map_initialized;
> +static struct hashmap subprocess_map;
> +
> +static int cmd2process_cmp(const struct subprocess_entry *e1,
> +			   const struct subprocess_entry *e2,
> +			   const void *unused)
> +{
> +	return strcmp(e1->cmd, e2->cmd);
> +}
> +
> +struct subprocess_entry *subprocess_find_entry(const char *cmd)
> +{
> +	struct subprocess_entry key;
> +
> +	if (!subprocess_map_initialized) {
> +		subprocess_map_initialized = 1;
> +		hashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);
> +		return NULL;
> +	}
> +
> +	hashmap_entry_init(&key, strhash(cmd));
> +	key.cmd = cmd;
> +	return hashmap_get(&subprocess_map, &key, NULL);
> +}
> +
> +void subprocess_read_status(int fd, struct strbuf *status)
> +{
> +	struct strbuf **pair;
> +	char *line;
> +	for (;;) {
> +		line = packet_read_line(fd, NULL);
> +		if (!line)
> +			break;
> +		pair = strbuf_split_str(line, '=', 2);
> +		if (pair[0] && pair[0]->len && pair[1]) {
> +			/* the last "status=<foo>" line wins */
> +			if (!strcmp(pair[0]->buf, "status=")) {
> +				strbuf_reset(status);
> +				strbuf_addbuf(status, pair[1]);
> +			}
> +		}
> +		strbuf_list_free(pair);
> +	}
> +}
> +
> +void subprocess_stop(struct subprocess_entry *entry)
> +{
> +	if (!entry)
> +		return;
> +
> +	entry->process.clean_on_exit = 0;
> +	kill(entry->process.pid, SIGTERM);
> +	finish_command(&entry->process);
> +
> +	hashmap_remove(&subprocess_map, entry, NULL);
> +}
> +
> +static void subprocess_exit_handler(struct child_process *process)
> +{
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +	/* Closing the pipe signals the subprocess to initiate a shutdown. */
> +	close(process->in);
> +	close(process->out);
> +	sigchain_pop(SIGPIPE);
> +	/* Finish command will wait until the shutdown is complete. */
> +	finish_command(process);
> +}
> +
> +int subprocess_start(struct subprocess_entry *entry, const char *cmd,
> +	subprocess_start_fn startfn)
> +{
> +	int err;
> +	struct child_process *process;
> +	const char *argv[] = { cmd, NULL };
> +
> +	if (!subprocess_map_initialized) {
> +		subprocess_map_initialized = 1;
> +		hashmap_init(&subprocess_map, (hashmap_cmp_fn)cmd2process_cmp, 0);
> +	}
> +
> +	entry->cmd = cmd;
> +	process = &entry->process;
> +
> +	child_process_init(process);
> +	process->argv = argv;
> +	process->use_shell = 1;
> +	process->in = -1;
> +	process->out = -1;
> +	process->clean_on_exit = 1;
> +	process->clean_on_exit_handler = subprocess_exit_handler;
> +
> +	err = start_command(process);
> +	if (err) {
> +		error("cannot fork to run subprocess '%s'", cmd);
> +		return err;
> +	}
> +
> +	hashmap_entry_init(entry, strhash(cmd));
> +
> +	err = startfn(entry);
> +	if (err) {
> +		error("initialization for subprocess '%s' failed", cmd);
> +		subprocess_stop(entry);
> +		return err;
> +	}
> +
> +	hashmap_add(&subprocess_map, entry);
> +	return 0;
> +}
> diff --git a/sub-process.h b/sub-process.h
> new file mode 100644
> index 0000000000..0cf1760a0a
> --- /dev/null
> +++ b/sub-process.h
> @@ -0,0 +1,46 @@
> +#ifndef SUBPROCESS_H
> +#define SUBPROCESS_H
> +
> +#include "git-compat-util.h"
> +#include "hashmap.h"
> +#include "run-command.h"
> +
> +/*
> + * Generic implementation of background process infrastructure.
> + * See Documentation/technical/api-background-process.txt.
> + */
> +
> + /* data structures */
> +
> +struct subprocess_entry {
> +	struct hashmap_entry ent; /* must be the first member! */
> +	const char *cmd;
> +	struct child_process process;
> +};
> +
> +/* subprocess functions */
> +
> +typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
> +int subprocess_start(struct subprocess_entry *entry, const char *cmd,
> +		subprocess_start_fn startfn);
> +
> +void subprocess_stop(struct subprocess_entry *entry);
> +
> +struct subprocess_entry *subprocess_find_entry(const char *cmd);
> +
> +/* subprocess helper functions */
> +
> +static inline struct child_process *subprocess_get_child_process(
> +		struct subprocess_entry *entry)
> +{
> +	return &entry->process;
> +}
> +
> +/*
> + * Helper function that will read packets looking for "status=<foo>"
> + * key/value pairs and return the value from the last "status" packet
> + */
> +
> +void subprocess_read_status(int fd, struct strbuf *status);
> +
> +#endif
> -- 
> 2.12.0.windows.1.31.g1548525701.dirty

> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> Enable sub-processes to gracefully handle when the process dies by
> updating subprocess_read_status to return an error on EOF instead of
> dying.
> 
> Update apply_multi_file_filter to take advantage of the revised
> subprocess_read_status.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> convert.c     | 10 ++++++++--
> sub-process.c | 10 +++++++---
> sub-process.h |  2 +-
> 3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index baa41da760..9e181e27ad 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -629,7 +629,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
> 	if (err)
> 		goto done;
> 
> -	subprocess_read_status(process->out, &filter_status);
> +	err = subprocess_read_status(process->out, &filter_status);
> +	if (err)
> +		goto done;
> +
> 	err = strcmp(filter_status.buf, "success");
> 	if (err)
> 		goto done;
> @@ -638,7 +641,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
> 	if (err)
> 		goto done;
> 
> -	subprocess_read_status(process->out, &filter_status);
> +	err = subprocess_read_status(process->out, &filter_status);
> +	if (err)
> +		goto done;
> +
> 	err = strcmp(filter_status.buf, "success");
> 
> done:
> diff --git a/sub-process.c b/sub-process.c
> index 60bb650012..c5057cafcd 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -30,13 +30,15 @@ struct subprocess_entry *subprocess_find_entry(const char *cmd)
> 	return hashmap_get(&subprocess_map, &key, NULL);
> }
> 
> -void subprocess_read_status(int fd, struct strbuf *status)
> +int subprocess_read_status(int fd, struct strbuf *status)
> {
> 	struct strbuf **pair;
> 	char *line;
> +	int len;
> +
> 	for (;;) {
> -		line = packet_read_line(fd, NULL);
> -		if (!line)
> +		len = packet_read_line_gently(fd, NULL, &line);
> +		if ((len == -1) || !line)

> 			break;
> 		pair = strbuf_split_str(line, '=', 2);
> 		if (pair[0] && pair[0]->len && pair[1]) {
> @@ -48,6 +50,8 @@ void subprocess_read_status(int fd, struct strbuf *status)
> 		}
> 		strbuf_list_free(pair);
> 	}
> +
> +	return len == -1 ? len : 0;

> }
> 
> void subprocess_stop(struct subprocess_entry *entry)
> diff --git a/sub-process.h b/sub-process.h
> index 0cf1760a0a..5a1eeeece0 100644
> --- a/sub-process.h
> +++ b/sub-process.h
> @@ -41,6 +41,6 @@ static inline struct child_process *subprocess_get_child_process(
>  * key/value pairs and return the value from the last "status" packet
>  */
> 
> -void subprocess_read_status(int fd, struct strbuf *status);
> +int subprocess_read_status(int fd, struct strbuf *status);
> 
> #endif
> -- 
> 2.12.0.windows.1.31.g1548525701.dirty
> 

> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> To enable future reuse of the filter.<driver>.process infrastructure,
> split the cmd2process structure into two separate parts.
> 
> subprocess_entry will now contain the generic data required to manage
> the creation and tracking of the child process in a hashmap. Also move
> all knowledge of the hashmap into the generic functions.
> 
> cmd2process is a filter protocol specific structure that is used to
> track the negotiated capabilities of the filter.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> convert.c | 57 +++++++++++++++++++++++++++++++--------------------------
> 1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index 404757eac9..f569026511 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -496,29 +496,40 @@ static int apply_single_file_filter(const char *path, const char *src, size_t le
> #define CAP_CLEAN    (1u<<0)
> #define CAP_SMUDGE   (1u<<1)
> 
> -struct cmd2process {
> +struct subprocess_entry {
> 	struct hashmap_entry ent; /* must be the first member! */
> -	unsigned int supported_capabilities;
> 	const char *cmd;
> 	struct child_process process;
> };
> 
> +struct cmd2process {
> +	struct subprocess_entry subprocess; /* must be the first member! */
> +	unsigned int supported_capabilities;
> +};
> +
> static int cmd_process_map_initialized;
> static struct hashmap cmd_process_map;
> 
> -static int cmd2process_cmp(const struct cmd2process *e1,
> -			   const struct cmd2process *e2,
> +static int cmd2process_cmp(const struct subprocess_entry *e1,
> +			   const struct subprocess_entry *e2,
> 			   const void *unused)
> {
> 	return strcmp(e1->cmd, e2->cmd);
> }
> 
> -static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
> +static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)

> {
> -	struct cmd2process key;
> +	struct subprocess_entry key;
> +
> +	if (!cmd_process_map_initialized) {
> +		cmd_process_map_initialized = 1;
> +		hashmap_init(&cmd_process_map, (hashmap_cmp_fn)cmd2process_cmp, 0);
> +		return NULL;
> +	}
> +

>> -static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
>> +static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)
>
> I am curious why you removed the hashmap parameter (here and in other pars of this patch). 
> I know the parameter is not strictly necessary as the hashmap is a global variable anyways. 
> However, I think it eases code maintainability in the long run if a function is "as pure as 
> possible" (IOW does rely on global state as less as possible).

> Lars Schneider <larsxschneider@gmail.com> writes:
>
>>> -static struct cmd2process *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
>>> +static struct subprocess_entry *find_multi_file_filter_entry(const char *cmd)
>> I am curious why you removed the hashmap parameter (here and in other pars of this patch).
>> I know the parameter is not strictly necessary as the hashmap is a global variable anyways.
>> However, I think it eases code maintainability in the long run if a function is "as pure as
>> possible" (IOW does rely on global state as less as possible).
> If the original relied on a global hashmap and this update kept the
> code like so, I wouldn't mind the end result of this series
> (i.e. rely on it being global).  But that is not the case.  It is
> making the code worse by stopping passing the hashmap through the
> callchain.

> On 4/16/2017 11:31 PM, Junio C Hamano wrote:
>> Lars Schneider <larsxschneider@gmail.com> writes:
>>
>>> However, I think it eases code maintainability in the long run if a function is "as pure as
>>> possible" (IOW does rely on global state as less as possible).
>>
>> If the original relied on a global hashmap and this update kept the
>> code like so, I wouldn't mind the end result of this series
>> (i.e. rely on it being global).  But that is not the case.  It is
>> making the code worse by stopping passing the hashmap through the
>> callchain.
>
> ...  Since it was already a global, I didn't feel
> like this made it any worse.

> Ben Peart <peartben@gmail.com> writes:
>
>> On 4/16/2017 11:31 PM, Junio C Hamano wrote:
>>> Lars Schneider <larsxschneider@gmail.com> writes:
>>>
>>>> However, I think it eases code maintainability in the long run if a function is "as pure as
>>>> possible" (IOW does rely on global state as less as possible).
>>> If the original relied on a global hashmap and this update kept the
>>> code like so, I wouldn't mind the end result of this series
>>> (i.e. rely on it being global).  But that is not the case.  It is
>>> making the code worse by stopping passing the hashmap through the
>>> callchain.
>> ...  Since it was already a global, I didn't feel
>> like this made it any worse.
> The code before your series can easily lose the globals with the
> attached patch, _exactly_ because it is prepared to be reusable by a
> new caller that supplies its own hashmap by passing it through the
> callchain.  The child-process machinery Lars made for his conversion
> thing, which you are trying to split out to make it reusable, can be
> used by somebody other than apply_multi_file_filter() if you do not
> lose the hashmap; what the new caller needs to do is to supply its
> own hashmap so that they do not interact with the set of processes
> used by Lars's conversion machinery.
>
> If we want to lose the global _after_ applying this patch 4/8, don't
> we have to essentially _undo_ 4/8?  How can it not be seen as making
> it worse?

>
>   convert.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index 8d652bf27c..ff831e85b8 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -503,9 +503,6 @@ struct cmd2process {
>   	struct child_process process;
>   };
>   
> -static int cmd_process_map_initialized;
> -static struct hashmap cmd_process_map;
> -
>   static int cmd2process_cmp(const struct cmd2process *e1,
>   			   const struct cmd2process *e2,
>   			   const void *unused)
> @@ -682,6 +679,9 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>   	struct strbuf filter_status = STRBUF_INIT;
>   	const char *filter_type;
>   
> +static int cmd_process_map_initialized;
> +static struct hashmap cmd_process_map;
> +
>   	if (!cmd_process_map_initialized) {
>   		cmd_process_map_initialized = 1;
>   		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
>

> On 07 Apr 2017, at 14:03, Ben Peart <peartben@gmail.com> wrote:
> 
> Update all functions that are going to be moved into a reusable module
> so that they only work with the reusable data structures.  Move code
> that is specific to the filter out into the filter specific functions.
> 
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
> convert.c | 46 ++++++++++++++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index f569026511..747c0c363b 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -576,14 +576,15 @@ static void stop_multi_file_filter(struct child_process *process)
> 	finish_command(process);
> }
> 
> -static int start_multi_file_filter_fn(struct cmd2process *entry)
> +static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
> {
> 	int err;
> +	struct cmd2process *entry = (struct cmd2process *)subprocess;

> 	struct string_list cap_list = STRING_LIST_INIT_NODUP;
> 	char *cap_buf;
> 	const char *cap_name;
> -	struct child_process *process = &entry->subprocess.process;
> -	const char *cmd = entry->subprocess.cmd;
> +	struct child_process *process = &subprocess->process;
> +	const char *cmd = subprocess->cmd;
> 
> 	sigchain_push(SIGPIPE, SIG_IGN);
> 
> @@ -638,17 +639,21 @@ static int start_multi_file_filter_fn(struct cmd2process *entry)
> 	return err;
> }

> static int apply_multi_file_filter(const char *path, const char *src, size_t len,
> @@ -692,9 +698,13 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
> 	fflush(NULL);
> 
> 	if (!entry) {
> -		entry = start_multi_file_filter(cmd);
> -		if (!entry)
> +		entry = xmalloc(sizeof(*entry));
> +		entry->supported_capabilities = 0;