Stefan Sperling wrote on Wed, Mar 30, 2011 at 15:06:35 +0200:
> On Wed, Mar 30, 2011 at 01:35:42PM +0200, Daniel Shahaf wrote:
> > > > 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.
> > >
> >
> > Given the way your patch passes errno to apr_status_t-expecting
> > functions, I'd say that APR_STATUS_IS_EPIPE(errno) should work.
>
> Fixed in r1086936.
>
Thanks.
> > > 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?
> >
> > Actually there is (svn_error_has_cause()), but what I had in mind while
> > writing my reply was "Hey, isn't it sufficient to require that
> > SVN_ERR_IO_PIPE_WRITE_ERROR be the outer-most code?"
>
> Well, that's what it's requiring. Hence the comments.
>
The comments require a chain that contains only one error, that's not
the same thing as requiring a chain whose outer-most error has error
code N.
> > > > > @@ -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.
>
> Though as I said on IRC, I can't come up with a good way of doing this.
>
> Do you want a macro like this? I think that's ugly.
>
> SVN_ERR_CALL_UNLESS(err->apr_err, SVN_ERR_IO_PIPE_WRITE_ERROR,
> svn_handle_error2(err, stderr, FALSE, "svn: "));
>
> #define SVN_ERR_CALL_UNLESS(error_code1, error_code2, expr) \
> if ((error_code1) != (error_code2) (expr)
>
> Something that hard-codes error_code2 to EPIPE would also be ugly.
>
How about the following, then? At least, it does avoid having
hard-coding any errors at the caller sites.
/** "%s\n\n%s\n" % (docstring, "see issue %d" % IforgotwhatNis) */
#define SVN_CMDLINE_IGNOREABLE_EXIT_ERROR(err) \
((err) == SVN_NO_ERROR \
|| (err)->apr_err == SVN_ERR_IO_PIPE_WRITE_ERROR \
|| (err)->apr_err == SVN_ERR_WHATEVER)
...
if (! SVN_CMDLINE_IGNOREABLE_EXIT_ERROR(err)):
svn_handle_error2(err);
Make sense?
> Note that this macro would need to be shared between several commands,
> not just 'svn'.
Received on 2011-03-30 20:36:17 CEST