Re: [PATCH v3 10/10] convert: add filter.<driver>.process option

On 04 Aug 2016, at 12:18, Jakub Narębski [off-list ref] wrote:

...

quoted

quoted

quoted

+
+	sigchain_push(SIGPIPE, SIG_IGN);

Hmmm... ignoring SIGPIPE was good for one-shot filters.  Is it still
O.K. for per-command persistent ones?

Very good question. You are right... we don't want to ignore any errors
during the protocol... I will remove it.

I was actually just wondering.

Actually the default behavior if SIGPIPE is not ignored (or if the
SIGPIPE signal is not blocked / masked out) is to *terminate* the
writing program, which we do not want.

The correct solution is to check for error during write, and check
if errno is set to EPIPE.  This means that reader (filter driver
process) has closed pipe, usually due to crash, and we need to handle
that sanely, either restarting or quitting while providing sane
information about error to the user.

Well, we might want to set a signal handler for SIGPIPE, not just
simply ignore it (especially for streaming case; stop streaming
if filter driver crashed); though signal handlers are quite limited
about what might be done in them.  But that's for the future.


Read from closed pipe returns EOF; write to closed pipe results in
SIGPIPE and returns -1 (setting errno to EPIPE).

OK, I think I understand. I will address that in the next round.

quoted

quoted

...
Or maybe extract writing the header for a file into a separate function?
This one gets a bit long...

Maybe... but I think that would make it harder to understand the protocol. I
think I would prefer to have all the communication in one function layer.

I don't understand your reasoning here ("make it harder to understand the
protocol").  If you choose good names for function writing header, then
the main function would be the high-level view of protocol, e.g.

  git> <command>
  git> <header>
  git> <contents>
  git> <flush>

  git< <command accepted>
  git< <contents>
  git< <flush>
  git< <sent status>

OK, I will move the header into a separate function.

quoted

quoted

quoted

...
+		cat ../test.o >test.r &&

Err, the above is just copying file, isn't it?
Maybe it was copied from other tests, I have not checked.

It was created in the "setup" test.

What I meant here (among other things) is that you uselessly use
'cat' to copy files:

   +		cp ../test.o test.r &&

Ah right. No idea why I did that. I'll use cp, of course :-)

quoted

quoted

quoted

+		echo "test22" >test2.r &&
+		mkdir testsubdir &&
+		echo "test333" >testsubdir/test3.r &&

All right, we test text file, we test binary file (I assume), we test
file in a subdirectory.  What about testing empty file?  Or large file
which would not fit in the stdin/stdout buffer (as EXPENSIVE test)?

No binary file. The main reason for this test is to check multiple files.
I'll add a empty file. A large file is tested in the next test.

I assume that this large file is binary file; what matters is that it
includes NUL character ("\0"), i.e. zero byte, checking that there is
no error that would terminate it at NUL.

Good idea! I will add a small test file with \0 bytes in between to test binaries.


Thanks,
Lars