Re: [PATCH v2 2/2] sub-process: refactor handshake to common function

[Junio C Hamano]

Jonathan Tan  writes:

> 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 

Overall I like the direction, even though the abstraction the
resulting code results in seems to me a bit too tightly defined; in
other words, I cannot be sure that this will be useful enough in a
more general context, or make some potential applications feel a bit
too constrained.

> + 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,
> + >supported_capabilities);
>  }

I would have defined the welcome prefix to lack the final dash,
i.e. forcing the hardcoded suffixes for clients and servers in any
protocol that uses this API to end with "-client" and "-server",
i.e. with dash.

> 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 = >process;
> + int i;
> + char *line;
> + const char *p;
> +
> + if (!chosen_version)
> + chosen_version = _scratch;
> + if (!supported_capabilities)
> + supported_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;
> + }
> + }

This forces version numbers to be positive integers, which is OK, as
I do not see it a downside that any potential application cannot use
"version=0".

> + if (packet_flush_gently(process->in))
> + goto error;
> +
> + if (!(line = packet_read_line(process->out, NULL)) ||
> + !skip_prefix(line, welcome_prefix, ) ||
> + strcmp(p, "server")) {
> + error("Unexpected line '%s', expected %sserver",
> +   line ? line : "", welcome_prefix);
> + goto error;
> + }
> + if (!(line = packet_read_line(process->out, NULL)) ||
> + !skip_prefix(line, "version=", ) ||
> + strtol_i(p, 10, chosen_version)) {
> + error("Unexpected line '%s', expected version",
> +   line ? line : "");
> + 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:

It would have been more natural to do

for (i = 0; versions[i]; i++)
if (versions[i] == *chosen_version)
break;
if (versions[i]) {
error("...");
goto error;
}

without "version_found:" label.  In general, I'd prefer to avoid
jumping to a label in the normal/expected case and reserve "goto"
for error handling.

> + if ((line = packet_read_line(process->out, NULL))) {
> + error("Unexpected line '%s', expected flush", line);
> + goto error;
> + }
> +
> + 

Re: [PATCH v2 2/2] sub-process: refactor handshake to common function

[Brandon Williams]

On 07/25, Jonathan Tan 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 
> ---
>  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 = >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(_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(_list, 0);
> - }
> -
> -done:
> - sigchain_pop(SIGPIPE);
>  
> - return err;
> + return subprocess_handshake(subprocess, "git-filter-", versions, NULL,
> + capabilities,
> + >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

[PATCH v2 2/2] sub-process: refactor handshake to common function

[Jonathan Tan]

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 
---
 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 = >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(_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(_list, 0);
-   }
-
-done:
-   sigchain_pop(SIGPIPE);
 
-   return err;
+   return subprocess_handshake(subprocess, "git-filter-", versions, NULL,
+   capabilities,
+   >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