> On 24 Jul 2017, at 23:38, Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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@xxxxxxxxxx> > --- > 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@xxxxxxxxxxxxx/ > [2] edcc858 ("convert: add filter.<driver>.process option", 2016-10-17) Thanks for this refactoring! That's great! 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? Thanks, Lars > --- > 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 >
