Greg Stein <gstein_at_gmail.com> writes:
> And that pattern was very annoying. Today, you can
> SVN_ERR(svn_fs_editor_commit(...)). With the pattern you're
> suggesting, it gets uglier:
>
> err = svn_fs_editor_commit(...);
> if (SVN_IS_VALID_REVNUM(revision))
> ... do some stuff
> else if (err == SVN_ERR_FS_CONFLICT)
> ... do some other stuff
> else if (post_commit_err)
> ... yet more stuff
I don't think you are comparing like with like there! Your first
example is:
SVN_ERR(svn_fs_editor_commit(...))
if (confict)
conflict occurred
else
if (post_commit_err)
post commit occurred
commit occurred
The second example is
err = svn_fs_editor_commit():
if (err == CONFLICT)
conflict occurred
else if (err) # extra
return err # extra
else
if (post_commit_err)
post commit occurred
commit occurred
Some difference, but not that much.
> And meanwhile, you're trying to manage whether an error was returned,
> which needs to be cleared, wrapped, returned, etc. And throw in there
> some transaction aborting, too.
>
> Take a look at libsvn_repos/commit.c:complete_cb(). The error handling
> logic is quite sane. Compare that to any caller of svn_fs_commit_txn()
> (a few) and the more-used svn_repos_fs_commit_txn(). They follow
> roughly similar patterns, but not all the same. Some drop the error
> anyways. All are more complicated.
>
> I chose a calling pattern that seems rational (looking at
> complete_cb), and takes into account the *current* behavior of FS and
> the low-quality conflict errors that it returns. If/when the FS wants
> to return something more, then great. My advice would be: don't make
> it an error. Return some kind of rich data structure that describes
> the conflict. Leave human-readable message generation to an exterior
> function which consumes that structure. That allows
> SVN_ERR(svn_fs_commit_txn()), relegating returned errors to true
> errors instead of semantic issues within the txn that are
> best-described with a mechanism other than svn_error_t. That is the
> pattern/design of svn_fs_editor_commit(), though the "structure" is
> just a simple path at the moment.
You could say the same about all our error reporting. Why is this one
special?
At the momement if an FS backend has some new conflict information it
can be added by the FS code that generate SVN_ERR_FS_CONFLICT. If I
tweak the error in FSFS and commit I get:
svn mkdir -mm http://localhost:8888/obj/repo/A
svn: Conflict at path '/A' due to some special condition
With your scheme we need to change the FS/editor API to get the
information out of the FS into the server, then have the server marshal
it across the wire (extend the protocol? pack it in an svn_error_t?)
then have the client construct an error for the user.
Maybe it's academic and the FS errors are never going to change, but I
don't see why this error is being handled differently.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com
Received on 2012-04-27 22:02:37 CEST