Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions

[Jeff King <peff@peff.net>]

On Fri, Apr 07, 2017 at 08:03:49AM -0400, Ben Peart wrote:

> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
>  done:
>  	sigchain_pop(SIGPIPE);
>  
> -	if (err || errno == EPIPE) {
> +	if (err || errno == EPIPE)
> +		err = err ? err : errno;
> +
> +	return err;
> +}

This isn't a new problem introduced by your patch, but this use of errno
seems funny to me. Specifically:

  1. Do we need to save errno before calling sigchain_pop()? It's making
     syscalls (though admittedly they are unlikely to fail).

  2. If err is 0, then nothing failed. Who would have set errno? Aren't
     we reading whatever cruft happened to be in errno before the
     function started?

     So I'm confused about what case would trigger on this errno check
     at all.

Also, I'm not quite sure what the return value of the function is
supposed to be; usually we'd use 0 for success and negative for errors.
But here we might return a negative value that we got from the packet_*
functions, or we might return EPIPE, which is positive. I don't think
it's a huge deal because the caller checks "if (err)", but perhaps it
should be "-errno" for consistency.

-Peff