You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2005/03/08 20:48:03 UTC
Re: svn commit: r13308 - branches/locking/subversion/libsvn_fs_base
cmpilato@tigris.org writes:
> Author: cmpilato
> Date: Tue Mar 8 14:00:34 2005
> New Revision: 13308
> --- branches/locking/subversion/libsvn_fs_base/tree.c (original)
> +++ branches/locking/subversion/libsvn_fs_base/tree.c Tue Mar 8 14:00:34 2005
> @@ -2541,6 +2541,7 @@
> struct commit_args *args = baton;
> apr_hash_t *changed_paths;
> apr_hash_index_t *hi;
> + apr_pool_t *subpool;
>
> svn_fs_txn_t *txn = args->txn;
> svn_fs_t *fs = txn->fs;
> @@ -2581,42 +2582,61 @@
> discovered locks. */
> SVN_ERR (svn_fs_bdb__changes_fetch (&changed_paths, trail->fs, txn_name,
> trail, trail->pool));
> + subpool = svn_pool_create (trail->pool);
> for (hi = apr_hash_first (trail->pool, changed_paths);
> hi; hi = apr_hash_next (hi))
> {
> const void *key;
> void *val;
> - const char *path;
> - dag_node_t *node;
> svn_fs_path_change_t *change;
> - svn_node_kind_t kind;
> -
> +
> + svn_pool_clear (subpool);
> +
> apr_hash_this (hi, &key, NULL, &val);
> - path = key;
> change = val;
>
> /* ### Ugh. Gotta figure out the node_kind of each changed-path.
> We REALLY REALLY need to put node-kind into the change skel!
> - Unfortunately, no schema changes allowed till svn 2.0. */
> - SVN_ERR (svn_fs_base__dag_get_node
> - (&node, trail->fs, change->node_rev_id, trail, trail->pool));
> - kind = svn_fs_base__dag_node_kind (node);
> -
> - /* always do a non-recursive lock check on a file. */
> - if (kind == svn_node_file)
> - SVN_ERR (svn_fs_base__allow_locked_operation (path, svn_node_file,
> - 0, trail, trail->pool));
> -
> - /* if a directory wasn't added or deleted, then it must be
> - listed because of a propchange only; do a non-recursive
> - lock-check. otherwise, added/deleted dirs must be checked
> - recursively. */
> + Unfortunately, no schema changes allowed till svn 2.0.
> + Worse, we currently have no reliable way to determine the
> + kind of a deleted path!!
> +
> + So, here's the plan. If a path is deleted, we'll look for
> + locks both as a file, and as a directory (recursively).
> + Otherwise, we'll do a more sane (single) check. */
> + if (change->change_kind != svn_fs_path_change_delete)
> + {
> + svn_node_kind_t kind;
> + svn_boolean_t recurse = TRUE;
> + dag_node_t *node;
> +
> + SVN_ERR (svn_fs_base__dag_get_node
> + (&node, trail->fs, change->node_rev_id, trail, subpool));
> + kind = svn_fs_base__dag_node_kind (node);
> +
> + /* If a directory wasn't added or deleted, then it must be
> + listed because of a propchange only; do a non-recursive
> + lock-check. And we always do non-recursive checks for
> + files. Note that if we weren't able to determine the
> + node kind, we have to use a recursive check. */
> + if ((kind == svn_node_file)
> + || ((kind == svn_node_dir)
> + && (change->change_kind == svn_fs_path_change_modify)))
> + recurse = FALSE;
This came up in a thread a while ago: I think you need to do a
recursive check whenever an item is added, even if the item is a file.
The reason is that it's possible to lock 'foo/bar' before 'foo'
exists, so knowing that 'foo' is a file tells you nothing about locks
under 'foo'.
The current code has this inconsistency:
Scenario 1
working copy A: create directory 'foo'
lock 'foo/bar'
working copy B: create directory 'foo'
commit 'foo' => fails because 'foo/bar' is locked
Scenario 2
working copy A: create directory 'foo'
lock 'foo/bar'
working copy B: create file 'foo'
commit 'foo' => succeeds
I don't think it's sensible for one of those commits to succeed and
one to fail.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13308 - branches/locking/subversion/libsvn_fs_base
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
> This came up in a thread a while ago: I think you need to do a
> recursive check whenever an item is added, even if the item is a file.
> The reason is that it's possible to lock 'foo/bar' before 'foo'
> exists, so knowing that 'foo' is a file tells you nothing about locks
> under 'foo'.
If the parent of the added item is itself added then there is probably
no need to do a recursive check even if the item is a directory, the
recursive check on the parent should suffice.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13308 - branches/locking/subversion/libsvn_fs_base
Posted by Ben Collins-Sussman <su...@collab.net>.
On Mar 8, 2005, at 3:28 PM, Philip Martin wrote:
>
> http://svn.haxx.se/dev/archive-2005-02/0070.shtml
>
> I was told that locks apply to pathnames and that locking schedule add
> files should be possible, so just add "create file 'foo/bar'" before
> the "lock 'foo/bar'" step.
>
Whoops, sorry for the confusion. I must have been speaking about
features I *wished* for, not things we've actually planned or
implemented. libsvn_fs certainly supports locks on nonexistent paths;
it's just a table of metadata that associates pathnames with svn_lock_t
structures.
But at the moment, we don't seem to be able to lock a schedule-add file:
[sussman@BenBook:~/scratch/wc]$ touch newfile
[sussman@BenBook:~/scratch/wc]$ svn add newfile
A newfile
[sussman@BenBook:~/scratch/wc]$ svn lock newfile -m "lock new file"
subversion/libsvn_ra_dav/util.c:365: (apr_err=160041)
svn: Path '/newfile' doesn't exist in HEAD revision.
Tracing through the code, I think I see the problem. The client is
passing a working-rev of 0 to the server, and libsvn_fs is using that
value to do an out-of-dateness check on the object! libsvn_fs tries to
look up the (0, path) and then get that node's created-rev. An invalid
revnum is returned, and then the error above is returned. (You can see
this code in the locking branch, libsvn_fs_base/lock.c,
txn_body_lock()).
I bet this is a simple bug to fix: if locking a schedule-add file,
just don't send the working-revnum.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13308 - branches/locking/subversion/libsvn_fs_base
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Collins-Sussman <su...@collab.net> writes:
> On Mar 8, 2005, at 2:48 PM, Philip Martin wrote:
>>
>> Scenario 1
>> working copy A: create directory 'foo'
>> lock 'foo/bar'
>> working copy B: create directory 'foo'
>> commit 'foo' => fails because 'foo/bar' is locked
>>
>> Scenario 2
>> working copy A: create directory 'foo'
>> lock 'foo/bar'
>> working copy B: create file 'foo'
>> commit 'foo' => succeeds
>>
>> I don't think it's sensible for one of those commits to succeed and
>> one to fail.
>>
>
> At the moment, the client has zero support for locking nonexistent
> paths; it doesn't even allow locking of URLs yet.
I only follow the locking stuff sporadically, so whenever I try to
reason about lock behaviour I run into problems with what is
possible/allowed :)
In the mail here:
http://svn.haxx.se/dev/archive-2005-02/0070.shtml
I was told that locks apply to pathnames and that locking schedule add
files should be possible, so just add "create file 'foo/bar'" before
the "lock 'foo/bar'" step.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r13308 - branches/locking/subversion/libsvn_fs_base
Posted by Ben Collins-Sussman <su...@collab.net>.
On Mar 8, 2005, at 2:48 PM, Philip Martin wrote:
>
> Scenario 1
> working copy A: create directory 'foo'
> lock 'foo/bar'
> working copy B: create directory 'foo'
> commit 'foo' => fails because 'foo/bar' is locked
>
> Scenario 2
> working copy A: create directory 'foo'
> lock 'foo/bar'
> working copy B: create file 'foo'
> commit 'foo' => succeeds
>
> I don't think it's sensible for one of those commits to succeed and
> one to fail.
>
At the moment, the client has zero support for locking nonexistent
paths; it doesn't even allow locking of URLs yet.
But your scenarios still make sense to me: replace your "working copy
A" text with "random DAV client", and they're quite possible.
So Mike, maybe we need to do both the non-recursive and recursive
checks for deleted *and* added objects.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org