Julian Foad <julianfoad_at_btopenworld.com> writes:
> Philip Martin wrote:
>> Julian Foad writes:
>>>> URL: http://svn.apache.org/r1577280
>>>> * subversion/include/svn_fs.h
>>>> (svn_fs_lock_target_t, svn_fs_lock_result_t,
>>>> svn_fs_lock2, svn_fs_unlock2): new.
>>>>
>>>> * subversion/include/svn_repos.h
>>>> (svn_repos_fs_lock2, svn_repos_fs_unlock2): new.
>>>
>>> Do we intend to deprecate the old versions? If not, then it would be
>>> better to name the new functions something like ...lock_many() instead
>>> of ...lock2().
>>
>> I'm unsure. In some ways it is convenient to have a single path
>> function as it avoids the need to construct hashes and the error
>> handling is simpler and less prone to leak. On the other hand when
>> svn_client_copy4 introduced multi-path copy source the single path
>> version was deprecated.
>
> Thoughts...
>
> A low level API such as libsvn_fs should aim to be clean and
> efficient, more so than to provide functions that are convenient for
> the casual user. (A high level API such as libsvn_client or the
> bindings is a more appropriate place to provide convenience
> functions.) We do have a number of APIs already, in several libraries,
> where the caller is expected to pass in a list of targets even if they
> only want to operate on one. Maybe in general the thing to aim for is
> to make it easy for a caller to do so.
>
> We could provide a singleton constructor for the
> hash-of-svn_fs_lock_target_t argument. Should we? Probably not at the
> libsvn_fs level; maybe at a high level.
I'l look at doing that.
> I started reading further through the new code. Here are some more
> review comments.
>
> 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.
> 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.
> We should provide a destructor for the hash-of-svn_fs_lock_result_t
> results, which contains errors that need to be cleared. svnserve's
> lock_many() already contains such code twice. It's simple code --
> around 5 lines -- but if we're demanding that callers ensure they do
> it, it's nice to provide a ready-made way for them to do it. This
> would also help in future-proofing against us revising the API in
> later releases.
I have a look at that.
> 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.
>
That's an error in libsvn_fs, libsvn_repos does it right. Fixed in
r1582845.
> + /* [JAF] Is that a question to reviewers? It depends. What errors can
> + svn_fs_lock2 return, and do any of them justify not running the
> + post-lock hook? Can it even return an error and also create
> + locks? Its doc string needs to say. */
> if (err)
> return svn_error_trace(err);
It's almost always possible for fs functions to return arbitrary errors
due to filesystem permissions, lack of disk space etc. This could
happen part way through creating locks. I'm not sure whether we should
attempt to run the post-hook.
> 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.
> 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.
> svn_hash_sets(targets, full_path, target);
> }
>
> @@ -2767,6 +2770,11 @@ static svn_error_t *lock_many(svn_ra_svn
>
> /* From here on we need to make sure any errors in authz_results, or
> results, are cleared before returning from this function. */
> +
> + /* Check authz access for each target, because ...
> + ### Why? We don't want svn_repos_fs_lock2() to do this for us ...?
> + Or, we want to log such errors and it's easier to do so before
> + than afterwards? */
That's the way servers/repos functions work.
> for (hi = apr_hash_first(pool, targets); hi; hi = apr_hash_next(hi))
> {
> const char *full_path = svn__apr_hash_index_key(hi);
> @@ -2791,7 +2799,8 @@ static svn_error_t *lock_many(svn_ra_svn
> 0, /* No expiration time. */
> steal_lock, pool, subpool);
>
> - /* The client expects results in the same order as paths were supplied. */
> + /* Send the results. The client expects results in the same order as
> + paths were supplied. */
> for (i = 0; i < path_revs->nelts; ++i)
> {
> const char *path, *full_path;
> @@ -2816,6 +2825,16 @@ static svn_error_t *lock_many(svn_ra_svn
> result = svn_hash_gets(authz_results, full_path);
> if (!result)
> /* No result? Should we return some sort of placeholder error? */
> + /* ### [JAF] Is that a question to reviewers? Certainly something
> + has gone wrong. Maybe svn_repos_fs_lock2 returned an error
> + overall, having processed none or only some paths -- is it
> + allowed to do so?
It could run out of disk space half way through.
> Breaking here would signal to the client that
> + something went wrong, because they'll see a too-short response
> + list, but would also potentially hide information about further
> + targets that were processed, some of which perhaps were locked
> + successfully. (Note: we're scanning them here in a different
> + order from the order in which svn_repos_fs_lock2() processed
> + them.) */
> break;
>
> if (result->err)
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Received on 2014-03-28 20:53:37 CET