Hi,
Jehan Bing wrote:
> 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 <jehan@xxxxxxx>
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:
error: cannot feed the input to external filter <foo>
error: external filter <foo> failed
[...]
> +++ b/convert.c
[...]
> @@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)
> if (start_command(&child_process))
> return error("cannot fork to run external filter %s", params->cmd);
>
> + sigchain_push(SIGPIPE, SIG_IGN);
Setting the signal disposition after launching the external filter
which would otherwise inherit it, so the filter does not have to cope
with unfamiliar SIGPIPE handling[*]. Phew.
> +
> write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
> if (close(child_process.in))
> write_err = 1;
> if (write_err)
> error("cannot feed the input to external filter %s", params->cmd);
>
> + sigchain_pop(SIGPIPE);
> +
This happens in an async procedure. SIGPIPE is ignored in the
following block in the other thread:
if (strbuf_read(&nbuf, async.out, len) < 0) {
error("read from external filter %s failed", cmd);
ret = 0;
}
if (close(async.out)) {
error("read from external filter %s failed", cmd);
ret = 0;
}
if (finish_async(&async)) {
That implies a tiny behavior change: if there is an I/O error reading
from async.out at the right moment and stderr is going to a closed
pipe, inability to report the error can result in the error flag being
set on stderr instead of the process being killed. I don't think
anyone will notice.
So at least on POSIX-y platforms, this patch looks good to me. Thanks
for writing it.
Sincerely,
Jonathan
[*] See http://bugs.python.org/issue1652 for some stories about what
we are narrowly escaping here. :)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html