Stefan Sperling wrote on Fri, Sep 16, 2016 at 09:09:08 +0200:
> On Fri, Sep 16, 2016 at 03:42:58AM +0000, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Fri, Sep 09, 2016 at 06:44:51 +0000:
> > > 3. Add to svn_commit_info_t an svn_error_t representation of the error
> > > chain that svn_commit_callback_t::post_commit_err summarizes,
> > Patch attached.
> > The new test fails over ra_svn; I haven't investigated that yet.
> > Does the approach look sane? This has more moving parts than a normal
> > add-member-to-struct patch because the new member is an svn_error_t so
> > it needs explicit clearing.
> > Thanks,
> > Daniel
> In your original post you stated:
> "The problem here is that the "post-commit hook failed" error isn't printed."
> If all you want to do is print a message, why not just pass a string around?
So it has the correct apr_status_t error code(s).
> Passing around (and having to marshal) a whole error chain seems like a
> rather complicated perfectionist approach for the problem at hand, don't
> you think? End-users won't care about the chain, they just care about the
> message. And developers might as well use a debugger to examine a post-commit
> error chain if needed.
Can we agree that whatever developers do is of secondary concern?
I think end-users do care about the error code. We print it out in the
command-line tools too and one of the reasons for that is so scripts can
Moreover, as per the TODO's in the patch, this is groundwork for passing
the error (code|chain) over the wire, which would allow libsvn_ra/libsvn_client
consumers to distinguish "post-commit hook error" from "post-commit FS
processing error" through means more robust than strstr(). (We already
have code to marshal svn_error_t over ra_svn.)
What concerns me at the moment is that the patch makes deltify_etc()
return an error in situations where previously it did not. I think
there are three possible solutions to this: revv the API deltify_etc()
implements; make the change in 1.10 as an API erratum; or move the new
logic from deltify_etc() to the libsvn_client consumers (svn and
svnmucc). Haven't yet chosen among them.
In the meantime, I've fixed the test and removed the #ifdef from the
header file (the new member is now declared as a const pointer).
Received on 2016-09-17 08:53:48 CEST