Re: [PATCH v5 14/15] convert: add filter.<driver>.process option
- From
Lars Schneider <larsxschneider@gmail.com>
- Date
- Aug 12, 2016, 16:59 UTC
- Message-ID
- <509A907F-B1B5-4244-B1C7-F1190296208D@gmail.com>
- In-Reply-To
- <CAGZ79kboxgBRHSa2s7CKZ1Uo=13WT=rT8VHCNJNj_Q9jQzZAYw@mail.gmail.com>
Show 39 quoted lines
> On 12 Aug 2016, at 18:33, Stefan Beller <sbeller@google.com> wrote: > > On Wed, Aug 10, 2016 at 6:04 AM, <larsxschneider@gmail.com> wrote: >> From: Lars Schneider <larsxschneider@gmail.com> >> >> Git's clean/smudge mechanism invokes an external filter process for every >> single blob that is affected by a filter. If Git filters a lot of blobs >> then the startup time of the external filter processes can become a >> significant part of the overall Git execution time. >> >> In a preliminary performance test this developer used a clean/smudge filter >> written in golang to filter 12,000 files. This process took 364s with the >> existing filter mechanism and 5s with the new mechanism. See details here: >> https://github.com/github/git-lfs/pull/1382 >> >> This patch adds the `filter.<driver>.process` string option which, if used, >> keeps the external filter process running and processes all blobs with >> the packet format (pkt-line) based protocol over standard input and standard >> output described below. >> >> Git starts the filter when it encounters the first file >> that needs to be cleaned or smudged. After the filter started >> Git sends a welcome message, a list of supported protocol >> version numbers, and a flush packet. Git expects to read the >> welcome message and one protocol version number from the >> previously sent list. Afterwards Git sends a list of supported >> capabilities and a flush packet. Git expects to read a list of >> desired capabilities, which must be a subset of the supported >> capabilities list, and a flush packet as response: >> ------------------------ >> packet: git> git-filter-client >> packet: git> version=2 >> packet: git> version=42 >> packet: git> 0000 >> packet: git< git-filter-server >> packet: git< version=2 > > what follows is specific to version=2? > version 42 may deem capabilities a bad idea?
"version=42" is just an example to show how the initialization could look like in a distant future when we support even another protocol version.
You are correct, what follows is specific to version=2. I will state that more clearly in the documentation.
Can you try to rephrase "version 42 may deem capabilities a bad idea?" I am not sure I understand what you mean.
Show 13 quoted lines
> >> packet: git> clean=true >> packet: git> smudge=true >> packet: git> not-yet-invented=true >> packet: git> 0000 >> packet: git< clean=true >> packet: git< smudge=true >> packet: git< 0000 >> ------------------------ >> Supported filter capabilities in version 2 are "clean" and >> "smudge". > > I assume version 2 is an example here and we actually start with v1?
No, it is actually called version 2 because I consider the current clean/smudge protocol version 1.
> Can you clarify why we need welcome messages? > (Is there a technical reason, or better debuggability for humans?)
The welcome message is necessary to distinguish the long running filter protocol (v2) from the current one-shot filter protocol (v1). This is becomes important if a users tries to use a v1 clean/smudge filter with the v2 git config settings.
Show 42 quoted lines
>> Afterwards Git sends a list of "key=value" pairs terminated with >> a flush packet. The list will contain at least the filter command >> (based on the supported capabilities) and the pathname of the file >> to filter relative to the repository root. Right after these packets >> Git sends the content split in zero or more pkt-line packets and a >> flush packet to terminate content. >> ------------------------ >> packet: git> command=smudge\n >> packet: git> pathname=path/testfile.dat\n >> packet: git> 0000 >> packet: git> CONTENT >> packet: git> 0000 >> ------------------------ >> >> The filter is expected to respond with a list of "key=value" pairs >> terminated with a flush packet. If the filter does not experience >> problems then the list must contain a "success" status. Right after >> these packets the filter is expected to send the content in zero >> or more pkt-line packets and a flush packet at the end. Finally, a >> second list of "key=value" pairs terminated with a flush packet >> is expected. The filter can change the status in the second list. >> ------------------------ >> packet: git< status=success\n >> packet: git< 0000 >> packet: git< SMUDGED_CONTENT >> packet: git< 0000 >> packet: git< 0000 # empty list! >> ------------------------ >> >> If the result content is empty then the filter is expected to respond >> with a success status and an empty list. >> ------------------------ >> packet: git< status=success\n >> packet: git< 0000 >> packet: git< 0000 # empty content! >> packet: git< 0000 # empty list! >> ------------------------ > > Why do we need the last flush packet? We'd expect as many successes > as we send out contents? Do we plan on interleaving operation, i.e. > Git sends out 10 files but the filter process is not as fast as Git sending > out and the answers trickle in slowly?
Git filter processes run sequentially right now (unfortunately).
re flush: please see Peff's answer: http://public-inbox.org/git/20160812163809.3wdkuqegxfjam2yn%40sigill.intra.peff.net/
Show 29 quoted lines
>> In case the filter cannot or does not want to process the content, >> it is expected to respond with an "error" status. Depending on the >> `filter.<driver>.required` flag Git will interpret that as error >> but it will not stop or restart the filter process. >> ------------------------ >> packet: git< status=error\n >> packet: git< 0000 >> ------------------------ >> >> In case the filter cannot or does not want to process the content >> as well as any future content for the lifetime of the Git process, >> it is expected to respond with an "error-all" status. Depending on >> the `filter.<driver>.required` flag Git will interpret that as error >> but it will not stop or restart the filter process. >> ------------------------ >> packet: git< status=error-all\n >> packet: git< 0000 >> ------------------------ >> >> If the filter experiences an error during processing, then it can >> send the status "error". Depending on the `filter.<driver>.required` >> flag Git will interpret that as error but it will not stop or restart >> the filter process. >> ------------------------ >> packet: git< status=success\n > > So the first success is meaningless essentially? > Would it make sense to move the sucess behind the content sending > in all cases?
Again, I refer to Peff's answer.
Show 21 quoted lines
>> packet: git< 0000 >> packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT >> packet: git< 0000 >> packet: git< status=error\n >> packet: git< 0000 >> ------------------------ >> >> If the filter dies during the communication or does not adhere to >> the protocol then Git will stop the filter process and restart it >> with the next file that needs to be processed. >> >> After the filter has processed a blob it is expected to wait for >> the next "key=value" list containing a command. When the Git process >> terminates, it will send a kill signal to the filter in that stage. >> >> If a `filter.<driver>.clean` or `filter.<driver>.smudge` command >> is configured then these commands always take precedence over >> a configured `filter.<driver>.process` command. > > okay. I think you can omit most of the commit message as it is a duplicate > of the documentation?
Yes it duplicates the documentation.
> Instead the commit message can answer questions that are not part of > the documentation. (See the questions above which can be summarized > as "Why do we do it this way and not differently?")
OK, point taken. I will write a new commit message for v6.
Show 8 quoted lines
>
>> + if (err || errno == EPIPE) {
>> + if (!strcmp(filter_status.buf, "error")) {
>> + /*
>> + * The filter signaled a problem with the file.
>> + */
>
> /* This could go into a single line comment. */OK, will change.
Show 16 quoted lines
>> + } else if (!strcmp(filter_status.buf, "error-all")) {
>> + /*
>> + * The filter signaled a permanent problem. Don't try to filter
>> + * files with the same command for the lifetime of the current
>> + * Git process.
>> + */
>> + entry->supported_capabilities &= ~wanted_capability;
>> + } else {
>> + /*
>> + * Something went wrong with the protocol filter.
>> + * Force shutdown and restart if another blob requires filtering!
>> + */
>> + error("external filter '%s' failed", cmd);
>
> failed .. Can you give more information to the user such that they can easier
> debug? (blob/path or state / expected state)Agreed, will add! However, we don't give this information with the current clean/smudge interface.
Show 13 quoted lines
>> +
>> static int read_convert_config(const char *var, const char *value, void *cb)
>> {
>> const char *key, *name;
>> @@ -526,6 +818,10 @@ static int read_convert_config(const char *var, const char *value, void *cb)
>> if (!strcmp("clean", key))
>> return git_config_string(&drv->clean, var, value);
>>
>> + if (!strcmp("process", key)) {
>> + return git_config_string(&drv->process, var, value);
>> + }
>
> optional nit: braces unnecessaryAgreed, will remove!
Thanks a lot for the review, Lars