Re: [PATCH] sub-process: refactor handshake to common function
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jul 24, 2017, 22:21 UTC
- Message-ID
- <20170724222139.GF13924@aiede.mtv.corp.google.com>
- In-Reply-To
- <20170724213810.29831-1-jonathantanmy@google.com>
Hi,
Jonathan Tan wrote:
Show 7 quoted lines
> 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> > ---
Sounds like a sensible thing to do.
[...]
Show 9 quoted lines
> --- 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.
[...]
Show 17 quoted lines
> @@ -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.
[...]
Show 8 quoted lines
> --- 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?
[...]
Show 8 quoted lines
> --- 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 ...]
Show 11 quoted lines
> - 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