Re: [PATCH v8 11/11] convert: add filter.<driver>.process option

[Lars Schneider <larsxschneider@gmail.com>]

> On 27 Sep 2016, at 00:41, Jakub Narębski <jnareb@gmail.com> wrote:
> 
> Part first of the review of 11/11.
> 
> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> index 7aff940..946dcad 100644
>> --- a/Documentation/gitattributes.txt
>> +++ b/Documentation/gitattributes.txt
>> @@ -293,7 +293,13 @@ checkout, when the `smudge` command is specified, the command is
>> fed the blob object from its standard input, and its standard
>> output is used to update the worktree file.  Similarly, the
>> `clean` command is used to convert the contents of worktree file
>> -upon checkin.
>> +upon checkin. By default these commands process only a single
>> +blob and terminate.  If a long running `process` filter is used
>   ^^^^
> 
> Should we use this terminology here?  I have not read the preceding
> part of documentation, so I don't know if it talks about "blobs" or
> if it uses "files" and/or "file contents".

I used that because it was used in the paragraph above already.


>> +Long Running Filter Process
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +If the filter command (a string value) is defined via
>> +`filter.<driver>.process` then Git can process all blobs with a
>> +single filter invocation for the entire life of a single Git
>> +command. This is achieved by using a packet format (pkt-line,
>> +see technical/protocol-common.txt) based protocol over standard
>> +input and standard output as follows. All packets are considered
>> +text and therefore are terminated by an LF. Exceptions are the
>> +"*CONTENT" packets and the flush packet.
> 
> I guess that reasoning here is that all but CONTENT packets are
> metadata, and thus to aid debuggability of the protocol are "text",
> as considered by pkt-line.
> 
> Perhaps a bit more readable would be the following (but current is
> just fine; I am nitpicking):
> 
>  All packets, except for the "{star}CONTENT" packets and the "0000"
>  flush packer, are considered text and therefore are terminated by
>  a LF.

OK, I use that!


> I think it might be a good idea to describe what flush packet is
> somewhere in this document; on the other hand referring (especially
> if hyperlinked) to pkt-line technical documentation might be good
> enough / better.  I'm unsure, but I tend on the side that referring
> to technical documentation is better.

I have this line in the first paragraph of the Long Running Filter process:
"packet format (pkt-line, see technical/protocol-common.txt) based protocol"

> 
>> +to read a welcome response message ("git-filter-server") and exactly
>> +one protocol version number from the previously sent list. All further
> 
> I guess that is to provide forward-compatibility, isn't it?  Also,
> "Git expects..." probably means filter process MUST send, in the
> RFC2119 (https://tools.ietf.org/html/rfc2119) meaning.

True. I feel "expects" reads better but I am happy to change it if
you feel strong about it.


>> +
>> +After the version negotiation Git sends a list of supported capabilities
>> +and a flush packet.
> 
> Is it that Git SHOULD send list of ALL supported capabilities, or is
> it that Git SHOULD NOT send capabilities it does not support, and that
> it MAY send only those capabilities it needs (so for example if command
> uses only `smudge`, it may not send `clean`, so that filter driver doesn't
> need to initialize data it would not need).

"After the version negotiation Git sends a list of all capabilities that
it supports and a flush packet."

Better?


> I wonder why it is "<capability>=true", and not "capability=<capability>".
> Is there a case where we would want to send "<capability>=false".  Or
> is it to allow configurable / value based capabilities?  Isn't it going
> a bit too far: is there even a hind of an idea for parametrize-able
> capability? YAGNI is a thing...

Peff suggested that format and I think it is OK:
http://public-inbox.org/git/20160803224619.bwtbvmslhuicx2qi@sigill.intra.peff.net/


> A few new capabilities that we might want to support in the near future
> is "size", "stream", which are options describing how to communicate,
> and "cleanFromFile", "smudgeToFile", which are new types of operations...
> but neither needs any parameter.
> 
> I guess that adding new capabilities doesn't require having to come up
> with the new version of the protocol, isn't it.

Correct.


>> +packet:          git< git-filter-server
>> +packet:          git< version=2
>> +packet:          git> clean=true
>> +packet:          git> smudge=true
>> +packet:          git> not-yet-invented=true
> 
> Hmmm... should we hint at the use of kebab-case versus snake_case
> or camelCase for new capabilities?

I personally prefer kebab-case but I think that is a discussion for
future contributions ;-)


>> +------------------------
>> +packet:          git> command=smudge
>> +packet:          git> pathname=path/testfile.dat
>> +packet:          git> 0000
>> +packet:          git> CONTENT
>> +packet:          git> 0000
>> +------------------------
> 
> I think it is important to mention that (at least with current
> `filter.<driver>.process` implementation, that is absent future
> "stream" capability / option) the filter process needs to read
> *whole contents* at once, *before* writing anything.  Otherwise
> it can lead to deadlock.
> 
> This is especially important in that it is different (!) from the
> current behavior of `clean` and `smudge` filters, which can
> stream their response because Git invokes them async.

I added this:
" Please note, that the filter
must not send any response before it received the content and the
final flush packet. "


>> +
>> +If the filter experiences an error during processing, then it can
>> +send the status "error" after the content was (partially or
>> +completely) sent. 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
>> +packet:          git< 0000
>> +packet:          git< HALF_WRITTEN_ERRONEOUS_CONTENT
>> +packet:          git< 0000
>> +packet:          git< status=error
>> +packet:          git< 0000
>> +------------------------
> 
> Good.  A question is if the filter process can send "status=abort"
> after partial contents, or does it need to wait for the next command?

I added:
"expected to respond with an "abort" status at any point in
the protocol."


>> +
>> +After the filter has processed a blob it is expected to wait for
>> +the next "key=value" list containing a command. Git will close
>> +the command pipe on exit. The filter is expected to detect EOF
>> +and exit gracefully on its own.
> 
> Good to have it documented.  
> 
> Anyway, as it is Git command that spawns the filter driver process,
> assuming that the filter process doesn't daemonize itself, wouldn't
> the operating system reap it after its parent process, that is the
> git command it invoked, dies? So detecting EOF is good, but not
> strictly necessary for simple filter that do not need to free
> its resources, or can leave freeing resources to the operating
> system? But I may be wrong here.

The filter process runs independent of Git.


>> +
>> +
>> Interaction between checkin/checkout attributes
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> 
>> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
>> new file mode 100755
>> index 0000000..c13a631
>> --- /dev/null
>> +++ b/contrib/long-running-filter/example.pl
> 
> To repeat myself, I think it would serve better as a separate patch.

OK


>> +        die "invalid packet size '$bytes_read' field";
> 
> This would read "invalid packet size '000' field", for example.
> Perhaps the following would be (slightly) better:
> 
>  +        die "invalid packet size field: '$bytes_read'";

OK


>> +    }
>> +    elsif ( $pkt_size > 4 ) {
> 
> Isn't a packet of $pkt_size == 4 a valid packet, a keep-alive
> one?  Or is it forbidden?

"Implementations SHOULD NOT send an empty pkt-line ("0004")."
Source: Documentation/technical/protocol-common.txt


>> +            die "invalid packet ($content_size expected; $bytes_read read)";
> 
> This error message would read "invalid packet (12 expected; 10 read)";
> I think it would be better to rephrase it as
> 
>  +            die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";

OK


>> +        die "invalid packet size";
> 
> I'm not sure if it is worth it (especially for the demo script),
> but perhaps we could show what this invalid size was?
> 
>  +        die "invalid packet size value '$pkt_size'";

OK


>> +sub packet_txt_read {
>> +    my ( $res, $buf ) = packet_bin_read();
>> +    unless ( $buf =~ /\n$/ ) {
> 
> Wouldn't
> 
>  +    unless ( $buf =~ s/\n$// ) {
> 
> or (less so)
> 
>  +    unless ( $buf =~ s/\n$\z// ) {
> 
> be more idiomatic (and not require use of 'substr')?  Remember,
> the s/// substitution quote-like operator returns number of
> substitutions in the scalar context.

OK.


>> +        die "A non-binary line SHOULD BE terminated by an LF.";
> 
> This is SHOULD be, not MUST be, so perhaps 'warn' would be enough.
> Not that Git should send us such line.

Actually it MUST per protocol definition. I'll change it to MUST.


>> +    my ($packet) = @_;
> 
> This is equivalent to
> 
>  +    my $packet = shift;
> 
> which, I think, is more common for single-parameter subroutines.
> 
> Also, this is $data (or $buf), not $packet.

OK


> Perhaps some comment that main begins here?
> 
>> +( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
>> +( packet_txt_read() eq ( 0, "version=2" ) )         || die "bad version";
>> +( packet_bin_read() eq ( 1, "" ) )                  || die "bad version end";
> 
> Actually, it is overly strict.  It should not fail if there
> are other "version=3", "version=4" etc. lines.

True, but I think for an example this is OK. I'll add a note
to the file header.


>> +
>> +while (1) {
>> +    my ($command)  = packet_txt_read() =~ /^command=([^=]+)$/;
>> +    my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
> 
> Do we require this order?  If it is, is that explained in the
> documentation?

Git sends that order right now but the filter should not rely
on that order.


>> +    packet_flush();    # empty list!
> 
> This is less "empty list!", and more keeping "status=success" unchanged.

OK


OK means, I agree and I added your suggestion to v9.
Thanks a lot for your review and the comments!

Cheers,
Lars