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

On 27 Sep 2016, at 00:41, Jakub Narębski [off-list ref] wrote:

Part first of the review of 11/11.

W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:

quoted

From: Lars Schneider <redacted>

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.

quoted

+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"

quoted

+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.

quoted

+
+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.

quoted

+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 ;-)

quoted

+------------------------
+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. "

quoted

+
+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."

quoted

+
+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.

quoted

+
+
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

quoted

+        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

quoted

+    }
+    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

quoted

+            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

quoted

+        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

quoted

+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.

quoted

+        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.

quoted

+    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?

quoted

+( 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.

quoted

+
+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.

quoted

+    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