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.martin_at_wandisco.com> wrote:
> 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 23:33:50 CEST