Re: [PATCH] Fix for issue 1797 - take three

makl <makl@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