[PATCH v6 0/8] refactor the filter process code into a reusable module

[PATCH v6 0/8] refactor the filter process code into a reusable module

[Ben Peart]

Refactor the filter.<driver>.process code into a separate sub-process
module that can be used to reduce the cost of starting up a sub-process
for multiple commands.  It does this by keeping the external process
running and processing all commands by communicating over standard input
and standard output using the packet format (pkt-line) based protocol.
Full documentation is in Documentation/technical/api-sub-process.txt.

This code is refactored from:

	Commit edcc85814c ("convert: add filter.<driver>.process option", 2016-10-16)
	keeps the external process running and processes all commands

Ben Peart (8):
  pkt-line: add packet_read_line_gently()
  convert: move packet_write_list() into pkt-line as packet_writel()
  convert: Split start_multi_file_filter into two separate functions
  convert: Separate generic structures and variables from the filter
    specific ones
  convert: Update generic functions to only use generic data structures
  convert: rename reusable sub-process functions
  sub-process: move sub-process functions into separate files
  convert: Update subprocess_read_status to not die on EOF

 Documentation/technical/api-sub-process.txt |  59 ++++++++++
 Makefile                                    |   1 +
 convert.c                                   | 161 ++++++----------------------
 pkt-line.c                                  |  33 +++++-
 pkt-line.h                                  |  11 ++
 sub-process.c                               | 106 ++++++++++++++++++
 sub-process.h                               |  49 +++++++++
 7 files changed, 291 insertions(+), 129 deletions(-)
 create mode 100644 Documentation/technical/api-sub-process.txt
 create mode 100644 sub-process.c
 create mode 100644 sub-process.h

-- 
2.12.0.windows.1.33.g243d9b384c

[PATCH v6 1/8] pkt-line: add packet_read_line_gently()

[Ben Peart]

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 | 14 +++++++++++++-
 pkt-line.h | 10 ++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/pkt-line.c b/pkt-line.c
index d4b6bfe076..bfdb177b34 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd,
 			      PACKET_READ_CHOMP_NEWLINE);
 	if (dst_len)
 		*dst_len = len;
-	return len ? packet_buffer : NULL;
+	return (len > 0) ? packet_buffer : NULL;
 }
 
 char *packet_read_line(int fd, int *len_p)
@@ -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);
+	if (dst_len)
+		*dst_len = len;
+	if (dst_line)
+		*dst_line = (len > 0) ? packet_buffer : NULL;
+	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..ad30db101a 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
+ * is not NULL it will return NULL for a flush packet and otherwise points to
+ * 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.
  */
-- 
2.12.0.windows.1.33.g243d9b384c

Re: [PATCH v6 1/8] pkt-line: add packet_read_line_gently()

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> 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 | 14 +++++++++++++-
>  pkt-line.h | 10 ++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index d4b6bfe076..bfdb177b34 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd,
>  			      PACKET_READ_CHOMP_NEWLINE);
>  	if (dst_len)
>  		*dst_len = len;
> -	return len ? packet_buffer : NULL;
> +	return (len > 0) ? packet_buffer : NULL;
>  }

The log does not seem to explain what this change is about.  

Is this supposed to be a preliminary bugfix where this helper used
to return a non-NULL buffer when underlying packet_read() signaled
an error by returning a negative len?  If so, this should probably
be a separate patch early in the series.

Should len==0 be considered an error?  Especially given that
PACKET_READ_CHOMP_NEWLINE is in use, I would expect that len==0
should be treated similarly to positive length, i.e. the otherside
gave us an empty line.

>  char *packet_read_line(int fd, int *len_p)
> @@ -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)

ERROR: "foo** bar" should be "foo **bar"
#29: FILE: pkt-line.c:326:
+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);
> +	if (dst_len)
> +		*dst_len = len;
> +	if (dst_line)
> +		*dst_line = (len > 0) ? packet_buffer : NULL;

I have the same doubt as above for len == 0 case.

> +	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..ad30db101a 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
> + * is not NULL it will return NULL for a flush packet and otherwise points to
> + * 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.
>   */

Re: [PATCH v6 1/8] pkt-line: add packet_read_line_gently()

[Ben Peart]

On 4/24/2017 12:21 AM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
>
>> 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 | 14 +++++++++++++-
>>   pkt-line.h | 10 ++++++++++
>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/pkt-line.c b/pkt-line.c
>> index d4b6bfe076..bfdb177b34 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd,
>>   			      PACKET_READ_CHOMP_NEWLINE);
>>   	if (dst_len)
>>   		*dst_len = len;
>> -	return len ? packet_buffer : NULL;
>> +	return (len > 0) ? packet_buffer : NULL;
>>   }
> The log does not seem to explain what this change is about.

This change was made as the result of a request in feedback from the 
previous version of the patch series which pointed out that the call to 
packet_read can return -1 in some error cases and to keep the code more 
consistent with packet_read_line_gently below.

If len < 0 then the old code would have incorrectly returned a pointer 
to a buffer with garbage while the new code correctly returns NULL 
(fixes potential bug)
if len == 0 then the code will return NULL before and after this change 
(no change in behavior)
if len > 0 then the code will return packet_buffer before and after this 
change (no change in behavior)

>
> Is this supposed to be a preliminary bugfix where this helper used
> to return a non-NULL buffer when underlying packet_read() signaled
> an error by returning a negative len?  If so, this should probably
> be a separate patch early in the series.
>
> Should len==0 be considered an error?  Especially given that
> PACKET_READ_CHOMP_NEWLINE is in use, I would expect that len==0
> should be treated similarly to positive length, i.e. the otherside
> gave us an empty line.
>
>>   char *packet_read_line(int fd, int *len_p)
>> @@ -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)
> ERROR: "foo** bar" should be "foo **bar"
> #29: FILE: pkt-line.c:326:
> +int packet_read_line_gently(int fd, int *dst_len, char** dst_line)

Sorry, missed that somehow.  I'll move the space before the ** in the 
next version of the patch series.

>
>> +{
>> +	int len = packet_read(fd, NULL, NULL,
>> +			      packet_buffer, sizeof(packet_buffer),
>> +			      PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);
>> +	if (dst_len)
>> +		*dst_len = len;
>> +	if (dst_line)
>> +		*dst_line = (len > 0) ? packet_buffer : NULL;
> I have the same doubt as above for len == 0 case.

packet_read() returns -1 when PACKET_READ_GENTLE_ON_EOF is passed and it 
hits truncated output from the remote process.  This occurs when the 
remote process exits unexpectedly which is the exact case I was fixing 
with this new function.  This requires testing len for this error 
condition so that it can correctly handle this error case, otherwise it 
would incorrectly return an invalid buffer. Since this is a new 
function, there isn't any before/after behavior comparisons but it is 
consistent with the similar packet_read_line() function.

Re: [PATCH v6 1/8] pkt-line: add packet_read_line_gently()

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> On 4/24/2017 12:21 AM, Junio C Hamano wrote:
>> Ben Peart <peartben@gmail.com> writes:
>>
>>> 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 | 14 +++++++++++++-
>>>   pkt-line.h | 10 ++++++++++
>>>   2 files changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/pkt-line.c b/pkt-line.c
>>> index d4b6bfe076..bfdb177b34 100644
>>> --- a/pkt-line.c
>>> +++ b/pkt-line.c
>>> @@ -315,7 +315,7 @@ static char *packet_read_line_generic(int fd,
>>>   			      PACKET_READ_CHOMP_NEWLINE);
>>>   	if (dst_len)
>>>   		*dst_len = len;
>>> -	return len ? packet_buffer : NULL;
>>> +	return (len > 0) ? packet_buffer : NULL;
>>>   }
>> The log does not seem to explain what this change is about.
>
> This change was made as the result of a request in feedback from the
> previous version of the patch series which pointed out that the call
> to packet_read can return -1 in some error cases and to keep the code
> more consistent with packet_read_line_gently below.
>
> If len < 0 then the old code would have incorrectly returned a pointer
> to a buffer with garbage while the new code correctly returns NULL
> (fixes potential bug)
> if len == 0 then the code will return NULL before and after this
> change (no change in behavior)
> if len > 0 then the code will return packet_buffer before and after
> this change (no change in behavior)

TL;DR "Yes this is a preliminary fix and I'll split out into a
separate patch early in the series in the next reroll"?  Thanks.

>> Is this supposed to be a preliminary bugfix where this helper used
>> to return a non-NULL buffer when underlying packet_read() signaled
>> an error by returning a negative len?  If so, this should probably
>> be a separate patch early in the series.
>>
>> Should len==0 be considered an error?  Especially given that
>> PACKET_READ_CHOMP_NEWLINE is in use, I would expect that len==0
>> should be treated similarly to positive length, i.e. the otherside
>> gave us an empty line.
>>
>>> +{
>>> +	int len = packet_read(fd, NULL, NULL,
>>> +			      packet_buffer, sizeof(packet_buffer),
>>> +			      PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);
>>> +	if (dst_len)
>>> +		*dst_len = len;
>>> +	if (dst_line)
>>> +		*dst_line = (len > 0) ? packet_buffer : NULL;
>> I have the same doubt as above for len == 0 case.
>
> packet_read() returns -1 when PACKET_READ_GENTLE_ON_EOF is passed and
> it hits truncated output from the remote process. 

I know, but that is irrelevant to my question, which is about
CHOMP_NEWLINE.  I didn't even ask "why a negative len treated
specially?"  My question is about the case where len == 0.  Your
patch treats len==0 just like len==-1, i.e. an error, but I do not
know if that is correct, hence my question.  We both know len < 0
is an error and you do not need to waste time elaborating on it.

Re: [PATCH v6 1/8] pkt-line: add packet_read_line_gently()

[Ben Peart]

Sorry if you get this twice, somehow Thunderbird converted my response 
to HTML


On 4/24/2017 10:19 PM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
>
>> On 4/24/2017 12:21 AM, Junio C Hamano wrote:
>>> Ben Peart <peartben@gmail.com> writes:
>>>
>>> +{ 
>>>> +	int len = packet_read(fd, NULL, NULL,
>>>> +			      packet_buffer, sizeof(packet_buffer),
>>>> +			      PACKET_READ_CHOMP_NEWLINE|PACKET_READ_GENTLE_ON_EOF);
>>>> +	if (dst_len)
>>>> +		*dst_len = len;
>>>> +	if (dst_line)
>>>> +		*dst_line = (len > 0) ? packet_buffer : NULL;
>>> I have the same doubt as above for len == 0 case.
>> packet_read() returns -1 when PACKET_READ_GENTLE_ON_EOF is passed and
>> it hits truncated output from the remote process.
> I know, but that is irrelevant to my question, which is about
> CHOMP_NEWLINE.  I didn't even ask "why a negative len treated
> specially?"  My question is about the case where len == 0.  Your
> patch treats len==0 just like len==-1, i.e. an error, but I do not
> know if that is correct, hence my question.  We both know len < 0
> is an error and you do not need to waste time elaborating on it.
>
>

The packet_read_line() function returns NULL when len == 0 and 
PACKET_READ_CHOMP_NEWLINE is passed so I wrote the similar 
packet_read_line_gently() function to behave the same.

How about I update the comment in the function header to make this more 
clear:

diff --git a/pkt-line.h b/pkt-line.h
index 7c278a158b..b2965869ad 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -78,9 +78,10 @@ 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
- * is not NULL it will return NULL for a flush packet and otherwise 
points to
- * 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.
+ * is not NULL it will return NULL for a flush packet or when the number of
+ * bytes copied is zero and otherwise points to 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);

[PATCH v6 2/8] convert: move packet_write_list() into pkt-line as packet_writel()

[Ben Peart]

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 bfdb177b34..756410d2f6 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 ad30db101a..957b9a5fa3 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.33.g243d9b384c

[PATCH v6 3/8] convert: Split start_multi_file_filter into two separate functions

[Ben Peart]

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 | 62 ++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 26 deletions(-)

diff --git a/convert.c b/convert.c
index 793c29ebfd..36401fe087 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,38 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
 done:
 	sigchain_pop(SIGPIPE);
 
-	if (err || errno == EPIPE) {
+	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;
@@ -733,7 +743,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 done:
 	sigchain_pop(SIGPIPE);
 
-	if (err || errno == EPIPE) {
+	if (err) {
 		if (!strcmp(filter_status.buf, "error")) {
 			/* The filter signaled a problem with the file. */
 		} else if (!strcmp(filter_status.buf, "abort")) {
-- 
2.12.0.windows.1.33.g243d9b384c

Re: [PATCH v6 3/8] convert: Split start_multi_file_filter into two separate functions

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> Subject: [PATCH v6 3/8] convert: Split start_multi_file_filter into two separate functions

Two minor nits, because the capital after "<area>:" looks ugly in shortlog
output, and having () there makes it easier to notice that a
function name is being discussed.  I.e.

    convert: split start_multi_file_filter() into two separate functions

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

The above fails to describe a lot more important/significant change
this patch makes.  Two mistaken check "errno == EPIPE" have been
removed as a preliminary bugfix.  I think the bugfix deserves to be
split into a separate patch by itself and hoisted much earlier in
the series.  It alone is a very good change, with or without the
remainder of the changes in this patch.

Thanks.

> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> ---
>  convert.c | 62 ++++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 36 insertions(+), 26 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index 793c29ebfd..36401fe087 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,38 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
>  done:
>  	sigchain_pop(SIGPIPE);
>  
> -	if (err || errno == EPIPE) {
> +	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;
> @@ -733,7 +743,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>  done:
>  	sigchain_pop(SIGPIPE);
>  
> -	if (err || errno == EPIPE) {
> +	if (err) {
>  		if (!strcmp(filter_status.buf, "error")) {
>  			/* The filter signaled a problem with the file. */
>  		} else if (!strcmp(filter_status.buf, "abort")) {

Re: [PATCH v6 3/8] convert: Split start_multi_file_filter into two separate functions

[Ben Peart]

On 4/24/2017 12:31 AM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
>
>> Subject: [PATCH v6 3/8] convert: Split start_multi_file_filter into two separate functions
> Two minor nits, because the capital after "<area>:" looks ugly in shortlog
> output, and having () there makes it easier to notice that a
> function name is being discussed.  I.e.
>
>      convert: split start_multi_file_filter() into two separate functions

I'll make it so in the next version of the patch series.

>> 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.
> The above fails to describe a lot more important/significant change
> this patch makes.  Two mistaken check "errno == EPIPE" have been
> removed as a preliminary bugfix.  I think the bugfix deserves to be
> split into a separate patch by itself and hoisted much earlier in
> the series.  It alone is a very good change, with or without the
> remainder of the changes in this patch.

OK, I'll pull that bugfix out into a separate patch.

> Thanks.
>
>> Signed-off-by: Ben Peart <benpeart@microsoft.com>
>> ---
>>   convert.c | 62 ++++++++++++++++++++++++++++++++++++--------------------------
>>   1 file changed, 36 insertions(+), 26 deletions(-)
>>
>> diff --git a/convert.c b/convert.c
>> index 793c29ebfd..36401fe087 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,38 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
>>   done:
>>   	sigchain_pop(SIGPIPE);
>>   
>> -	if (err || errno == EPIPE) {
>> +	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;
>> @@ -733,7 +743,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>>   done:
>>   	sigchain_pop(SIGPIPE);
>>   
>> -	if (err || errno == EPIPE) {
>> +	if (err) {
>>   		if (!strcmp(filter_status.buf, "error")) {
>>   			/* The filter signaled a problem with the file. */
>>   		} else if (!strcmp(filter_status.buf, "abort")) {

[PATCH v6 4/8] convert: Separate generic structures and variables from the filter specific ones

[Ben Peart]

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 | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/convert.c b/convert.c
index 36401fe087..bfb19beed5 100644
--- a/convert.c
+++ b/convert.c
@@ -496,26 +496,31 @@ 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(struct hashmap *hashmap, const char *cmd)
 {
-	struct cmd2process key;
+	struct subprocess_entry key;
+
 	hashmap_entry_init(&key, strhash(cmd));
 	key.cmd = cmd;
 	return hashmap_get(hashmap, &key, NULL);
@@ -541,7 +546,7 @@ static void read_multi_file_filter_status(int fd, struct strbuf *status)
 	}
 }
 
-static void kill_multi_file_filter(struct hashmap *hashmap, struct cmd2process *entry)
+static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *entry)
 {
 	if (!entry)
 		return;
@@ -571,8 +576,8 @@ static int start_multi_file_filter_fn(struct cmd2process *entry)
 	struct string_list cap_list = STRING_LIST_INIT_NODUP;
 	char *cap_buf;
 	const char *cap_name;
-	struct child_process *process = &entry->process;
-	const char *cmd = entry->cmd;
+	struct child_process *process = &entry->subprocess.process;
+	const char *cmd = entry->subprocess.cmd;
 
 	sigchain_push(SIGPIPE, SIG_IGN);
 
@@ -632,9 +637,9 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
 	const char *argv[] = { cmd, NULL };
 
 	entry = xmalloc(sizeof(*entry));
-	entry->cmd = cmd;
+	entry->subprocess.cmd = cmd;
 	entry->supported_capabilities = 0;
-	process = &entry->process;
+	process = &entry->subprocess.process;
 
 	child_process_init(process);
 	process->argv = argv;
@@ -654,7 +659,7 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
 	err = start_multi_file_filter_fn(entry);
 	if (err) {
 		error("initialization for external filter '%s' failed", cmd);
-		kill_multi_file_filter(hashmap, entry);
+		kill_multi_file_filter(hashmap, &entry->subprocess);
 		return NULL;
 	}
 
@@ -678,7 +683,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
 		entry = NULL;
 	} else {
-		entry = find_multi_file_filter_entry(&cmd_process_map, cmd);
+		entry = (struct cmd2process *)find_multi_file_filter_entry(&cmd_process_map, cmd);
 	}
 
 	fflush(NULL);
@@ -688,7 +693,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 		if (!entry)
 			return 0;
 	}
-	process = &entry->process;
+	process = &entry->subprocess.process;
 
 	if (!(wanted_capability & entry->supported_capabilities))
 		return 0;
@@ -759,7 +764,7 @@ 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(&cmd_process_map, entry);
+			kill_multi_file_filter(&cmd_process_map, &entry->subprocess);
 		}
 	} else {
 		strbuf_swap(dst, &nbuf);
-- 
2.12.0.windows.1.33.g243d9b384c

[PATCH v6 5/8] convert: Update generic functions to only use generic data structures

[Ben Peart]

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 | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/convert.c b/convert.c
index bfb19beed5..d15b10a3c5 100644
--- a/convert.c
+++ b/convert.c
@@ -556,7 +556,6 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_en
 	finish_command(&entry->process);
 
 	hashmap_remove(hashmap, entry, NULL);
-	free(entry);
 }
 
 static void stop_multi_file_filter(struct child_process *process)
@@ -570,14 +569,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);
 
@@ -629,17 +629,16 @@ static int start_multi_file_filter_fn(struct cmd2process *entry)
 	return err;
 }
 
-static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, const char *cmd)
+typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
+int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,
+	subprocess_start_fn startfn)
 {
 	int err;
-	struct cmd2process *entry;
 	struct child_process *process;
 	const char *argv[] = { cmd, NULL };
 
-	entry = xmalloc(sizeof(*entry));
-	entry->subprocess.cmd = cmd;
-	entry->supported_capabilities = 0;
-	process = &entry->subprocess.process;
+	entry->cmd = cmd;
+	process = &entry->process;
 
 	child_process_init(process);
 	process->argv = argv;
@@ -649,22 +648,23 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
 	process->clean_on_exit = 1;
 	process->clean_on_exit_handler = stop_multi_file_filter;
 
-	if (start_command(process)) {
+	err = start_command(process);
+	if (err) {
 		error("cannot fork to run external filter '%s'", cmd);
-		return NULL;
+		return err;
 	}
 
 	hashmap_entry_init(entry, strhash(cmd));
 
-	err = start_multi_file_filter_fn(entry);
+	err = startfn(entry);
 	if (err) {
 		error("initialization for external filter '%s' failed", cmd);
-		kill_multi_file_filter(hashmap, &entry->subprocess);
-		return NULL;
+		kill_multi_file_filter(hashmap, entry);
+		return err;
 	}
 
 	hashmap_add(hashmap, entry);
-	return entry;
+	return 0;
 }
 
 static int apply_multi_file_filter(const char *path, const char *src, size_t len,
@@ -689,9 +689,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_process_map, cmd);
-		if (!entry)
+		entry = xmalloc(sizeof(*entry));
+		entry->supported_capabilities = 0;
+
+		if (start_multi_file_filter(&cmd_process_map, &entry->subprocess, cmd, start_multi_file_filter_fn)) {
+			free(entry);
 			return 0;
+		}
 	}
 	process = &entry->subprocess.process;
 
@@ -765,6 +769,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 			 */
 			error("external filter '%s' failed", cmd);
 			kill_multi_file_filter(&cmd_process_map, &entry->subprocess);
+			free(entry);
 		}
 	} else {
 		strbuf_swap(dst, &nbuf);
-- 
2.12.0.windows.1.33.g243d9b384c

[PATCH v6 6/8] convert: rename reusable sub-process functions

[Ben Peart]

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 | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/convert.c b/convert.c
index d15b10a3c5..cfae45aeee 100644
--- a/convert.c
+++ b/convert.c
@@ -507,8 +507,8 @@ struct cmd2process {
 	unsigned int supported_capabilities;
 };
 
-static int cmd_process_map_initialized;
-static struct hashmap cmd_process_map;
+static int subprocess_map_initialized;
+static struct hashmap subprocess_map;
 
 static int cmd2process_cmp(const struct subprocess_entry *e1,
 			   const struct subprocess_entry *e2,
@@ -517,7 +517,7 @@ static int cmd2process_cmp(const struct subprocess_entry *e1,
 	return strcmp(e1->cmd, e2->cmd);
 }
 
-static struct subprocess_entry *find_multi_file_filter_entry(struct hashmap *hashmap, const char *cmd)
+static struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const char *cmd)
 {
 	struct subprocess_entry key;
 
@@ -526,7 +526,7 @@ static struct subprocess_entry *find_multi_file_filter_entry(struct hashmap *has
 	return hashmap_get(hashmap, &key, NULL);
 }
 
-static void read_multi_file_filter_status(int fd, struct strbuf *status)
+static void subprocess_read_status(int fd, struct strbuf *status)
 {
 	struct strbuf **pair;
 	char *line;
@@ -546,7 +546,7 @@ static void read_multi_file_filter_status(int fd, struct strbuf *status)
 	}
 }
 
-static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *entry)
+static void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry)
 {
 	if (!entry)
 		return;
@@ -558,10 +558,10 @@ static void kill_multi_file_filter(struct hashmap *hashmap, struct subprocess_en
 	hashmap_remove(hashmap, entry, NULL);
 }
 
-static void stop_multi_file_filter(struct child_process *process)
+static void subprocess_exit_handler(struct child_process *process)
 {
 	sigchain_push(SIGPIPE, SIG_IGN);
-	/* Closing the pipe signals the filter to initiate a shutdown. */
+	/* Closing the pipe signals the subprocess to initiate a shutdown. */
 	close(process->in);
 	close(process->out);
 	sigchain_pop(SIGPIPE);
@@ -630,7 +630,7 @@ static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
 }
 
 typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
-int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,
+int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,
 	subprocess_start_fn startfn)
 {
 	int err;
@@ -646,11 +646,11 @@ int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *en
 	process->in = -1;
 	process->out = -1;
 	process->clean_on_exit = 1;
-	process->clean_on_exit_handler = stop_multi_file_filter;
+	process->clean_on_exit_handler = subprocess_exit_handler;
 
 	err = start_command(process);
 	if (err) {
-		error("cannot fork to run external filter '%s'", cmd);
+		error("cannot fork to run subprocess '%s'", cmd);
 		return err;
 	}
 
@@ -658,8 +658,8 @@ int start_multi_file_filter(struct hashmap *hashmap, struct subprocess_entry *en
 
 	err = startfn(entry);
 	if (err) {
-		error("initialization for external filter '%s' failed", cmd);
-		kill_multi_file_filter(hashmap, entry);
+		error("initialization for subprocess '%s' failed", cmd);
+		subprocess_stop(hashmap, entry);
 		return err;
 	}
 
@@ -678,12 +678,12 @@ 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;
 
-	if (!cmd_process_map_initialized) {
-		cmd_process_map_initialized = 1;
-		hashmap_init(&cmd_process_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
+	if (!subprocess_map_initialized) {
+		subprocess_map_initialized = 1;
+		hashmap_init(&subprocess_map, (hashmap_cmp_fn) cmd2process_cmp, 0);
 		entry = NULL;
 	} else {
-		entry = (struct cmd2process *)find_multi_file_filter_entry(&cmd_process_map, cmd);
+		entry = (struct cmd2process *)subprocess_find_entry(&subprocess_map, cmd);
 	}
 
 	fflush(NULL);
@@ -692,7 +692,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 		entry = xmalloc(sizeof(*entry));
 		entry->supported_capabilities = 0;
 
-		if (start_multi_file_filter(&cmd_process_map, &entry->subprocess, cmd, start_multi_file_filter_fn)) {
+		if (subprocess_start(&subprocess_map, &entry->subprocess, cmd, start_multi_file_filter_fn)) {
 			free(entry);
 			return 0;
 		}
@@ -737,7 +737,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
-	read_multi_file_filter_status(process->out, &filter_status);
+	subprocess_read_status(process->out, &filter_status);
 	err = strcmp(filter_status.buf, "success");
 	if (err)
 		goto done;
@@ -746,7 +746,7 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
-	read_multi_file_filter_status(process->out, &filter_status);
+	subprocess_read_status(process->out, &filter_status);
 	err = strcmp(filter_status.buf, "success");
 
 done:
@@ -768,7 +768,7 @@ 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(&cmd_process_map, &entry->subprocess);
+			subprocess_stop(&subprocess_map, &entry->subprocess);
 			free(entry);
 		}
 	} else {
-- 
2.12.0.windows.1.33.g243d9b384c

[PATCH v6 7/8] sub-process: move sub-process functions into separate files

[Ben Peart]

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 |  59 ++++++++++++++++
 Makefile                                    |   1 +
 convert.c                                   | 104 +---------------------------
 sub-process.c                               | 102 +++++++++++++++++++++++++++
 sub-process.h                               |  49 +++++++++++++
 5 files changed, 212 insertions(+), 103 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..793508cf3e
--- /dev/null
+++ b/Documentation/technical/api-sub-process.txt
@@ -0,0 +1,59 @@
+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.
diff --git a/Makefile b/Makefile
index eb1a1a7cff..97c87fb647 100644
--- a/Makefile
+++ b/Makefile
@@ -840,6 +840,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 cfae45aeee..79d04907a5 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,12 +497,6 @@ 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;
@@ -510,65 +505,6 @@ struct cmd2process {
 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(struct hashmap *hashmap, const char *cmd)
-{
-	struct subprocess_entry key;
-
-	hashmap_entry_init(&key, strhash(cmd));
-	key.cmd = cmd;
-	return hashmap_get(hashmap, &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 hashmap *hashmap, struct subprocess_entry *entry)
-{
-	if (!entry)
-		return;
-
-	entry->process.clean_on_exit = 0;
-	kill(entry->process.pid, SIGTERM);
-	finish_command(&entry->process);
-
-	hashmap_remove(hashmap, 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);
-}
-
 static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
 {
 	int err;
@@ -629,44 +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 hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,
-	subprocess_start_fn startfn)
-{
-	int err;
-	struct child_process *process;
-	const char *argv[] = { cmd, NULL };
-
-	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(hashmap, entry);
-		return err;
-	}
-
-	hashmap_add(hashmap, 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..536b60cced
--- /dev/null
+++ b/sub-process.c
@@ -0,0 +1,102 @@
+/*
+ * Generic implementation of background process infrastructure.
+ */
+#include "sub-process.h"
+#include "sigchain.h"
+#include "pkt-line.h"
+
+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(struct hashmap *hashmap, const char *cmd)
+{
+	struct subprocess_entry key;
+
+	hashmap_entry_init(&key, strhash(cmd));
+	key.cmd = cmd;
+	return hashmap_get(hashmap, &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 hashmap *hashmap, struct subprocess_entry *entry)
+{
+	if (!entry)
+		return;
+
+	entry->process.clean_on_exit = 0;
+	kill(entry->process.pid, SIGTERM);
+	finish_command(&entry->process);
+
+	hashmap_remove(hashmap, 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 hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,
+	subprocess_start_fn startfn)
+{
+	int err;
+	struct child_process *process;
+	const char *argv[] = { cmd, NULL };
+
+	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(hashmap, entry);
+		return err;
+	}
+
+	hashmap_add(hashmap, entry);
+	return 0;
+}
diff --git a/sub-process.h b/sub-process.h
new file mode 100644
index 0000000000..a88e782bfc
--- /dev/null
+++ b/sub-process.h
@@ -0,0 +1,49 @@
+#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 */
+
+int cmd2process_cmp(const struct subprocess_entry *e1,
+	const struct subprocess_entry *e2, const void *unused);
+
+typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
+int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,
+		subprocess_start_fn startfn);
+
+void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry);
+
+struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, 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.33.g243d9b384c

[PATCH v6 8/8] convert: Update subprocess_read_status to not die on EOF

[Ben Peart]

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 79d04907a5..f1e168bc30 100644
--- a/convert.c
+++ b/convert.c
@@ -635,7 +635,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;
@@ -644,7 +647,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 536b60cced..92f8aea70a 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -21,13 +21,15 @@ struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const ch
 	return hashmap_get(hashmap, &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 < 0) || !line)
 			break;
 		pair = strbuf_split_str(line, '=', 2);
 		if (pair[0] && pair[0]->len && pair[1]) {
@@ -39,6 +41,8 @@ void subprocess_read_status(int fd, struct strbuf *status)
 		}
 		strbuf_list_free(pair);
 	}
+
+	return (len < 0) ? len : 0;
 }
 
 void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry)
diff --git a/sub-process.h b/sub-process.h
index a88e782bfc..7d451e1cde 100644
--- a/sub-process.h
+++ b/sub-process.h
@@ -44,6 +44,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.33.g243d9b384c