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
