Looking for help to understand external filter driver code

Hi,

a long time ago in aa4ed4 Junio introduced the external filter driver definition. Since that time we fork the Git process and then we fork again to run the external filter. This is probably a super stupid question with an obvious answer... but can anyone help me to understand the code and explain why we fork twice? Wouldn't it be sufficient to just fork once for the external filter since we cannot process anything in parallel anyways?

In 546bb5 Hannes refactored Junio's code to use the async infrastructure that is used today.

Thank you, Lars

Lars Schneider <larsxschneider@gmail.com> writes:

In "git show aa4ed4" you find this picture:

       /*
        * Create a pipeline to have the command filter the buffer's
        * contents.
        *
        * (child --> cmd) --> us
        */

where "child" is a fork of the original Git process; it still has the <src, size> buffered data inherited from the original process, so it can write_in_full() into the pipe going to the cmd process. "us" is the original process that has the reading end of the pipe that connects us to the "(child --> cmd)" processes, so that it can read the filtered result from them.

The key benefit of this arrangement is the above can be done without having to do poll() to flip between reading and writing that is needed to avoid deadlocking, which kept the code simpler. A later conversion of the write side into async does not fundamentally change anything from the original arrangement.

Junio C Hamano <gitster@pobox.com> writes:

Translation: I was too lazy to worry about doing poll()/select()
when I did it originally.  As long as you can do so correctly, be my
guest to reduce one process by having the main process do both
reading and writing.

;-)

On 19 Jul 2016, at 20:53, Junio C Hamano <gitster@pobox.com> wrote:

Thanks a lot for this explanation. My goal is to add an option to the clean/smudge filter config that instructs Git to keep the external filter process running. Whenever Git encounters a file to be filtered then it would continue to talk to the filter process over a simple pipe protocol:

Git writes --> 4 byte filename length Git writes --> filename string Git writes --> 4 byte content length Git writes --> content string Git reads <-- 4 byte filtered content length Git reads <-- filtered content

I still need to read more about poll()/select() but it looks to me as if these functions are only required if Git doesn't know what to expect. With the sketched protocol above that wouldn't be the case. Therefore, I wonder if I would need to use poll()/select()?

Thanks, Lars

Lars Schneider <larsxschneider@gmail.com> writes:

Why limit to 32GB? Perhaps NUL termination is more appropriate here?

Do you really need to force the sender to know the length in advance? Together with the sequential nature of the above exchange, i.e. the filter is forbidden from producing even a single byte of its output before reading everything Git feeds it, you are making it impossible to use filters that perform streaming conversion.

Of course, with the "sequential" thing, you do not have to worry about deadlocking hence no need for poll/select, but I am not sure that is a good thing.

On 19 Jul 2016, at 23:33, Junio C Hamano <gitster@pobox.com> wrote:

OK, I will use NUL termination for the filename. You're also right about the limit - I will use 8 byte to encode the content length.

That is correct. However, for my particular use case streaming conversion wouldn't be useful anyways: https://github.com/github/git-lfs/pull/1382

Thanks for the confirmation. I consider to exchange a "filter protocol version" right after the filter process has started. That way someone could add a more evolved "filter driver protocol" later on that supports streaming and the external filter could pick whatever protocol is most appropriate (and supported). Could that be an acceptable compromise to get a serious review of the "sequential" thing?

Thanks, Lars

On 07/20/2016 12:01 AM, Lars Schneider wrote:

Is there any reason to encode the file length in binary format? With all the discussions about big endianess, little endianess, 4GiB or 32 GiB. How about simply writing the length as ASCII ?

Unless we don't want to have a "spare" field for future extensions, it could be good to add an option field, which may be empty. On top of that, do we want a field separator different from the line separator ?

How about this: <options><TAB><length><TAB><filename><NUL>

<options> may be "var=value;var2=value2" or simply ""

W dniu 2016-07-19 o 23:33, Junio C Hamano pisze:

Errr, I think limiting _filename_ to 32GB is a reasonable
assumption, for forever...
 

...while limiting file _content_ to 32GB might not be future-proof enough.

;-)

Well, Java packages can get reaaaally long :D

I think this is what Junio meant to say. At least I interpreted it that way.

- Lars

On Tue, Jul 19, 2016 at 02:33:09PM -0700, Junio C Hamano wrote:

Another option: use pkt-lines with a flush packet to indicate end-of-input. That allows arbitrary sized data, with streaming, and reuses existing concepts from git. There is proportional overhead, but it's only 4 bytes per 64k, which is a tiny percent.

It does make some implementations easier if they know the size ahead of time, though, so if we are _sure_ that nobody will want streaming later, it may not be a good tradeoff. If we do print a size ahead of time, the "normal" thing in git would be to do so in base-10 ascii followed by a newline (e.g., as found in "cat-file --batch", or fast-import's "data" command).

-Peff