You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by makl <ma...@tigris.org> on 2004/04/07 05:28:04 UTC
Re: [PATCH] Fix for issue 1797 - take three
[[[
Fix for issue 1797. non-recursive delete of directory tree fails
Patch from <ma...@tigris.org>
* subversion/include/svn_wc.h
Add new function svn_wc_adm_extend.
* subversion/libsvn_wc/lock.c
(do_open): Allow locking of directories that are already locked.
(svn_wc_adm_open2):
(svn_wc__adm_pre_open):
Add the new parameters to the call of do_open.
(svn_wc_adm_extend): New function. This function is similar to
svn_wc_adm_open2 but doesn't return a SVN_ERR_WC_LOCKED error if
the directory has already a lock in associated.
* subversion/libsvn_client/commit.c
(svn_client_commit): Always lock deleted directories recursive.
* subversion/libsvn_client/commit_util.c
(svn_client__do_commit): Remove any deleted entries where the parent
is deleted too.
* subversion/tests/clients/cmdline/commit_tests.py
(commit_deleted_directory_tree_non_recursive_1): New
regression test for case one.
(commit_deleted_directory_tree_non_recursive_2_1): New
regression test for case two with one deleted directory.
(commit_deleted_directory_tree_non_recursive_2_1): New
regression test for case two with three independent deleted
directories.
]]]
Re: [PATCH] Fix for issue 1797 - take four
Posted by makl <ma...@tigris.org>.
[[[
Fix for issue 1797. non-recursive delete of directory tree fails
Patch from <ma...@tigris.org>
* subversion/include/svn_wc.h
Add new function svn_wc_adm_extend.
* subversion/libsvn_wc/lock.c
(do_open): Allow locking of directories that are already locked.
(svn_wc_adm_open2):
(svn_wc__adm_pre_open):
Add the new parameters to the call of do_open.
(svn_wc_adm_extend): New function. This function is similar to
svn_wc_adm_open2 but doesn't return a SVN_ERR_WC_LOCKED error if
the directory has already a lock in associated.
* subversion/libsvn_client/commit.c
(svn_client_commit): Always lock deleted directories recursive.
* subversion/libsvn_client/commit_util.c
(svn_client__do_commit): Remove any deleted entries where the parent
is deleted too.
* subversion/tests/clients/cmdline/commit_tests.py
(commit_deleted_directory_tree_non_recursive_1): New
regression test for case one.
(commit_deleted_directory_tree_non_recursive_2_1): New
regression test for case two with one deleted directory.
(commit_deleted_directory_tree_non_recursive_2_1): New
regression test for case two with three independent deleted
directories.
]]]
Re: [PATCH] Fix for issue 1797 - take three
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
>>>>@@ -329,22 +329,29 @@
>>>> static svn_error_t *
>>>> do_open (svn_wc_adm_access_t **adm_access,
>>>> svn_wc_adm_access_t *associated,
>>>>+ const svn_wc_adm_access_t *root_associated,
>>> I don't see why you added this additional access baton parameter, it
>>> looks like you could just use associated.
>>
>> Look at the recursion. In the next subdirectory, associated has an empty
>> set.
>
> Could the recursive call pass associated rather than the new lock?
No, probably not. It looks like we need two access batons :-(
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 1797 - take three
Posted by Philip Martin <ph...@codematters.co.uk>.
makl <ma...@tigris.org> writes:
> Philip Martin wrote:
>
>> makl <ma...@tigris.org> writes:
>>
>>>Index: subversion/libsvn_wc/lock.c
>>>===================================================================
>>>--- subversion/libsvn_wc/lock.c (revision 9290)
>>>+++ subversion/libsvn_wc/lock.c (working copy)
>>>@@ -329,22 +329,29 @@
>>> static svn_error_t *
>>> do_open (svn_wc_adm_access_t **adm_access,
>>> svn_wc_adm_access_t *associated,
>>>+ const svn_wc_adm_access_t *root_associated,
>> I don't see why you added this additional access baton parameter, it
>> looks like you could just use associated.
>
> Look at the recursion. In the next subdirectory, associated has an empty
> set.
Could the recursive call pass associated rather than the new lock?
>>>+
>>>+ if (lock == &missing)
>>>+ lock = NULL;
>> I think we need to verify the type of the lock, if write_lock is set
>> it's probably not acceptable to use a read-only lock.
>
> missing is a static entry declared in line 80. It has no meaningful
> information, so there is nothing to check.
My comment wasn't directed at missing in particular, but at the fact
that there was no check for the type.
>> Hmm, now I see you are upgrading read-only locks into write locks, but
>> you didn't document this behaviour anywhere. As far as I can tell it
>> even upgrades existing read-only locks! That won't work, not unless
>> the entries cache is thrown away and repopulated. I don't like it,
>> I'd prefer locks not to change type. That's the reason
>> adm_access_alloc takes a lock type; once allocated it never changes.
>
> Can we accept write locks if read locks are requested?
I don't know. If the calling code is making assumptions about the
behaviour of the entries cache (assuming it won't change because it's
read-only) then it's probably safer to return an error.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 1797 - take three
Posted by makl <ma...@tigris.org>.
Philip Martin wrote:
> makl <ma...@tigris.org> writes:
>
>
>>Index: subversion/libsvn_wc/lock.c
>>===================================================================
>>--- subversion/libsvn_wc/lock.c (revision 9290)
>>+++ subversion/libsvn_wc/lock.c (working copy)
>>@@ -329,22 +329,29 @@
>> static svn_error_t *
>> do_open (svn_wc_adm_access_t **adm_access,
>> svn_wc_adm_access_t *associated,
>>+ const svn_wc_adm_access_t *root_associated,
>
>
> I don't see why you added this additional access baton parameter, it
> looks like you could just use associated.
Look at the recursion. In the next subdirectory, associated has an empty
set.
>>+
>>+ if (lock == &missing)
>>+ lock = NULL;
>
>
> I think we need to verify the type of the lock, if write_lock is set
> it's probably not acceptable to use a read-only lock.
missing is a static entry declared in line 80. It has no meaningful
information, so there is nothing to check.
> Hmm, now I see you are upgrading read-only locks into write locks, but
> you didn't document this behaviour anywhere. As far as I can tell it
> even upgrades existing read-only locks! That won't work, not unless
> the entries cache is thrown away and repopulated. I don't like it,
> I'd prefer locks not to change type. That's the reason
> adm_access_alloc takes a lock type; once allocated it never changes.
Can we accept write locks if read locks are requested?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix for issue 1797 - take three
Posted by Philip Martin <ph...@codematters.co.uk>.
makl <ma...@tigris.org> writes:
> Index: subversion/libsvn_wc/lock.c
> ===================================================================
> --- subversion/libsvn_wc/lock.c (revision 9290)
> +++ subversion/libsvn_wc/lock.c (working copy)
> @@ -329,22 +329,29 @@
> static svn_error_t *
> do_open (svn_wc_adm_access_t **adm_access,
> svn_wc_adm_access_t *associated,
> + const svn_wc_adm_access_t *root_associated,
I don't see why you added this additional access baton parameter, it
looks like you could just use associated.
> const char *path,
> svn_boolean_t write_lock,
> int depth,
> svn_boolean_t under_construction,
> + svn_boolean_t allow_existing_locks,
This parameter I understand, although neither parameter is documented.
> apr_pool_t *pool)
> {
> - svn_wc_adm_access_t *lock;
> + svn_wc_adm_access_t *lock = NULL;
> int wc_format;
> svn_error_t *err;
> + svn_boolean_t is_new_lock = FALSE;
>
> if (associated)
> {
> adm_ensure_set (associated);
>
> lock = apr_hash_get (associated->set, path, APR_HASH_KEY_STRING);
> - if (lock && lock != &missing)
> +
> + if (! lock && root_associated && allow_existing_locks)
> + lock = apr_hash_get (root_associated->set, path, APR_HASH_KEY_STRING);
> +
> + if (lock && lock != &missing && ! allow_existing_locks)
> /* Already locked. The reason we don't return the existing baton
> here is that the user is supposed to know whether a directory is
> locked: if it's not locked call svn_wc_adm_open, if it is locked
That comment is now a bit odd.
> @@ -352,9 +359,12 @@
> return svn_error_createf (SVN_ERR_WC_LOCKED, NULL,
> "Working copy '%s' locked",
> path);
> +
> + if (lock == &missing)
> + lock = NULL;
I think we need to verify the type of the lock, if write_lock is set
it's probably not acceptable to use a read-only lock.
> }
>
> - if (! under_construction)
> + if (! under_construction && ! lock)
> {
> /* By reading the format file we check both that PATH is a directory
> and that it is a working copy. */
> @@ -377,19 +387,21 @@
> pool));
> }
>
> + if (! lock)
> + {
> + lock = adm_access_alloc (svn_wc__adm_access_unlocked, path, pool);
Why unlocked? Is it possible to get here when write_lock is set?
> + is_new_lock = TRUE;
> + }
> +
> /* Need to create a new lock */
> - if (write_lock)
> + if (write_lock && lock->type != svn_wc__adm_access_write_lock)
> {
> - lock = adm_access_alloc (svn_wc__adm_access_write_lock, path, pool);
> + lock->type = svn_wc__adm_access_write_lock;
Hmm, now I see you are upgrading read-only locks into write locks, but
you didn't document this behaviour anywhere. As far as I can tell it
even upgrades existing read-only locks! That won't work, not unless
the entries cache is thrown away and repopulated. I don't like it,
I'd prefer locks not to change type. That's the reason
adm_access_alloc takes a lock type; once allocated it never changes.
> SVN_ERR (create_lock (lock, 0, pool));
> lock->lock_exists = TRUE;
> }
> - else
> - {
> - lock = adm_access_alloc (svn_wc__adm_access_unlocked, path, pool);
> - }
>
> - if (! under_construction)
> + if (! under_construction && is_new_lock)
> {
> lock->wc_format = wc_format;
> if (write_lock)
> @@ -433,8 +445,9 @@
> entry_path = svn_path_join (lock->path, entry->name, subpool);
>
> /* Don't use the subpool pool here, the lock needs to persist */
> - err = do_open (&entry_access, lock, entry_path, write_lock, depth,
> - FALSE, lock->pool);
> + err = do_open (&entry_access, lock, root_associated, entry_path,
> + write_lock, depth,
> + FALSE, allow_existing_locks, lock->pool);
> if (err)
> {
> if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
> @@ -494,8 +507,9 @@
> when the cleanup handler runs. If the apr_xlate_t cleanup handler
> were to run *before* the access baton cleanup handler, then the access
> baton's handler won't work. */
> - apr_pool_cleanup_register (lock->pool, lock, pool_cleanup,
> - pool_cleanup_child);
> + if (is_new_lock)
> + apr_pool_cleanup_register (lock->pool, lock, pool_cleanup,
> + pool_cleanup_child);
> *adm_access = lock;
> return SVN_NO_ERROR;
> }
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org