Hi,
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.
>
> Signed-off-by: Jonathan Tan <jonathanta...@google.com>
> ---
Sounds like a sensible thing to do.
[...]
> --- 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;
What does this flag represent? What values can it have? A comment
might help.
[...]
> @@ -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);
> +
Is there a more precise pointer within the gitattributes documentation
that describes what this does? I searched for "handshake" and found
nothing. Is the "Long Running Filter Process" section where I should
have started?
The API docs for this header are currently in
Documentation/technical/api-sub-process.txt. Perhaps these docs
should also go there, or, even better, the docs in
Documentation/technical/ could move to this header in a preparatory
patch.
Aside (not about this patch): why is the subprocess object called
struct subprocess_entry? Would it make sense to rename it to struct
subprocess?
Back to this function. Is this a function I should only call once,
when first launching a subprocess, or can I call it again later? A
note about suggested usage might help.
[...]
> --- 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
This error message looks a little less friendly than the old one.
Is that okay because it is not supposed to happen in practice?
[...]
> --- 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;
[... many lines snipped ...]
> - 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);
> }
API looks nice.
[...]
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -105,3 +105,97 @@ int subprocess_start(struct hashmap *hashmap, struct
> subprocess_entry *entry, co
Implementation looks sane from a quick glance.
Thanks,
Jonathan
