Thread (1 message) 1 message, 1 author, 2016-06-15

Re: [PATCH] Ignore SIGPIPE when running a filter driver

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:07

Jonathan Nieder [off-list ref] writes:
quoted
If a filter is not defined or if it fails, git behaves as if the filter
is a no-op passthru. However, if the filter exits before reading all
the content, and depending on the timing git, could be kill with
SIGPIPE instead.

Ignore SIGPIPE while processing the filter to detect when it exits
early and fallback to using the unfiltered content.

Signed-off-by: Jehan Bing <redacted>
For the benefit of the uninitiated ("how would ignoring an error help
me detect an error?"): setting the SIGPIPE handler to SIG_IGN does not
actually ignore the broken pipe condition but causes it to be reported
as an I/O error, errno == EPIPE.  That means instead of being killed
by SIGPIPE, git gets to fall back to passthrough and report the
filter's mistake.
Yes.  

You could rephrase  bit better to further clarify it, perhaps like this:

    Ignore SIGPIPE when running a filter driver
    
    If a filter is not defined or if it fails, git should behave as if the
    filter is a no-op passthru.
    
    However, if the filter exits before reading all the content, depending on
    the timing, git could be killed with SIGPIPE when it tries to write to the
    pipe connected to the filter.
    
    Ignore SIGPIPE while processing the filter to give us a chance to check
    the return value from a failed write, in order to detect and act on this
    mode of failure in a more controlled way.
    
    Signed-off-by: Jehan Bing [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

although I think Jehan's original was already clear enough.
So at least on POSIX-y platforms, this patch looks good to me.  Thanks
for writing it.
Thank you and Johannes for eyeballing and sanity checking.

Will queue.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help