On Wed, Mar 30, 2011 at 11:45:01AM +0200, Daniel Shahaf wrote:
> [email protected] wrote on Tue, Mar 29, 2011 at 14:46:39 -0000:
> > @@ -338,7 +338,19 @@ svn_cmdline_fputs(const char *string, FI
> > if (fputs(out, stream) == EOF)
> > {
> > if (errno)
> > - return svn_error_wrap_apr(errno, _("Write error"));
> > + {
> > + err = svn_error_wrap_apr(errno, _("Write error"));
> > +
> > + /* ### Issue #3014: Return a specific error for broken pipes,
> > + * ### with a single element in the error chain. */
> > + if (APR_STATUS_IS_EPIPE(err->apr_err))
> > + {
> > + svn_error_clear(err);
>
> So you're doing:
>
> * get error number
> * construct an svn_error_t object (that's includes a few malloc()s)
> * check error number
> * destroy svn_error_t object
>
> Won't it be simpler to avoid constructing the svn_error_t object in the
> first place (when errno is EPIPE)?
Yes, that's what I had initially.
Something like: if (errno == EPIPE) ...
But I wasn't sure if that would need #ifdef on windows.
And I didn't want to break the windows bots.
So I decided to let APR figure out how to represent EPIPE.
> > @@ -376,7 +400,15 @@ svn_cmdline_handle_exit_error(svn_error_
> > apr_pool_t *pool,
> > const char *prefix)
> > {
> > - svn_handle_error2(err, stderr, FALSE, prefix);
> > + /* Issue #3014:
> > + * Don't print anything on broken pipes. The pipe was likely
> > + * closed by the process at the other end. We expect that
> > + * process to perform error reporting as necessary.
> > + *
> > + * ### This assumes that there is only one error in a chain for
> > + * ### SVN_ERR_IO_PIPE_WRITE_ERROR. See svn_cmdline_fputs(). */
> > + if (err->apr_err != SVN_ERR_IO_PIPE_WRITE_ERROR)
> > + svn_handle_error2(err, stderr, FALSE, prefix);
>
> This ### seems easy enough to avoid, isn't it? i.e., why did you need
> to assume a single-link chain?
Well, if the pipe write error is somewhere within the chain, we'd
have to iterate the entire chain. Is there an API for that?
I don't want to add a loop...
> > @@ -383,7 +383,15 @@ svn_opt_print_generic_help2(const char *
> > return;
> >
> > print_error:
> > - svn_handle_error2(err, stderr, FALSE, "svn: ");
> > + /* Issue #3014:
> > + * Don't print anything on broken pipes. The pipe was likely
> > + * closed by the process at the other end. We expect that
> > + * process to perform error reporting as necessary.
> > + *
> > + * ### This assumes that there is only one error in a chain for
> > + * ### SVN_ERR_IO_PIPE_WRITE_ERROR. See svn_cmdline_fputs(). */
> > + if (err->apr_err != SVN_ERR_IO_PIPE_WRITE_ERROR)
> > + svn_handle_error2(err, stderr, FALSE, "svn: ");
>
> This bit of code (including the comment, the ###, and the specific
> condition) is repeated N times below.
>
> Abstract it into a private helper macro?
Good idea.
