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
parse it.
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).
WDYT?
Cheers,
Daniel
Received on 2016-09-17 08:53:48 CEST