Re: Review of lock-many API [was: svn commit: r1577280 [1/3] ...]

I (Julian Foad) wrote:
> In principle, the 'is_dav_comment' and 'expiration_date'
> parameters should similarly be per target,

Not "should", but rather "could be argued, at some pedantic level".

> but that makes less sense in practice
> as they're only used for generic DAV clients via mod_dav_svn. As an
> alternative, we might consider dropping them from this API [...]

[...]
> The semantics relating to returning one error per path and an overall error
> needs to be fully documented and/or changed. For example, svn_repos_fs_lock()
> assumes that svn_repos_fs_lock2() has set *results to a valid hash if it returns
> any overall error, while svn_fs_lock() assumes that svn_fs_lock2() WON'T
> have put any error in *results if it returns an error overall. These look at
> least surprising.

In fact, I strongly feel we should not return error objects within the results structure. A per-target result is not, in some abstract sense, an error -- the caller didn't try to do something it is forbidden to try, and the server didn't encounter an unexpected condition or anything broken in its execution. A result such as "the lock you're trying to remove is not present" is merely one of the possible and expected outcomes.

The problem existed before this lock-many work. Wrapping the existing design in a multi-target wrapper just exacerbates the issue.

So, rather, we should return flags or other information that specifically states the result for that target. (If a higher level caller wants to consider it an error and construct an error object, that's it's business. For backwards compatibility that will obviously have to be done in some places.)

A different, practical objection to error objects constructed on the server is they are not localized to the client's human language.

- Julian
Received on 2014-03-27 14:52:28 CET