I'm growing weary of this. Daniel and I spent a couple hours discussing calling patterns, error states, and everything. This felt the best approach.
If you have a concrete suggestion, and a patch, then I'll take a look. -g On Fri, Apr 27, 2012 at 16:02, Philip Martin <philip.mar...@wandisco.com> wrote: > Greg Stein <gst...@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
