On Fri, Apr 27, 2012 at 13:29, Philip Martin <philip.martin_at_wandisco.com> wrote:
> Greg Stein <gstein_at_gmail.com> writes:
>> On Fri, Apr 27, 2012 at 11:20, Philip Martin <philip.martin_at_wandisco.com> wrote:
>>> Greg Stein <gstein_at_gmail.com> writes:
>>>
>>>> On Wed, Apr 25, 2012 at 16:43, Philip Martin <philip.martin_at_wandisco.com> wrote:
>>>>> Perhaps we should keep the error and toss the path?
>>>>
>>>> mod_dav_svn uses that path to construct a "nice" error message based
>>>> on the context of the commit.
>>>
>>> mod_dav_svn isn't the only user of this API.
>>
>> WELL aware of that fact. I was simply giving a data point for how the
>> path was used.
>>
>> What's your point?
>
> I don't understand why the fact that mod_dav_svn uses the path is a
> reason to discard the error. Particularly when svnserve currently
> discards the path and returns the error to the client.
The current err->message says "Conflict at '/some/path'." (no real context)
mod_dav_svn returns a message such as:
"A conflict occurred during the CHECKIN processing. The problem
occurred with the \"%s\" resource."
or
"Conflict when committing '%s'."
or
"A conflict occurred during the MERGE processing. The problem occurred
with the \"%s\" resource."
To construct each of these, it needs the path. And I argue the above
messages are more understandable.
Now: if we returned a chain, then we could have that simplistic
message, then wrapped with something like "Conflict occurred during
the CHECKIN processing", and omit the path. But, alas, we don't have
chain reporting (at this time).
>>>>> So currently reporting "conflict on path 'foo'" is probably sufficient
>>>>> but it's odd to limit ourselves. We have an error reporting mechanism
>>>>> why would we choose not to use it?
>>>>
>>>> Well... we return that conflict_path so that callers can make
>>>> nicer/contextual error messages with the path. We don't transfer the
>>>> whole chain over the wire, so if the path is on the inner error (say,
>>>> if we just wrapped context and relied on the path to be in the inner
>>>> error), then the user would never see the path.
>>>>
>>>> If we could/would marshal the whole error chain, then yes: we could
>>>> rely on our error reporting mechanism. But we don't, so we can't.
>>>
>>> svnserve returns an error chain, see svn_ra_svn_write_cmd_failure.
>>
>> And other RA layers do not.
>
> You seem to be suggesting that because mod_dav_svn doesn't return the
> error chain the error should not be available to other RA layers.
If the error had something interesting in it, then I might have
designed something to incorporate that error and produce an error
chain. And then maybe figured out some way to marshal that back to the
client, in a backwards/forwards compatible fashion. But that's a lot
of work when the returned errors have nothing of consequence anyways.
> What happens if an FS layer wants to return more information about a
> conflict? Does it return SVN_ERR_FS_CONFLICT_RICH to bypass the
> discarding of SVN_ERR_FS_CONFLICT?
When it does, then we can revisit the problem.
> Perhaps we should keep the current behaviour and return both an error
> and a path and let the caller decide which to one use.
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
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.
Cheers,
-g
Received on 2012-04-27 20:38:34 CEST