Some redundant tests in locking code?

(I brought this up in another thread but got no response.)

It looks to me like the code below is testing for several errors that either
can't occur or don't warrant the following action being taken. I can't see
that any harm will result, but if I'm right the code is wrong and confusing.

In subversion/libsvn_ra_dav/commit.c:794: commit_delete_entry():

> /* 404 is ignored, because mod_dav_svn is effectively merging
> against the HEAD revision on-the-fly. In such a universe, a
> failed deletion (because it's already missing) is OK; deletion
> is an idempotent merge operation. */
> serr = simple_request(parent->cc->ras, "DELETE", child, &code,
> extra_headers,
> 204 /* Created */, 404 /* Not Found */, pool);
>
> /* A locking-related error most likely means we were deleting a
> directory rather than a file, and didn't send all of the
> necessary lock-tokens within the directory. */
> if (serr && ((serr->apr_err == SVN_ERR_FS_BAD_LOCK_TOKEN)
> || (serr->apr_err == SVN_ERR_FS_NO_LOCK_TOKEN)
> || (serr->apr_err == SVN_ERR_FS_LOCK_OWNER_MISMATCH)
> || (serr->apr_err == SVN_ERR_FS_PATH_ALREADY_LOCKED)))
> {
> /* Re-attempt the DELETE request as if the path were a directory.
> Discover all lock-tokens within the directory, and send them in
> the body of the request (which is normally empty). Of course,
> if we don't *find* any additional lock-tokens, don't bother to
> retry (it ain't gonna do any good).

It is trying to delete a path, having assumed to begin with that the path is
just a file. If that fails, maybe the path was a directory and so more lock
tokens are needed. That "if" is trying to say, "if (an error occured that
might be resolved by sending all the lock tokens held under this directory)".
However, it doesn't appear to be checking the most appropriate set of error
codes. As far as I can tell, neither SVN_ERR_FS_BAD_LOCK_TOKEN nor
SVN_ERR_FS_LOCK_OWNER_MISMATCH could be resolved just by sending more tokens,
so there's no point in them being in the list. And
SVN_ERR_FS_PATH_ALREADY_LOCKED only applies to creating new locks, not using them.

Please could somebody tell me whether I'm understanding this correctly?

- Julian

-- 
http://www.foad.me.uk/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org