patchsub-process: refactor handshake to common function

> Refactor, into a common function, the version and capability negotiation
> done when invoking a long-running process as a clean or smudge filter.
> This will be useful for other Git code that needs to interact similarly
> with a long-running process.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---

> --- a/sub-process.h
> +++ b/sub-process.h
> @@ -18,6 +18,11 @@ struct subprocess_entry {
>  	struct child_process process;
>  };
>  
> +struct subprocess_capability {
> +	const char *name;
> +	unsigned int flag;

> @@ -41,6 +46,19 @@ static inline struct child_process *subprocess_get_child_process(
>  	return &entry->process;
>  }
>  
> +/*
> + * Perform the handshake to a long-running process as described in the
> + * gitattributes documentation using the given requested versions and
> + * capabilities. The "versions" and "capabilities" parameters are arrays
> + * terminated by a 0 or blank struct.
> + */
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities);
> +

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

> --- a/convert.c
> +++ b/convert.c
> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;
>  
>  static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
>  {
> -	int err;
> -	struct cmd2process *entry = (struct cmd2process *)subprocess;

> -	return err;
> +	static int versions[] = {2, 0};
> +	static struct subprocess_capability capabilities[] = {
> +		{"clean", CAP_CLEAN}, {"smudge", CAP_SMUDGE}, {NULL, 0}
> +	};
> +	struct cmd2process *entry = (struct cmd2process *)subprocess;
> +
> +	return subprocess_handshake(subprocess, "git-filter-", versions, NULL,
> +				    capabilities,
> +				    &entry->supported_capabilities);
>  }

> Refactor, into a common function, the version and capability negotiation
> done when invoking a long-running process as a clean or smudge filter.
> This will be useful for other Git code that needs to interact similarly
> with a long-running process.
> 

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> This will be useful for anyone implementing functionality like that in
> [1].
> 
> It is unfortunate that the code grew larger because it had to be more
> generic, but I think this is better than having (in the future) 2 or
> more separate handshake functions.
> 

> I also don't think that the protocol should be permissive - I think
> things would be simpler if all protocol errors were fatal errors. As it
> is, most parts are permissive, but packet_read_line() already dies
> anyway upon a malformed packet, so it may not be too drastic a change to
> change this. For reference, the original protocol comes from [2].
> 
> [1] https://public-inbox.org/git/20170714132651.170708-2-benpeart@microsoft.com/
> [2] edcc858 ("convert: add filter.<driver>.process option", 2016-10-17)
> ---
>   convert.c             | 61 ++++-----------------------------
>   pkt-line.c            | 19 -----------
>   pkt-line.h            |  2 --
>   sub-process.c         | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   sub-process.h         | 18 ++++++++++
>   t/t0021-conversion.sh |  2 +-
>   6 files changed, 120 insertions(+), 76 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index deaf0ba7b..ec8ecc2ea 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;
>   
>   static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
>   {
> -	int err;
> +	static int versions[] = {2, 0};
> +	static struct subprocess_capability capabilities[] = {
> +		{"clean", CAP_CLEAN}, {"smudge", CAP_SMUDGE}, {NULL, 0}
> +	};
>   	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 = &subprocess->process;
> -	const char *cmd = subprocess->cmd;
> -
> -	sigchain_push(SIGPIPE, SIG_IGN);
> -
> -	err = packet_writel(process->in, "git-filter-client", "version=2", NULL);
> -	if (err)
> -		goto done;
> -
> -	err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
> -	if (err) {
> -		error("external filter '%s' does not support filter protocol version 2", cmd);
> -		goto done;
> -	}
> -	err = strcmp(packet_read_line(process->out, NULL), "version=2");
> -	if (err)
> -		goto done;
> -	err = packet_read_line(process->out, NULL) != NULL;
> -	if (err)
> -		goto done;
> -
> -	err = packet_writel(process->in, "capability=clean", "capability=smudge", NULL);
> -
> -	for (;;) {
> -		cap_buf = packet_read_line(process->out, NULL);
> -		if (!cap_buf)
> -			break;
> -		string_list_split_in_place(&cap_list, cap_buf, '=', 1);
> -
> -		if (cap_list.nr != 2 || strcmp(cap_list.items[0].string, "capability"))
> -			continue;
> -
> -		cap_name = cap_list.items[1].string;
> -		if (!strcmp(cap_name, "clean")) {
> -			entry->supported_capabilities |= CAP_CLEAN;
> -		} else if (!strcmp(cap_name, "smudge")) {
> -			entry->supported_capabilities |= CAP_SMUDGE;
> -		} else {
> -			warning(
> -				"external filter '%s' requested unsupported filter capability '%s'",
> -				cmd, cap_name
> -			);
> -		}
> -
> -		string_list_clear(&cap_list, 0);
> -	}
> -
> -done:
> -	sigchain_pop(SIGPIPE);
>   
> -	return err;
> +	return subprocess_handshake(subprocess, "git-filter-", versions, NULL,
> +				    capabilities,
> +				    &entry->supported_capabilities);
>   }
>   
>   static int apply_multi_file_filter(const char *path, const char *src, size_t len,
> diff --git a/pkt-line.c b/pkt-line.c
> index 9d845ecc3..7db911957 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
>   	return status;
>   }
>   
> -int packet_writel(int fd, const char *line, ...)
> -{

> -	va_list args;
> -	int err;
> -	va_start(args, line);
> -	for (;;) {
> -		if (!line)
> -			break;
> -		if (strlen(line) > LARGE_PACKET_DATA_MAX)
> -			return -1;
> -		err = packet_write_fmt_gently(fd, "%s\n", line);
> -		if (err)
> -			return err;
> -		line = va_arg(args, const char*);
> -	}
> -	va_end(args);
> -	return packet_flush_gently(fd);
> -}
> -
>   static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>   {
>   	static char packet_write_buffer[LARGE_PACKET_MAX];
> diff --git a/pkt-line.h b/pkt-line.h
> index 450183b64..66ef610fc 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);
>   void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
>   int packet_flush_gently(int fd);
>   int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> -LAST_ARG_MUST_BE_NULL
> -int packet_writel(int fd, const char *line, ...);
>   int write_packetized_from_fd(int fd_in, int fd_out);
>   int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
>   
> diff --git a/sub-process.c b/sub-process.c
> index a3cfab1a9..1a3f39bdf 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>   	hashmap_add(hashmap, entry);
>   	return 0;
>   }
> +
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities) {
> +	int version_scratch;
> +	unsigned int capabilities_scratch;
> +	struct child_process *process = &entry->process;
> +	int i;
> +	char *line;
> +	const char *p;
> +
> +	if (!chosen_version)
> +		chosen_version = &version_scratch;
> +	if (!supported_capabilities)
> +		supported_capabilities = &capabilities_scratch;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	if (packet_write_fmt_gently(process->in, "%sclient\n",
> +				    welcome_prefix)) {
> +		error("Could not write client identification");
> +		goto error;
> +	}
> +	for (i = 0; versions[i]; i++) {
> +		if (packet_write_fmt_gently(process->in, "version=%d\n",
> +					    versions[i])) {
> +			error("Could not write requested version");
> +			goto error;
> +		}
> +	}
> +	if (packet_flush_gently(process->in))
> +		goto error;
> +
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, welcome_prefix, &p) ||
> +	    strcmp(p, "server")) {
> +		error("Unexpected line '%s', expected %sserver",
> +		      line ? line : "<flush packet>", welcome_prefix);
> +		goto error;
> +	}
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, "version=", &p) ||
> +	    strtol_i(p, 10, chosen_version)) {
> +		error("Unexpected line '%s', expected version",
> +		      line ? line : "<flush packet>");
> +		goto error;
> +	}
> +	for (i = 0; versions[i]; i++) {
> +		if (versions[i] == *chosen_version)
> +			goto version_found;
> +	}

> +	error("Version %d not supported", *chosen_version);
> +	goto error;
> +version_found:
> +	if ((line = packet_read_line(process->out, NULL))) {
> +		error("Unexpected line '%s', expected flush", line);
> +		goto error;
> +	}
> +
> +	for (i = 0; capabilities[i].name; i++) {
> +		if (packet_write_fmt_gently(process->in, "capability=%s\n",
> +					    capabilities[i].name)) {
> +			error("Could not write requested capability");
> +			goto error;
> +		}
> +	}
> +	if (packet_flush_gently(process->in))
> +		goto error;
> +
> +	while ((line = packet_read_line(process->out, NULL))) {
> +		if (!skip_prefix(line, "capability=", &p))
> +			continue;
> +
> +		for (i = 0; capabilities[i].name; i++) {
> +			if (!strcmp(p, capabilities[i].name)) {
> +				*supported_capabilities |= capabilities[i].flag;
> +				goto capability_found;
> +			}
> +		}
> +		warning("external filter requested unsupported filter capability '%s'",
> +			p);
> +capability_found:
> +		;
> +	}
> +
> +	sigchain_pop(SIGPIPE);
> +	return 0;
> +error:
> +	sigchain_pop(SIGPIPE);
> +	return 1;
> +}
> diff --git a/sub-process.h b/sub-process.h
> index 96a2cca36..a72e7f7cf 100644
> --- a/sub-process.h
> +++ b/sub-process.h
> @@ -18,6 +18,11 @@ struct subprocess_entry {
>   	struct child_process process;
>   };
>   
> +struct subprocess_capability {
> +	const char *name;
> +	unsigned int flag;
> +};
> +
>   /* subprocess functions */
>   
>   extern int cmd2process_cmp(const void *unused_cmp_data,
> @@ -41,6 +46,19 @@ static inline struct child_process *subprocess_get_child_process(
>   	return &entry->process;
>   }
>   
> +/*
> + * Perform the handshake to a long-running process as described in the
> + * gitattributes documentation using the given requested versions and
> + * capabilities. The "versions" and "capabilities" parameters are arrays
> + * terminated by a 0 or blank struct.
> + */
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities);
> +
>   /*
>    * Helper function that will read packets looking for "status=<foo>"
>    * key/value pairs and return the value from the last "status" packet
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 161f56044..d191a7228 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '
>   
>   		cp "$TEST_ROOT/test.o" test.r &&
>   		test_must_fail git add . 2>git-stderr.log &&
> -		grep "does not support filter protocol version" git-stderr.log
> +		grep "expected git-filter-server" git-stderr.log
>   	)
>   '
>   
> 

> Christian Couder has been working on a similar refactoring for the perl 
> versions of very similar helper functions.  They can be found in the 
> following patch series:
> 
> https://public-inbox.org/git/20170620075523.26961-1-chriscool@tuxfamily.org/
> 
> In particular:
> 
>      - Patches 02/49 to 08/49 create a Git/Packet.pm module by
>        refactoring "t0021/rot13-filter.pl". Functions from this new
>        module will be used later in test scripts.
> 
> Some differences I noticed: this patch does both the version and 
> capability negotiation and it lives in "sub-process.h/c" files.  In the 
> perl script patch series, the version negotiation is in the new 
> "packet.pm" module but it does not include the capability negotiation.
> 
> It would make sense to me for these to have the same API and usage 
> behaviors.  Perhaps pull them together into a single patch series so 
> that we can review and keep them in sync?

> > It is unfortunate that the code grew larger because it had to be more
> > generic, but I think this is better than having (in the future) 2 or
> > more separate handshake functions.
> 
> I'm always happy to see an effort to refactor common code into reusable 
> APIs.  Its a good engineering practice that makes it easier to 
> stabilize, extend and maintain the code. The challenge is making sure 
> the common function supports all the requirements of the various callers 
> and that the increase in complexity doesn't outweigh the benefit of 
> centralizing the logic.

> > +int subprocess_handshake(struct subprocess_entry *entry,
> > +			 const char *welcome_prefix,
> > +			 int *versions,
> > +			 int *chosen_version,
> > +			 struct subprocess_capability *capabilities,
> > +			 unsigned int *supported_capabilities) {
> > +	int version_scratch;
> > +	unsigned int capabilities_scratch;
> > +	struct child_process *process = &entry->process;
> > +	int i;
> > +	char *line;
> > +	const char *p;
> > +
> > +	if (!chosen_version)
> > +		chosen_version = &version_scratch;
> > +	if (!supported_capabilities)
> > +		supported_capabilities = &capabilities_scratch;
> > +
> > +	sigchain_push(SIGPIPE, SIG_IGN);
> > +
> > +	if (packet_write_fmt_gently(process->in, "%sclient\n",
> > +				    welcome_prefix)) {
> > +		error("Could not write client identification");
> > +		goto error;
> > +	}
> > +	for (i = 0; versions[i]; i++) {
> > +		if (packet_write_fmt_gently(process->in, "version=%d\n",
> > +					    versions[i])) {
> > +			error("Could not write requested version");
> > +			goto error;
> > +		}
> > +	}
> > +	if (packet_flush_gently(process->in))
> > +		goto error;
> > +
> > +	if (!(line = packet_read_line(process->out, NULL)) ||
> > +	    !skip_prefix(line, welcome_prefix, &p) ||
> > +	    strcmp(p, "server")) {
> > +		error("Unexpected line '%s', expected %sserver",
> > +		      line ? line : "<flush packet>", welcome_prefix);
> > +		goto error;
> > +	}
> > +	if (!(line = packet_read_line(process->out, NULL)) ||
> > +	    !skip_prefix(line, "version=", &p) ||
> > +	    strtol_i(p, 10, chosen_version)) {
> > +		error("Unexpected line '%s', expected version",
> > +		      line ? line : "<flush packet>");
> > +		goto error;
> > +	}
> > +	for (i = 0; versions[i]; i++) {
> > +		if (versions[i] == *chosen_version)
> > +			goto version_found;
> > +	}
> 
> This doesn't look like it supports negotiating a common version between 
> the client and server.  If a client supports version 1, 2, and 3 and the 
> server only supports version 1 and 2, which version and capabilities 
> will be used?

> Refactor, into a common function, the version and capability negotiation
> done when invoking a long-running process as a clean or smudge filter.
> This will be useful for other Git code that needs to interact similarly
> with a long-running process.
> 
> As you can see in the change to t0021, this commit changes the error
> message reported when the long-running process does not introduce itself
> with the expected "server"-terminated line. Originally, the error
> message reports that the filter "does not support filter protocol
> version 2", differentiating between the old single-file filter protocol
> and the new multi-file filter protocol - I have updated it to something
> more generic and useful.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  convert.c             | 61 ++++-----------------------------
>  pkt-line.c            | 19 -----------
>  pkt-line.h            |  2 --
>  sub-process.c         | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  sub-process.h         | 26 ++++++++++++++
>  t/t0021-conversion.sh |  2 +-
>  6 files changed, 128 insertions(+), 76 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index deaf0ba7b..ec8ecc2ea 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;
>  
>  static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
>  {
> -	int err;
> +	static int versions[] = {2, 0};
> +	static struct subprocess_capability capabilities[] = {
> +		{"clean", CAP_CLEAN}, {"smudge", CAP_SMUDGE}, {NULL, 0}
> +	};
>  	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 = &subprocess->process;
> -	const char *cmd = subprocess->cmd;
> -
> -	sigchain_push(SIGPIPE, SIG_IGN);
> -
> -	err = packet_writel(process->in, "git-filter-client", "version=2", NULL);
> -	if (err)
> -		goto done;
> -
> -	err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
> -	if (err) {
> -		error("external filter '%s' does not support filter protocol version 2", cmd);
> -		goto done;
> -	}
> -	err = strcmp(packet_read_line(process->out, NULL), "version=2");
> -	if (err)
> -		goto done;
> -	err = packet_read_line(process->out, NULL) != NULL;
> -	if (err)
> -		goto done;
> -
> -	err = packet_writel(process->in, "capability=clean", "capability=smudge", NULL);
> -
> -	for (;;) {
> -		cap_buf = packet_read_line(process->out, NULL);
> -		if (!cap_buf)
> -			break;
> -		string_list_split_in_place(&cap_list, cap_buf, '=', 1);
> -
> -		if (cap_list.nr != 2 || strcmp(cap_list.items[0].string, "capability"))
> -			continue;
> -
> -		cap_name = cap_list.items[1].string;
> -		if (!strcmp(cap_name, "clean")) {
> -			entry->supported_capabilities |= CAP_CLEAN;
> -		} else if (!strcmp(cap_name, "smudge")) {
> -			entry->supported_capabilities |= CAP_SMUDGE;
> -		} else {
> -			warning(
> -				"external filter '%s' requested unsupported filter capability '%s'",
> -				cmd, cap_name
> -			);
> -		}
> -
> -		string_list_clear(&cap_list, 0);
> -	}
> -
> -done:
> -	sigchain_pop(SIGPIPE);
>  
> -	return err;
> +	return subprocess_handshake(subprocess, "git-filter-", versions, NULL,
> +				    capabilities,
> +				    &entry->supported_capabilities);
>  }
>  
>  static int apply_multi_file_filter(const char *path, const char *src, size_t len,
> diff --git a/pkt-line.c b/pkt-line.c
> index 9d845ecc3..7db911957 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
>  	return status;
>  }
>  
> -int packet_writel(int fd, const char *line, ...)
> -{
> -	va_list args;
> -	int err;
> -	va_start(args, line);
> -	for (;;) {
> -		if (!line)
> -			break;
> -		if (strlen(line) > LARGE_PACKET_DATA_MAX)
> -			return -1;
> -		err = packet_write_fmt_gently(fd, "%s\n", line);
> -		if (err)
> -			return err;
> -		line = va_arg(args, const char*);
> -	}
> -	va_end(args);
> -	return packet_flush_gently(fd);
> -}
> -
>  static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>  {
>  	static char packet_write_buffer[LARGE_PACKET_MAX];
> diff --git a/pkt-line.h b/pkt-line.h
> index 450183b64..66ef610fc 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
>  int packet_flush_gently(int fd);
>  int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> -LAST_ARG_MUST_BE_NULL
> -int packet_writel(int fd, const char *line, ...);
>  int write_packetized_from_fd(int fd_in, int fd_out);
>  int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
>  
> diff --git a/sub-process.c b/sub-process.c
> index a3cfab1a9..1a3f39bdf 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>  	hashmap_add(hashmap, entry);
>  	return 0;
>  }
> +
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities) {
> +	int version_scratch;
> +	unsigned int capabilities_scratch;
> +	struct child_process *process = &entry->process;
> +	int i;
> +	char *line;
> +	const char *p;
> +
> +	if (!chosen_version)
> +		chosen_version = &version_scratch;
> +	if (!supported_capabilities)
> +		supported_capabilities = &capabilities_scratch;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	if (packet_write_fmt_gently(process->in, "%sclient\n",
> +				    welcome_prefix)) {
> +		error("Could not write client identification");
> +		goto error;
> +	}
> +	for (i = 0; versions[i]; i++) {
> +		if (packet_write_fmt_gently(process->in, "version=%d\n",
> +					    versions[i])) {
> +			error("Could not write requested version");
> +			goto error;
> +		}
> +	}
> +	if (packet_flush_gently(process->in))
> +		goto error;
> +
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, welcome_prefix, &p) ||
> +	    strcmp(p, "server")) {
> +		error("Unexpected line '%s', expected %sserver",
> +		      line ? line : "<flush packet>", welcome_prefix);
> +		goto error;
> +	}
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, "version=", &p) ||
> +	    strtol_i(p, 10, chosen_version)) {
> +		error("Unexpected line '%s', expected version",
> +		      line ? line : "<flush packet>");
> +		goto error;
> +	}
> +	for (i = 0; versions[i]; i++) {
> +		if (versions[i] == *chosen_version)
> +			goto version_found;
> +	}
> +	error("Version %d not supported", *chosen_version);
> +	goto error;
> +version_found:
> +	if ((line = packet_read_line(process->out, NULL))) {
> +		error("Unexpected line '%s', expected flush", line);
> +		goto error;
> +	}
> +
> +	for (i = 0; capabilities[i].name; i++) {
> +		if (packet_write_fmt_gently(process->in, "capability=%s\n",
> +					    capabilities[i].name)) {
> +			error("Could not write requested capability");
> +			goto error;
> +		}
> +	}
> +	if (packet_flush_gently(process->in))
> +		goto error;
> +
> +	while ((line = packet_read_line(process->out, NULL))) {
> +		if (!skip_prefix(line, "capability=", &p))
> +			continue;
> +
> +		for (i = 0; capabilities[i].name; i++) {
> +			if (!strcmp(p, capabilities[i].name)) {
> +				*supported_capabilities |= capabilities[i].flag;
> +				goto capability_found;
> +			}
> +		}
> +		warning("external filter requested unsupported filter capability '%s'",
> +			p);
> +capability_found:
> +		;
> +	}

> +
> +	sigchain_pop(SIGPIPE);
> +	return 0;
> +error:
> +	sigchain_pop(SIGPIPE);
> +	return 1;
> +}
> diff --git a/sub-process.h b/sub-process.h
> index 9e6975b5e..28863fabc 100644
> --- a/sub-process.h
> +++ b/sub-process.h
> @@ -29,6 +29,16 @@ struct subprocess_entry {
>  	struct child_process process;
>  };
>  
> +struct subprocess_capability {
> +	const char *name;
> +
> +	/*
> +	 * subprocess_handshake will "|=" this value to supported_capabilities
> +	 * if the server reports that it supports this capability.
> +	 */
> +	unsigned int flag;
> +};
> +
>  /* subprocess functions */
>  
>  /* Function to test two subprocess hashmap entries for equality. */
> @@ -62,6 +72,22 @@ static inline struct child_process *subprocess_get_child_process(
>  	return &entry->process;
>  }
>  
> +/*
> + * Perform the version and capability negotiation as described in the "Long
> + * Running Filter Process" section of the gitattributes documentation using the
> + * given requested versions and capabilities. The "versions" and "capabilities"
> + * parameters are arrays terminated by a 0 or blank struct.
> + *
> + * This function is typically called when a subprocess is started (as part of
> + * the "startfn" passed to subprocess_start).
> + */
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities);
> +
>  /*
>   * Helper function that will read packets looking for "status=<foo>"
>   * key/value pairs and return the value from the last "status" packet
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 161f56044..d191a7228 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '
>  
>  		cp "$TEST_ROOT/test.o" test.r &&
>  		test_must_fail git add . 2>git-stderr.log &&
> -		grep "does not support filter protocol version" git-stderr.log
> +		grep "expected git-filter-server" git-stderr.log
>  	)
>  '
>  
> -- 
> 2.14.0.rc0.400.g1c36432dff-goog
> 

> Refactor, into a common function, the version and capability negotiation
> done when invoking a long-running process as a clean or smudge filter.
> This will be useful for other Git code that needs to interact similarly
> with a long-running process.
>
> As you can see in the change to t0021, this commit changes the error
> message reported when the long-running process does not introduce itself
> with the expected "server"-terminated line. Originally, the error
> message reports that the filter "does not support filter protocol
> version 2", differentiating between the old single-file filter protocol
> and the new multi-file filter protocol - I have updated it to something
> more generic and useful.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

> +	static int versions[] = {2, 0};
> +	static struct subprocess_capability capabilities[] = {
> +		{"clean", CAP_CLEAN}, {"smudge", CAP_SMUDGE}, {NULL, 0}
> +	};
>  	struct cmd2process *entry = (struct cmd2process *)subprocess;
> ...
> +	return subprocess_handshake(subprocess, "git-filter-", versions, NULL,
> +				    capabilities,
> +				    &entry->supported_capabilities);
>  }

> diff --git a/sub-process.c b/sub-process.c
> index a3cfab1a9..1a3f39bdf 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
>  	hashmap_add(hashmap, entry);
>  	return 0;
>  }
> +
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities) {
> +	int version_scratch;
> +	unsigned int capabilities_scratch;
> +	struct child_process *process = &entry->process;
> +	int i;
> +	char *line;
> +	const char *p;
> +
> +	if (!chosen_version)
> +		chosen_version = &version_scratch;
> +	if (!supported_capabilities)
> +		supported_capabilities = &capabilities_scratch;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	if (packet_write_fmt_gently(process->in, "%sclient\n",
> +				    welcome_prefix)) {
> +		error("Could not write client identification");
> +		goto error;
> +	}
> +	for (i = 0; versions[i]; i++) {
> +		if (packet_write_fmt_gently(process->in, "version=%d\n",
> +					    versions[i])) {
> +			error("Could not write requested version");
> +			goto error;
> +		}
> +	}

> +	if (packet_flush_gently(process->in))
> +		goto error;
> +
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, welcome_prefix, &p) ||
> +	    strcmp(p, "server")) {
> +		error("Unexpected line '%s', expected %sserver",
> +		      line ? line : "<flush packet>", welcome_prefix);
> +		goto error;
> +	}
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, "version=", &p) ||
> +	    strtol_i(p, 10, chosen_version)) {
> +		error("Unexpected line '%s', expected version",
> +		      line ? line : "<flush packet>");
> +		goto error;
> +	}
> +	for (i = 0; versions[i]; i++) {
> +		if (versions[i] == *chosen_version)
> +			goto version_found;
> +	}
> +	error("Version %d not supported", *chosen_version);
> +	goto error;
> +version_found:

> +	if ((line = packet_read_line(process->out, NULL))) {
> +		error("Unexpected line '%s', expected flush", line);
> +		goto error;
> +	}
> +
> +	for (i = 0; capabilities[i].name; i++) {
> +		if (packet_write_fmt_gently(process->in, "capability=%s\n",
> +					    capabilities[i].name)) {
> +			error("Could not write requested capability");
> +			goto error;
> +		}
> +	}
> +	if (packet_flush_gently(process->in))
> +		goto error;
> +
> +	while ((line = packet_read_line(process->out, NULL))) {
> +		if (!skip_prefix(line, "capability=", &p))
> +			continue;
> +
> +		for (i = 0; capabilities[i].name; i++) {
> +			if (!strcmp(p, capabilities[i].name)) {
> +				*supported_capabilities |= capabilities[i].flag;
> +				goto capability_found;
> +			}
> +		}
> +		warning("external filter requested unsupported filter capability '%s'",
> +			p);
> +capability_found:
> +		;

> +	}
> +
> +	sigchain_pop(SIGPIPE);
> +	return 0;
> +error:
> +	sigchain_pop(SIGPIPE);
> +	return 1;

> ---
>  Documentation/technical/api-sub-process.txt | 59 -----------------------------
>  sub-process.h                               | 25 +++++++++++-
>  2 files changed, 23 insertions(+), 61 deletions(-)
>  delete mode 100644 Documentation/technical/api-sub-process.txt
> 
> diff --git a/Documentation/technical/api-sub-process.txt b/Documentation/technical/api-sub-process.txt
> deleted file mode 100644
> index 793508cf3..000000000
> --- a/Documentation/technical/api-sub-process.txt
> +++ /dev/null
> @@ -1,59 +0,0 @@
> -sub-process API
> -===============
> -
> -The sub-process API makes it possible to run background sub-processes
> -for the entire lifetime of a Git invocation. If Git needs to communicate
> -with an external process multiple times, then this can reduces the process
> -invocation overhead. Git and the sub-process communicate through stdin and
> -stdout.
> -
> -The sub-processes are kept in a hashmap by command name and looked up
> -via the subprocess_find_entry function.  If an existing instance can not
> -be found then a new process should be created and started.  When the
> -parent git command terminates, all sub-processes are also terminated.
> -
> -This API is based on the run-command API.
> -
> -Data structures
> ----------------
> -
> -* `struct subprocess_entry`
> -
> -The sub-process structure.  Members should not be accessed directly.
> -
> -Types
> ------
> -
> -'int(*subprocess_start_fn)(struct subprocess_entry *entry)'::
> -
> -	User-supplied function to initialize the sub-process.  This is
> -	typically used to negotiate the interface version and capabilities.
> -
> -
> -Functions
> ----------
> -
> -`cmd2process_cmp`::
> -
> -	Function to test two subprocess hashmap entries for equality.
> -
> -`subprocess_start`::
> -
> -	Start a subprocess and add it to the subprocess hashmap.
> -
> -`subprocess_stop`::
> -
> -	Kill a subprocess and remove it from the subprocess hashmap.
> -
> -`subprocess_find_entry`::
> -
> -	Find a subprocess in the subprocess hashmap.
> -
> -`subprocess_get_child_process`::
> -
> -	Get the underlying `struct child_process` from a subprocess.
> -
> -`subprocess_read_status`::
> -
> -	Helper function to read packets looking for the last "status=<foo>"
> -	key/value pair.
> diff --git a/sub-process.h b/sub-process.h
> index 96a2cca36..9e6975b5e 100644
> --- a/sub-process.h
> +++ b/sub-process.h
> @@ -6,12 +6,23 @@
>  #include "run-command.h"
>  
>  /*
> - * Generic implementation of background process infrastructure.
> - * See: Documentation/technical/api-sub-process.txt
> + * 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 */
>  
> +/* Members should not be accessed directly. */
>  struct subprocess_entry {
>  	struct hashmap_entry ent; /* must be the first member! */
>  	const char *cmd;
> @@ -20,21 +31,31 @@ struct subprocess_entry {
>  
>  /* subprocess functions */
>  
> +/* Function to test two subprocess hashmap entries for equality. */
>  extern int cmd2process_cmp(const void *unused_cmp_data,
>  			   const struct subprocess_entry *e1,
>  			   const struct subprocess_entry *e2,
>  			   const void *unused_keydata);
>  
> +/*
> + * User-supplied function to initialize the sub-process.  This is
> + * typically used to negotiate the interface version and capabilities.
> + */
>  typedef int(*subprocess_start_fn)(struct subprocess_entry *entry);
> +
> +/* Start a subprocess and add it to the subprocess hashmap. */
>  int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, const char *cmd,
>  		subprocess_start_fn startfn);
>  
> +/* Kill a subprocess and remove it from the subprocess hashmap. */
>  void subprocess_stop(struct hashmap *hashmap, struct subprocess_entry *entry);
>  
> +/* Find a subprocess in the subprocess hashmap. */
>  struct subprocess_entry *subprocess_find_entry(struct hashmap *hashmap, const char *cmd);
>  
>  /* subprocess helper functions */
>  
> +/* Get the underlying `struct child_process` from a subprocess. */
>  static inline struct child_process *subprocess_get_child_process(
>  		struct subprocess_entry *entry)
>  {
> -- 
> 2.14.0.rc0.400.g1c36432dff-goog
> 

> On 24 Jul 2017, at 23:38, Jonathan Tan <jonathantanmy@google.com> wrote:
> 
> Refactor, into a common function, the version and capability negotiation
> done when invoking a long-running process as a clean or smudge filter.
> This will be useful for other Git code that needs to interact similarly
> with a long-running process.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> This will be useful for anyone implementing functionality like that in
> [1].
> 
> It is unfortunate that the code grew larger because it had to be more
> generic, but I think this is better than having (in the future) 2 or
> more separate handshake functions.
> 
> I also don't think that the protocol should be permissive - I think
> things would be simpler if all protocol errors were fatal errors. As it
> is, most parts are permissive, but packet_read_line() already dies
> anyway upon a malformed packet, so it may not be too drastic a change to
> change this. For reference, the original protocol comes from [2].
> 
> [1] https://public-inbox.org/git/20170714132651.170708-2-benpeart@microsoft.com/
> [2] edcc858 ("convert: add filter.<driver>.process option", 2016-10-17)

> ---
> convert.c             | 61 ++++-----------------------------
> pkt-line.c            | 19 -----------
> pkt-line.h            |  2 --
> sub-process.c         | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++
> sub-process.h         | 18 ++++++++++
> t/t0021-conversion.sh |  2 +-
> 6 files changed, 120 insertions(+), 76 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index deaf0ba7b..ec8ecc2ea 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -512,62 +512,15 @@ static struct hashmap subprocess_map;
> 
> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
> {
> -	int err;
> +	static int versions[] = {2, 0};
> +	static struct subprocess_capability capabilities[] = {
> +		{"clean", CAP_CLEAN}, {"smudge", CAP_SMUDGE}, {NULL, 0}
> +	};
> 	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 = &subprocess->process;
> -	const char *cmd = subprocess->cmd;
> -
> -	sigchain_push(SIGPIPE, SIG_IGN);
> -
> -	err = packet_writel(process->in, "git-filter-client", "version=2", NULL);
> -	if (err)
> -		goto done;
> -
> -	err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
> -	if (err) {
> -		error("external filter '%s' does not support filter protocol version 2", cmd);
> -		goto done;
> -	}
> -	err = strcmp(packet_read_line(process->out, NULL), "version=2");
> -	if (err)
> -		goto done;
> -	err = packet_read_line(process->out, NULL) != NULL;
> -	if (err)
> -		goto done;
> -
> -	err = packet_writel(process->in, "capability=clean", "capability=smudge", NULL);
> -
> -	for (;;) {
> -		cap_buf = packet_read_line(process->out, NULL);
> -		if (!cap_buf)
> -			break;
> -		string_list_split_in_place(&cap_list, cap_buf, '=', 1);
> -
> -		if (cap_list.nr != 2 || strcmp(cap_list.items[0].string, "capability"))
> -			continue;
> -
> -		cap_name = cap_list.items[1].string;
> -		if (!strcmp(cap_name, "clean")) {
> -			entry->supported_capabilities |= CAP_CLEAN;
> -		} else if (!strcmp(cap_name, "smudge")) {
> -			entry->supported_capabilities |= CAP_SMUDGE;
> -		} else {
> -			warning(
> -				"external filter '%s' requested unsupported filter capability '%s'",
> -				cmd, cap_name
> -			);
> -		}
> -
> -		string_list_clear(&cap_list, 0);
> -	}
> -
> -done:
> -	sigchain_pop(SIGPIPE);
> 
> -	return err;
> +	return subprocess_handshake(subprocess, "git-filter-", versions, NULL,
> +				    capabilities,
> +				    &entry->supported_capabilities);
> }
> 
> static int apply_multi_file_filter(const char *path, const char *src, size_t len,
> diff --git a/pkt-line.c b/pkt-line.c
> index 9d845ecc3..7db911957 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
> 	return status;
> }
> 
> -int packet_writel(int fd, const char *line, ...)
> -{
> -	va_list args;
> -	int err;
> -	va_start(args, line);
> -	for (;;) {
> -		if (!line)
> -			break;
> -		if (strlen(line) > LARGE_PACKET_DATA_MAX)
> -			return -1;
> -		err = packet_write_fmt_gently(fd, "%s\n", line);
> -		if (err)
> -			return err;
> -		line = va_arg(args, const char*);
> -	}
> -	va_end(args);
> -	return packet_flush_gently(fd);
> -}
> -
> static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> {
> 	static char packet_write_buffer[LARGE_PACKET_MAX];
> diff --git a/pkt-line.h b/pkt-line.h
> index 450183b64..66ef610fc 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);
> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> int packet_flush_gently(int fd);
> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> -LAST_ARG_MUST_BE_NULL
> -int packet_writel(int fd, const char *line, ...);
> int write_packetized_from_fd(int fd_in, int fd_out);
> int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
> 
> diff --git a/sub-process.c b/sub-process.c
> index a3cfab1a9..1a3f39bdf 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
> 	hashmap_add(hashmap, entry);
> 	return 0;
> }
> +
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities) {
> +	int version_scratch;
> +	unsigned int capabilities_scratch;
> +	struct child_process *process = &entry->process;
> +	int i;
> +	char *line;
> +	const char *p;
> +
> +	if (!chosen_version)
> +		chosen_version = &version_scratch;
> +	if (!supported_capabilities)
> +		supported_capabilities = &capabilities_scratch;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	if (packet_write_fmt_gently(process->in, "%sclient\n",
> +				    welcome_prefix)) {
> +		error("Could not write client identification");
> +		goto error;
> +	}
> +	for (i = 0; versions[i]; i++) {
> +		if (packet_write_fmt_gently(process->in, "version=%d\n",
> +					    versions[i])) {
> +			error("Could not write requested version");
> +			goto error;
> +		}
> +	}
> +	if (packet_flush_gently(process->in))
> +		goto error;
> +
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, welcome_prefix, &p) ||
> +	    strcmp(p, "server")) {
> +		error("Unexpected line '%s', expected %sserver",
> +		      line ? line : "<flush packet>", welcome_prefix);
> +		goto error;
> +	}
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, "version=", &p) ||
> +	    strtol_i(p, 10, chosen_version)) {
> +		error("Unexpected line '%s', expected version",
> +		      line ? line : "<flush packet>");
> +		goto error;
> +	}
> +	for (i = 0; versions[i]; i++) {
> +		if (versions[i] == *chosen_version)
> +			goto version_found;
> +	}
> +	error("Version %d not supported", *chosen_version);
> +	goto error;
> +version_found:
> +	if ((line = packet_read_line(process->out, NULL))) {
> +		error("Unexpected line '%s', expected flush", line);
> +		goto error;
> +	}
> +
> +	for (i = 0; capabilities[i].name; i++) {
> +		if (packet_write_fmt_gently(process->in, "capability=%s\n",
> +					    capabilities[i].name)) {
> +			error("Could not write requested capability");
> +			goto error;
> +		}
> +	}
> +	if (packet_flush_gently(process->in))
> +		goto error;
> +
> +	while ((line = packet_read_line(process->out, NULL))) {
> +		if (!skip_prefix(line, "capability=", &p))
> +			continue;
> +
> +		for (i = 0; capabilities[i].name; i++) {
> +			if (!strcmp(p, capabilities[i].name)) {
> +				*supported_capabilities |= capabilities[i].flag;
> +				goto capability_found;
> +			}
> +		}
> +		warning("external filter requested unsupported filter capability '%s'",
> +			p);
> +capability_found:
> +		;
> +	}
> +
> +	sigchain_pop(SIGPIPE);
> +	return 0;
> +error:
> +	sigchain_pop(SIGPIPE);
> +	return 1;
> +}
> diff --git a/sub-process.h b/sub-process.h
> index 96a2cca36..a72e7f7cf 100644
> --- a/sub-process.h
> +++ b/sub-process.h
> @@ -18,6 +18,11 @@ struct subprocess_entry {
> 	struct child_process process;
> };
> 
> +struct subprocess_capability {
> +	const char *name;
> +	unsigned int flag;
> +};
> +
> /* subprocess functions */
> 
> extern int cmd2process_cmp(const void *unused_cmp_data,
> @@ -41,6 +46,19 @@ static inline struct child_process *subprocess_get_child_process(
> 	return &entry->process;
> }
> 
> +/*
> + * Perform the handshake to a long-running process as described in the
> + * gitattributes documentation using the given requested versions and
> + * capabilities. The "versions" and "capabilities" parameters are arrays
> + * terminated by a 0 or blank struct.
> + */
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities);
> +
> /*
>  * Helper function that will read packets looking for "status=<foo>"
>  * key/value pairs and return the value from the last "status" packet
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 161f56044..d191a7228 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '
> 
> 		cp "$TEST_ROOT/test.o" test.r &&
> 		test_must_fail git add . 2>git-stderr.log &&
> -		grep "does not support filter protocol version" git-stderr.log
> +		grep "expected git-filter-server" git-stderr.log
> 	)
> '
> 
> -- 
> 2.14.0.rc0.400.g1c36432dff-goog
> 

> Please note that I've recently refactored the capabilities negotiation a bit:
> https://github.com/git/git/commit/1514c8edd62d96006cd1de31e906ed5798dd4681
>
> This change is still cooking in `next`. I am not sure how this should/could
> be handled but maybe you can use my refactoring as your base?

> On 26 Jul 2017, at 20:17, Jonathan Tan <jonathantanmy@google.com> wrote:
> 
> Refactor, into a common function, the version and capability negotiation
> done when invoking a long-running process as a clean or smudge filter.
> This will be useful for other Git code that needs to interact similarly
> with a long-running process.
> 
> As you can see in the change to t0021, this commit changes the error
> message reported when the long-running process does not introduce itself
> with the expected "server"-terminated line. Originally, the error
> message reports that the filter "does not support filter protocol
> version 2", differentiating between the old single-file filter protocol
> and the new multi-file filter protocol - I have updated it to something
> more generic and useful.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> convert.c             |  75 ++++--------------------------------
> pkt-line.c            |  19 ----------
> pkt-line.h            |   2 -
> sub-process.c         | 103 ++++++++++++++++++++++++++++++++++++++++++++++++++
> sub-process.h         |  26 +++++++++++++
> t/t0021-conversion.sh |   2 +-
> 6 files changed, 137 insertions(+), 90 deletions(-)
> 
> diff --git a/convert.c b/convert.c
> index dbdbb24e4..1012462e3 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -513,78 +513,17 @@ static struct hashmap subprocess_map;
> 
> static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
> {
> -	int err, i;
> -	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 = &subprocess->process;
> -	const char *cmd = subprocess->cmd;
> -
> -	static const struct {
> -		const char *name;
> -		unsigned int cap;
> -	} known_caps[] = {
> +	static int versions[] = {2, 0};
> +	static struct subprocess_capability capabilities[] = {
> 		{ "clean",  CAP_CLEAN  },
> 		{ "smudge", CAP_SMUDGE },
> 		{ "delay",  CAP_DELAY  },
> +		{ NULL, 0 }
> 	};
> -
> -	sigchain_push(SIGPIPE, SIG_IGN);
> -
> -	err = packet_writel(process->in, "git-filter-client", "version=2", NULL);
> -	if (err)
> -		goto done;
> -
> -	err = strcmp(packet_read_line(process->out, NULL), "git-filter-server");
> -	if (err) {
> -		error("external filter '%s' does not support filter protocol version 2", cmd);
> -		goto done;
> -	}
> -	err = strcmp(packet_read_line(process->out, NULL), "version=2");
> -	if (err)
> -		goto done;
> -	err = packet_read_line(process->out, NULL) != NULL;
> -	if (err)
> -		goto done;
> -
> -	for (i = 0; i < ARRAY_SIZE(known_caps); ++i) {
> -		err = packet_write_fmt_gently(
> -			process->in, "capability=%s\n", known_caps[i].name);
> -		if (err)
> -			goto done;
> -	}
> -	err = packet_flush_gently(process->in);
> -	if (err)
> -		goto done;
> -
> -	for (;;) {
> -		cap_buf = packet_read_line(process->out, NULL);
> -		if (!cap_buf)
> -			break;
> -		string_list_split_in_place(&cap_list, cap_buf, '=', 1);
> -
> -		if (cap_list.nr != 2 || strcmp(cap_list.items[0].string, "capability"))
> -			continue;
> -
> -		cap_name = cap_list.items[1].string;
> -		i = ARRAY_SIZE(known_caps) - 1;
> -		while (i >= 0 && strcmp(cap_name, known_caps[i].name))
> -			i--;
> -
> -		if (i >= 0)
> -			entry->supported_capabilities |= known_caps[i].cap;
> -		else
> -			warning("external filter '%s' requested unsupported filter capability '%s'",
> -			cmd, cap_name);
> -
> -		string_list_clear(&cap_list, 0);
> -	}
> -
> -done:
> -	sigchain_pop(SIGPIPE);
> -
> -	return err;
> +	struct cmd2process *entry = (struct cmd2process *)subprocess;
> +	return subprocess_handshake(subprocess, "git-filter", versions, NULL,
> +				    capabilities,
> +				    &entry->supported_capabilities);

> }
> 
> static void handle_filter_error(const struct strbuf *filter_status,
> diff --git a/pkt-line.c b/pkt-line.c
> index 9d845ecc3..7db911957 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,25 +171,6 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
> 	return status;
> }
> 
> -int packet_writel(int fd, const char *line, ...)
> -{
> -	va_list args;
> -	int err;
> -	va_start(args, line);
> -	for (;;) {
> -		if (!line)
> -			break;
> -		if (strlen(line) > LARGE_PACKET_DATA_MAX)
> -			return -1;
> -		err = packet_write_fmt_gently(fd, "%s\n", line);
> -		if (err)
> -			return err;
> -		line = va_arg(args, const char*);
> -	}
> -	va_end(args);
> -	return packet_flush_gently(fd);
> -}
> -
> static int packet_write_gently(const int fd_out, const char *buf, size_t size)
> {
> 	static char packet_write_buffer[LARGE_PACKET_MAX];
> diff --git a/pkt-line.h b/pkt-line.h
> index 450183b64..66ef610fc 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,8 +25,6 @@ void packet_buf_flush(struct strbuf *buf);
> void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> int packet_flush_gently(int fd);
> int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
> -LAST_ARG_MUST_BE_NULL
> -int packet_writel(int fd, const char *line, ...);
> int write_packetized_from_fd(int fd_in, int fd_out);
> int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
> 
> diff --git a/sub-process.c b/sub-process.c
> index 6cbffa440..37b4bd0ad 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -108,3 +108,106 @@ int subprocess_start(struct hashmap *hashmap, struct subprocess_entry *entry, co
> 	hashmap_add(hashmap, entry);
> 	return 0;
> }
> +
> +static int handshake_version(struct child_process *process,
> +			     const char *welcome_prefix, int *versions,

> +			     int *chosen_version)
> +{
> +	int version_scratch;
> +	int i;
> +	char *line;
> +	const char *p;
> +
> +	if (!chosen_version)
> +		chosen_version = &version_scratch;

> +
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, welcome_prefix, &p) ||
> +	    strcmp(p, "-server"))
> +		return error("Unexpected line '%s', expected %s-server",
> +			     line ? line : "<flush packet>", welcome_prefix);
> +	if (!(line = packet_read_line(process->out, NULL)) ||
> +	    !skip_prefix(line, "version=", &p) ||
> +	    strtol_i(p, 10, chosen_version))

> +			     line ? line : "<flush packet>");
> +	if ((line = packet_read_line(process->out, NULL)))
> +		return error("Unexpected line '%s', expected flush", line);
> +
> +	/* Check to make sure that the version received is supported */
> +	for (i = 0; versions[i]; i++) {
> +		if (versions[i] == *chosen_version)
> +			break;
> +	}
> +	if (!versions[i])
> +		return error("Version %d not supported", *chosen_version);
> +
> +	return 0;
> +}
> +
> +static int handshake_capabilities(struct child_process *process,
> +				  struct subprocess_capability *capabilities,
> +				  unsigned int *supported_capabilities)

> +{
> +	int i;
> +	char *line;
> +
> +	for (i = 0; capabilities[i].name; i++) {
> +		if (packet_write_fmt_gently(process->in, "capability=%s\n",
> +					    capabilities[i].name))
> +			return error("Could not write requested capability");

> +		for (i = 0;
> +		     capabilities[i].name && strcmp(p, capabilities[i].name);
> +		     i++)
> +			;
> +		if (capabilities[i].name) {
> +			if (supported_capabilities)
> +				*supported_capabilities |= capabilities[i].flag;
> +		} else {
> +			warning("external filter requested unsupported filter capability '%s'",
> +				p);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities) {
> +	int retval;
> +	struct child_process *process = &entry->process;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	retval = handshake_version(process, welcome_prefix, versions,
> +				   chosen_version) ||
> +		 handshake_capabilities(process, capabilities,
> +					supported_capabilities);
> +
> +	sigchain_pop(SIGPIPE);
> +	return retval;
> +}
> diff --git a/sub-process.h b/sub-process.h
> index d37c1499a..6857eb1b5 100644
> --- a/sub-process.h
> +++ b/sub-process.h
> @@ -29,6 +29,16 @@ struct subprocess_entry {
> 	struct child_process process;
> };
> 
> +struct subprocess_capability {
> +	const char *name;
> +
> +	/*
> +	 * subprocess_handshake will "|=" this value to supported_capabilities
> +	 * if the server reports that it supports this capability.
> +	 */
> +	unsigned int flag;
> +};
> +
> /* subprocess functions */
> 
> /* Function to test two subprocess hashmap entries for equality. */
> @@ -62,6 +72,22 @@ static inline struct child_process *subprocess_get_child_process(
> 	return &entry->process;
> }
> 
> +/*
> + * Perform the version and capability negotiation as described in the "Long
> + * Running Filter Process" section of the gitattributes documentation using the
> + * given requested versions and capabilities. The "versions" and "capabilities"
> + * parameters are arrays terminated by a 0 or blank struct.

> + *
> + * This function is typically called when a subprocess is started (as part of
> + * the "startfn" passed to subprocess_start).
> + */
> +int subprocess_handshake(struct subprocess_entry *entry,
> +			 const char *welcome_prefix,
> +			 int *versions,
> +			 int *chosen_version,
> +			 struct subprocess_capability *capabilities,
> +			 unsigned int *supported_capabilities);
> +
> /*
>  * Helper function that will read packets looking for "status=<foo>"
>  * key/value pairs and return the value from the last "status" packet
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index eb3d83744..46f8e583c 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -697,7 +697,7 @@ test_expect_success PERL 'invalid process filter must fail (and not hang!)' '
> 
> 		cp "$TEST_ROOT/test.o" test.r &&
> 		test_must_fail git add . 2>git-stderr.log &&
> -		grep "does not support filter protocol version" git-stderr.log
> +		grep "expected git-filter-server" git-stderr.log
> 	)
> '
> 

> > +	struct cmd2process *entry = (struct cmd2process *)subprocess;
> > +	return subprocess_handshake(subprocess, "git-filter", versions, NULL,
> > +				    capabilities,
> > +				    &entry->supported_capabilities);
> 
> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?

> > +
> > +static int handshake_version(struct child_process *process,
> > +			     const char *welcome_prefix, int *versions,
> 
> Maybe it would be less ambiguous if we call it `supported_versions` ? 

> > +			     int *chosen_version)
> > +{
> > +	int version_scratch;
> > +	int i;
> > +	char *line;
> > +	const char *p;
> > +
> > +	if (!chosen_version)
> > +		chosen_version = &version_scratch;
> 
> I am not an C expert but wouldn't 'version_scratch' go out of scope as soon
> as the function returns? Why don't you error here right away?

> > +	if (packet_write_fmt_gently(process->in, "%s-client\n",
> > +				    welcome_prefix))
> > +		return error("Could not write client identification");
> 
> Nit: Would it make sense to rename `welcome_prefix` to `client_id`?
> Alternatively, could we rename the error messages to "welcome prefix"?

> > +	for (i = 0; versions[i]; i++) {
> > +		if (packet_write_fmt_gently(process->in, "version=%d\n",
> > +					    versions[i]))
> > +			return error("Could not write requested version");
> 
> Maybe: "Could not write supported versions"?

> > +	}
> > +	if (packet_flush_gently(process->in))
> > +		return error("Could not write flush packet");
> 
> I feel this error is too generic.
> Maybe: "Could not finish writing supported versions"?

> > +
> > +	if (!(line = packet_read_line(process->out, NULL)) ||
> > +	    !skip_prefix(line, welcome_prefix, &p) ||
> > +	    strcmp(p, "-server"))
> > +		return error("Unexpected line '%s', expected %s-server",
> > +			     line ? line : "<flush packet>", welcome_prefix);
> > +	if (!(line = packet_read_line(process->out, NULL)) ||
> > +	    !skip_prefix(line, "version=", &p) ||
> > +	    strtol_i(p, 10, chosen_version))
> 
> Maybe `strlen("version=")` would be more clear than 10?

> > +static int handshake_capabilities(struct child_process *process,
> > +				  struct subprocess_capability *capabilities,
> > +				  unsigned int *supported_capabilities)
> 
> I feel the naming could be misleading. I think ...
> `capabilities` is really `supported_capabilities` 
> and 
> `supported_capabilities` is really `negiotated_capabilties` or `agreed_capabilites`

> > +	for (i = 0; capabilities[i].name; i++) {
> > +		if (packet_write_fmt_gently(process->in, "capability=%s\n",
> > +					    capabilities[i].name))
> > +			return error("Could not write requested capability");
> 
> I think this should be "Could not write supported capability", no?

> > +	}
> > +	if (packet_flush_gently(process->in))
> > +		return error("Could not write flush packet");
> 
> Maybe " "Could not finish writing supported capability" ?

> > +	while ((line = packet_read_line(process->out, NULL))) {
> > +		const char *p;
> > +		if (!skip_prefix(line, "capability=", &p))
> > +			continue;
> 
> Shouldn't we write an error in this case?

> > +/*
> > + * Perform the version and capability negotiation as described in the "Long
> > + * Running Filter Process" section of the gitattributes documentation using the
> > + * given requested versions and capabilities. The "versions" and "capabilities"
> > + * parameters are arrays terminated by a 0 or blank struct.
> 
> Should we reference the "gitattributes docs" if we want to make this generic?
> I thought "technical/api-sub-process.txt" would explain this kind of stuff
> and I was surprised that you deleted it in an earlier patch.

> On 07 Aug 2017, at 19:21, Jonathan Tan <jonathantanmy@google.com> wrote:
> 
> On Sun, 6 Aug 2017 21:58:24 +0200
> Lars Schneider <larsxschneider@gmail.com> wrote:
> 
>>> +	struct cmd2process *entry = (struct cmd2process *)subprocess;
>>> +	return subprocess_handshake(subprocess, "git-filter", versions, NULL,
>>> +				    capabilities,
>>> +				    &entry->supported_capabilities);
>> 
>> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?
> 
> The members of "struct subprocess_entry" are not supposed to be accessed
> directly, according to the documentation. If we relaxed that, then we
> could do this, but before that I think it's better to let the caller
> handle it.

>>> 
>>> +static int handshake_version(struct child_process *process,
>>> +			     const char *welcome_prefix, int *versions,
>> 
>> Maybe it would be less ambiguous if we call it `supported_versions` ? 
> 
> I thought of that, but I think "supported_versions" is actually more
> ambiguous, since we don't know if these are versions supported by the
> server or client or both.

>>> +			     int *chosen_version)
>>> +{
>>> +	int version_scratch;
>>> +	int i;
>>> +	char *line;
>>> +	const char *p;
>>> +
>>> +	if (!chosen_version)
>>> +		chosen_version = &version_scratch;
>> 
>> I am not an C expert but wouldn't 'version_scratch' go out of scope as soon
>> as the function returns? Why don't you error here right away?
> 
> It does, but so does chosen_version. This is meant to allow the caller
> to pass NULL to this function.

>>> +	if (packet_write_fmt_gently(process->in, "%s-client\n",
>>> +				    welcome_prefix))
>>> +		return error("Could not write client identification");
>> 
>> Nit: Would it make sense to rename `welcome_prefix` to `client_id`?
>> Alternatively, could we rename the error messages to "welcome prefix"?
> 
> I was retaining the existing terminology, but your suggestions seem
> reasonable. This might be best done in another patch once this series
> lands in master, though.

>>> +	for (i = 0; versions[i]; i++) {
>>> +		if (packet_write_fmt_gently(process->in, "version=%d\n",
>>> +					    versions[i]))
>>> +			return error("Could not write requested version");
>> 
>> Maybe: "Could not write supported versions"?
> 
> Same as above - "supported" is ambiguous.
> 
>>> +	}
>>> +	if (packet_flush_gently(process->in))
>>> +		return error("Could not write flush packet");
>> 
>> I feel this error is too generic.
>> Maybe: "Could not finish writing supported versions"?
> 
> That's reasonable. This is a rare error, though, and if it does occur, I
> think this message is more informative. But I'm OK either way.

> 
>>> +
>>> +	if (!(line = packet_read_line(process->out, NULL)) ||
>>> +	    !skip_prefix(line, welcome_prefix, &p) ||
>>> +	    strcmp(p, "-server"))
>>> +		return error("Unexpected line '%s', expected %s-server",
>>> +			     line ? line : "<flush packet>", welcome_prefix);
>>> +	if (!(line = packet_read_line(process->out, NULL)) ||
>>> +	    !skip_prefix(line, "version=", &p) ||
>>> +	    strtol_i(p, 10, chosen_version))
>> 
>> Maybe `strlen("version=")` would be more clear than 10?
> 
> The 10 here is the base, not the length. If there's a better way to
> convert strings to integers, let me know.

> 
>>> +		return error("Unexpected line '%s', expected version",
>> 
>> Maybe "... expected version number" ?
> 
> I'm fine either way.
> 
>>> +static int handshake_capabilities(struct child_process *process,
>>> +				  struct subprocess_capability *capabilities,
>>> +				  unsigned int *supported_capabilities)
>> 
>> I feel the naming could be misleading. I think ...
>> `capabilities` is really `supported_capabilities` 
>> and 
>> `supported_capabilities` is really `negiotated_capabilties` or `agreed_capabilites`
> 
> These "supported capabilities" are those supported by both the client
> (Git) and the server (the process Git is invoking). I think it's better
> to use this term for the intersection of capabilities, rather than
> exclusively for the client or server.
> 
>>> +	for (i = 0; capabilities[i].name; i++) {
>>> +		if (packet_write_fmt_gently(process->in, "capability=%s\n",
>>> +					    capabilities[i].name))
>>> +			return error("Could not write requested capability");
>> 
>> I think this should be "Could not write supported capability", no?
> 
> Same comment as above.
> 
>>> +	}
>>> +	if (packet_flush_gently(process->in))
>>> +		return error("Could not write flush packet");
>> 
>> Maybe " "Could not finish writing supported capability" ?
> 
> Same comment as the one about writing flush packets above.
> 
>>> +	while ((line = packet_read_line(process->out, NULL))) {
>>> +		const char *p;
>>> +		if (!skip_prefix(line, "capability=", &p))
>>> +			continue;
>> 
>> Shouldn't we write an error in this case?
> 
> I'm preserving the existing behavior.

> 
> > On 07 Aug 2017, at 19:21, Jonathan Tan <jonathantanmy@google.com> wrote:
> > 
> > On Sun, 6 Aug 2017 21:58:24 +0200
> > Lars Schneider <larsxschneider@gmail.com> wrote:
> > 
> >>> +	struct cmd2process *entry = (struct cmd2process *)subprocess;
> >>> +	return subprocess_handshake(subprocess, "git-filter", versions, NULL,
> >>> +				    capabilities,
> >>> +				    &entry->supported_capabilities);
> >> 
> >> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?
> > 
> > The members of "struct subprocess_entry" are not supposed to be accessed
> > directly, according to the documentation. If we relaxed that, then we
> > could do this, but before that I think it's better to let the caller
> > handle it.
> 
> @Ben: You wrote that " Members should not be accessed directly.":
> https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22
> Can you give me a hint why?
> 
> @Jonathan: What do you mean by "it's better to let the caller handle it"

> > It does, but so does chosen_version. This is meant to allow the caller
> > to pass NULL to this function.
> 
> Hm. I think every protocol should be versioned otherwise we could run
> into trouble in the long run.
> 
> TBH I wouldn't support NULL in that case in the first place. If you
> want to support it then I think we should document it.

> On Mon, 7 Aug 2017 19:51:04 +0200
> Lars Schneider <larsxschneider@gmail.com> wrote:
> 
>>
>>> On 07 Aug 2017, at 19:21, Jonathan Tan <jonathantanmy@google.com> wrote:
>>>
>>> On Sun, 6 Aug 2017 21:58:24 +0200
>>> Lars Schneider <larsxschneider@gmail.com> wrote:
>>>
>>>>> +	struct cmd2process *entry = (struct cmd2process *)subprocess;
>>>>> +	return subprocess_handshake(subprocess, "git-filter", versions, NULL,
>>>>> +				    capabilities,
>>>>> +				    &entry->supported_capabilities);
>>>>
>>>> Wouldn't it make sense to add `supported_capabilities` to `struct subprocess_entry` ?
>>>
>>> The members of "struct subprocess_entry" are not supposed to be accessed
>>> directly, according to the documentation. If we relaxed that, then we
>>> could do this, but before that I think it's better to let the caller
>>> handle it.
>>
>> @Ben: You wrote that " Members should not be accessed directly.":
>> https://github.com/git/git/commit/99605d62e8e7e568035dc953b24b79b3d52f0522#diff-c1655ad5d68943a3dc5bfae8c98466f2R22
>> Can you give me a hint why?
>>

>> @Jonathan: What do you mean by "it's better to let the caller handle it"
> 
> Let the caller provide their own place to store the capabilities, I
> mean, instead of (say) using a field as you describe and an accessor
> method.
> 
> I don't feel strongly about this, though.
> 
>>> It does, but so does chosen_version. This is meant to allow the caller
>>> to pass NULL to this function.
>>
>> Hm. I think every protocol should be versioned otherwise we could run
>> into trouble in the long run.
>>
>> TBH I wouldn't support NULL in that case in the first place. If you
>> want to support it then I think we should document it.
> 
> Note that this NULL is for the chosen version as chosen by the server,
> not the versions declared as supported by the client.
> 
> The protocol is versioned. Some users (e.g. the filter mechanism) of
> this subprocess thing would want to pass NULL because they only support
> one version and the subprocess thing already ensures that the server
> report that it supports one of the versions sent.
> 

> Thanks for all your comments.
>
> This is now built off "next" to include Lars's changes.
>
> About whether this is too restrictive (for example, as Junio mentions,
> this will not allow future capabilities of the form "key=???"), I think
> that this can be upgraded later if necessary. For now, both the filter
> code and the ODB (and ODB-like) proposals on the mailing list do not
> require such a thing, so I have not included that functionality.

>
> Changes from v2:
>  - now rebased onto next, to pick up Lars's changes
>  - split up into more functions
>  - welcome prefix does not include final dash
>  - no more gotos in expected cases (or at all)
>
> Jonathan Tan (2):
>   Documentation: migrate sub-process docs to header
>   sub-process: refactor handshake to common function
>
>  Documentation/technical/api-sub-process.txt |  59 ----------------
>  convert.c                                   |  75 ++------------------
>  pkt-line.c                                  |  19 -----
>  pkt-line.h                                  |   2 -
>  sub-process.c                               | 103 ++++++++++++++++++++++++++++
>  sub-process.h                               |  51 +++++++++++++-
>  t/t0021-conversion.sh                       |   2 +-
>  7 files changed, 160 insertions(+), 151 deletions(-)
>  delete mode 100644 Documentation/technical/api-sub-process.txt