Hi Philip. Thanks for the responses.
(I see you've changed the API implementation to a callback pattern now.)
Philip Martin wrote:
> Julian Foad <julianfoad_at_btopenworld.com> writes:
>> The 'comment' parameter to svn_fs_lock2 should be a member of the
>> 'target' struct, since it is inherently a per-target attribute. Of
>> course there are common scenarios where a client wants to lock a bunch
>> of paths all with the same comment, but the benefits of a lock-many
>> API should also be made available to clients which supply different
>> comments. This applies all the way up the call stack. However, I note
>> that from the RA layer upwards we have had lock-many APIs since v1.2
>> which take a single comment for all the paths, so changing to
>> per-target comments throughout the stack is perhaps beyond the scope
>> of this issue. We could still do it at the FS and repos layers now in
>> preparation; I don't see that it would add significant overhead in
>> run-time or in maintenance.
>
> I think per-lock comments in a multi-path lock are unneccesary. I don't
> think anybody locking dozens/hundreds of files will bother defining
> different comments. Anyone who wants two or three locks with distinct
> comments can call the function multiple times.
For a high level client API, I agree with you. But the same argument doesn't make sense for the low level APIs. Here, it's not a matter of what *people* are likely to do, but another layer of *software*.
Imagine the caller is an svn GUI that has an offline mode in which the user can queue up lock requests, and the software will send them to the server when it's back on line. Does the GUI's back end need to run through the queued lock requests and re-group them into batches such that all the lock requests in a batch have the same comment?
Or imagine we want to teach 'svnsync' to synchronize locks: it reads all the locks in the source repo and writes them to the target repo. Do we want to force this software to re-group the locks into batches or write them one at a time? Wouldn't it be better if it can just say "Here is a bunch of locks; please write them"?
The principle that makes sense to me is, if the semantics of the data objects being written is one comment per lock, then the API should match that, and not restrict the caller unnecessarily.
>> In principle, the 'is_dav_comment' and 'expiration_date' parameters
>> should similarly be per target, 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 and keeping
>> the original single-lock API (not deprecated) to cater for such
>> locks. RA-local and svnserve and 'svnadmin lock' don't support
>> expiration and always pass is_dav_comment=FALSE. Only mod_dav_svn
>> supplies these two options, and it currently only uses the old
>> one-lock-at-a-time API anyway.
>
> Eventually we will define a new request and mod_dav_svn will start
> calling the multiple path version. It will still handle the current
> LOCK request but may use the multi-path function to do it.
Eventually we will define a new request and mod_dav_svn MAY start calling the multiple path version of the API, but it will never need to set multiple locks at once, because mod_dav_svn talks to DAV via the dav_hooks_locks vtable which only deals with one path at a time. Look at the calls to svn_repos_fs_lock() in mod_dav_svn/lock.c. (A slight confusion is that the vtable methods support multiple locks per path; but that's irrelevant as our implementation doesn't.)
So it could still just as efficiently use the old single-path version of the API, if we support that, couldn't it?
[...]
>> Index: subversion/svnserve/serve.c
>> ===================================================================
>> --- subversion/svnserve/serve.c (revision 1582225)
>> +++ subversion/svnserve/serve.c (working copy)
>> @@ -2733,7 +2733,8 @@ static svn_error_t *lock_many(svn_ra_svn
>> an error. */
>> SVN_ERR(must_have_access(conn, pool, b, svn_authz_write, NULL, TRUE));
>>
>> - /* Loop through the lock requests. */
>> + /* Loop through the lock requests
>> + ### and do what? */
>
> I just copied that comment from the old code. I probably should have
> removed it.
Well, I find it very useful to have a comment at the top of a non-trivial block of code summarizing what it's supposed to be doing. How about
/* Parse PATH_REVS into TARGETS. */
here?
>> for (i = 0; i < path_revs->nelts; ++i)
>> {
>> const char *path, *full_path;
>> @@ -2759,6 +2760,8 @@ static svn_error_t *lock_many(svn_ra_svn
>> target->current_rev = current_rev;
>>
>> /* We could check for duplicate paths and reject the request? */
>> + /* ### [JAF] Is that a question to reviewers? We could, but I
>> + don't think it's useful to do so. */
>
> I brought up the handling of canonical paths on the dev list, it wasn't
> conclusive from my point of view so I just implemented something. Given
> that FS generally accepts non-canonical paths it is hard to know what to
> do.
Agreed. Let's just update the comment here to say we decided not to?
Please could you mark comments with '###' or similar when they are questions to reviewers about the code or design being unfinished or questionable, etc.
- Julian
Received on 2014-04-03 16:04:29 CEST