Hello
This patch implements a parent-child lock relationship. It passes
the regression tests, including Justin's new one. I'm posting it to
ask if this is the right way to go. One thing that worries me is the
memory usage, it's probably trivial at the moment, but if we start to
cache the entries will it prove to be too expensive?
Since my visitors' baby has stopped crying I'm going to try to get
some sleep now :)
Continuing issue #749. Make svn_wc_adm_access_t optionally
hierarchical.
Also revert r2504 as it's no longer needed.
* subversion/include/svn_error_codes.h (SVN_ERR_WC_INVALID_LOCK): Added.
* subversion/include/svn_wc.h
(svn_wc_adm_open): Add parent parameter.
(svn_wc_adm_close): Document new behavior.
* subversion/libsvn_wc/adm_ops.c
(svn_wc_process_committed): Pass parent to svn_wc_adm_open. Allocate
child batons from parent's pool and don't explicitly close them.
(svn_wc_delete, svn_wc_revert, svn_wc_remove_from_revision_control):
Pass parent to svn_wc_adm_open.
* subversion/libsvn_wc/log.c (log_do_delete_entry): Pass parent to
svn_wc_adm_open.
* subversion/libsvn_wc/props.c (svn_wc_merge_prop_diffs): Pass NULL
parent to svn_wc_adm_open.
* subversion/libsvn_wc/adm_files.c (init_adm): Pass NULL parent to
svn_wc_adm_open.
* subversion/libsvn_client/commit.c
(svn_client_commit): Revert r2504. Return an error if the hash lookup
for an access baton fails.
* subversion/libsvn_client/commit_util.c
(lock_dir): Pass parent to svn_wc_adm_open.
* subversion/libsvn_wc/update_editor.c (delete_entry, close_directory,
svn_wc_install_file): Pass NULL parent to svn_wc_adm_open.
* subversion/libsvn_client/externals.c (relegate_external,
handle_external_item_change): Pass NULL parent to svn_wc_adm_open.
* subversion/libsvn_wc/wc.h
(enum svn_wc__adm_access_type): Renamed from enum svn_wc_adm_access_type.
(svn_wc__adm_access_unlocked, svn_wc__adm_access_read_lock,
svn_wc__adm_access_write_lock): Renamed enum identifiers.
(struct svn_wc_adm_access_t): Added parent and children members.
* subversion/libsvn_wc/lock.c
(svn_wc__adm_access_alloc): New function.
(svn_wc__adm_steal_write_lock): Use svn_wc__adm_access_alloc.
(svn_wc_adm_open): Add parent parameter and processing. Use
svn_wc__adm_access_alloc. For read locks check path is a directory.
(svn_wc_adm_close): Close children.
(svn_wc_adm_write_check): enum svn_wc__adm_access_type name change.
* subversion/tests/clients/cmdline/copy_tests.py
(copy_modify_commit): New test.
Index: ./subversion/include/svn_error_codes.h
===================================================================
--- ./subversion/include/svn_error_codes.h
+++ ./subversion/include/svn_error_codes.h Sun Jul 14 00:32:26 2002
@@ -200,6 +200,9 @@
SVN_ERRDEF (SVN_ERR_WC_NOT_LOCKED,
"Working copy not locked")
+ SVN_ERRDEF (SVN_ERR_WC_INVALID_LOCK,
+ "Invalid lock")
+
SVN_ERRDEF (SVN_ERR_WC_NOT_DIRECTORY,
"Path is not a working copy directory")
Index: ./subversion/include/svn_wc.h
===================================================================
--- ./subversion/include/svn_wc.h
+++ ./subversion/include/svn_wc.h Sun Jul 14 00:32:26 2002
@@ -59,14 +59,34 @@
/* Return an access baton in ADM_ACCESS for the working copy administrative
area associated with the directory PATH. If WRITE_LOCK is set the baton
will include a write lock, otherwise the baton can only be used for read
- access. POOL will be used to allocate the baton and any subsequently
- cached items. */
+ access.
+
+ If PARENT_ACCESS is an open access baton PATH must refer to a
+ subdirectory of the PARENT_ACCESS directory, and ADM_ACCESS will be a
+ child of PARENT_ACCESS. svn_wc_adm_close can be used to close
+ ADM_ACCESS. If ADM_ACCESS has not been closed when PARENT_ACCESS is
+ closed, it wil be closed automatically at that stage.
+
+ If PARENT_ACCESS is an open access baton, and PATH refers to a
+ subdirectory that has already been opened as a child of PARENT_ACCESS
+ and not yet explicitly closed, then the baton previously opened will be
+ returned.
+
+ PARENT_ACCESS can be NULL, in which case ADM_ACCESS is not a child of
+ any other baton and must be explicitly closed.
+
+ POOL will be used to allocate memory for the baton and any subsequently
+ cached items. If ADM_ACCESS is to be closed automatically by closing
+ PARENT_ACCESS then POOL must have a sufficient lifetime.*/
svn_error_t *svn_wc_adm_open (svn_wc_adm_access_t **adm_access,
+ svn_wc_adm_access_t *parent_access,
const char *path,
svn_boolean_t write_lock,
apr_pool_t *pool);
-/* Give up the access baton ADM_ACCESS, and its lock if any */
+/* Give up the access baton ADM_ACCESS, and its lock if any. It will also
+ close any child batons, for which this is a parent baton, that have not
+ yet been explicitly closed. */
svn_error_t *svn_wc_adm_close (svn_wc_adm_access_t *adm_access);
/* Ensure ADM_ACCESS has a write lock, and that it still exists. Returns
Index: ./subversion/libsvn_wc/props.c
===================================================================
--- ./subversion/libsvn_wc/props.c
+++ ./subversion/libsvn_wc/props.c Sun Jul 14 00:32:26 2002
@@ -393,7 +393,7 @@
return SVN_NO_ERROR; /* ### svn_node_none or svn_node_unknown */
}
- SVN_ERR (svn_wc_adm_open (&adm_access, parent, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, parent, TRUE, pool));
SVN_ERR (svn_wc__open_adm_file (&log_fp, parent, SVN_WC__ADM_LOG,
(APR_WRITE | APR_CREATE), /* not excl */
pool));
Index: ./subversion/libsvn_wc/wc.h
===================================================================
--- ./subversion/libsvn_wc/wc.h
+++ ./subversion/libsvn_wc/wc.h Sun Jul 14 00:32:26 2002
@@ -142,11 +142,11 @@
/* PATH to directory which contains the administrative area */
const char *path;
- enum svn_wc_adm_access_type {
+ enum svn_wc__adm_access_type {
/* SVN_WC_ADM_ACCESS_UNLOCKED indicates no lock is held allowing
read-only access without cacheing. */
- svn_wc_adm_access_unlocked,
+ svn_wc__adm_access_unlocked,
#if 0 /* How cacheing might work one day */
@@ -160,12 +160,12 @@
/* SVN_WC_ADM_ACCESS_READ_LOCK indicates that read-only access and
cacheing are allowed. */
- svn_wc_adm_access_read_lock,
+ svn_wc__adm_access_read_lock,
#endif
/* SVN_WC_ADM_ACCESS_WRITE_LOCK indicates that read-write access and
cacheing are allowed. */
- svn_wc_adm_access_write_lock
+ svn_wc__adm_access_write_lock
} type;
@@ -183,6 +183,13 @@
ENTRIES will be NULL. */
apr_hash_t *entries;
#endif
+
+ /* PARENT access baton, may be NULL. */
+ svn_wc_adm_access_t *parent;
+
+ /* CHILDREN is a hash of svn_wc_adm_access_t* keyed on char*
+ representing the path to sub-directories that are also locked. */
+ apr_hash_t *children;
/* POOL is used to allocate cached items, they need to persist for the
lifetime of this access baton */
Index: ./subversion/libsvn_wc/log.c
===================================================================
--- ./subversion/libsvn_wc/log.c
+++ ./subversion/libsvn_wc/log.c Sun Jul 14 00:32:26 2002
@@ -601,7 +601,8 @@
if (entry->kind == svn_node_dir)
{
svn_wc_adm_access_t *adm_access;
- SVN_ERR (svn_wc_adm_open (&adm_access, full_path, TRUE, loggy->pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, loggy->adm_access, full_path, TRUE,
+ loggy->pool));
err = svn_wc_remove_from_revision_control (adm_access,
SVN_WC_ENTRY_THIS_DIR,
TRUE, loggy->pool);
@@ -795,7 +796,8 @@
{
pdir = svn_path_join (loggy->adm_access->path, key, pool);
base_name = SVN_WC_ENTRY_THIS_DIR;
- SVN_ERR (svn_wc_adm_open (&entry_access, pdir, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&entry_access, loggy->adm_access, pdir,
+ TRUE, pool));
}
if (base_name)
Index: ./subversion/libsvn_wc/adm_ops.c
===================================================================
--- ./subversion/libsvn_wc/adm_ops.c
+++ ./subversion/libsvn_wc/adm_ops.c Sun Jul 14 00:57:16 2002
@@ -354,8 +354,8 @@
this_path = svn_path_join (path, name, subpool);
if (current_entry->kind == svn_node_dir)
- SVN_ERR (svn_wc_adm_open (&child_access, this_path, TRUE,
- subpool));
+ SVN_ERR (svn_wc_adm_open (&child_access, adm_access, this_path,
+ TRUE, subpool));
else
child_access = adm_access;
@@ -366,9 +366,6 @@
(current_entry->kind == svn_node_dir) ? TRUE : FALSE,
new_revnum, rev_date, rev_author, subpool));
- if (current_entry->kind == svn_node_dir)
- SVN_ERR (svn_wc_adm_close (child_access));
-
svn_pool_clear (subpool);
}
@@ -630,7 +627,7 @@
### function, at which stage this open will fail and can be
### removed. */
svn_wc_adm_access_t *adm_access;
- SVN_ERR (svn_wc_adm_open (&adm_access, path, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, path, TRUE, pool));
SVN_ERR (svn_wc_remove_from_revision_control
(adm_access, SVN_WC_ENTRY_THIS_DIR, FALSE, pool));
@@ -1201,7 +1198,7 @@
if (entry->kind == svn_node_file)
{
was_deleted = entry->deleted;
- SVN_ERR (svn_wc_adm_open (&adm_access, parent, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, parent, TRUE, pool));
}
else if (entry->kind == svn_node_dir)
{
@@ -1211,7 +1208,7 @@
parents_entry = apr_hash_get (entries, basey, APR_HASH_KEY_STRING);
if (parents_entry)
was_deleted = parents_entry->deleted;
- SVN_ERR (svn_wc_adm_open (&adm_access, path, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, path, TRUE, pool));
}
/* Remove the item from revision control. */
@@ -1507,8 +1504,8 @@
const char *entrypath = svn_path_join (adm_access->path,
current_entry_name,
subpool);
- SVN_ERR (svn_wc_adm_open (&entry_access, entrypath, TRUE,
- subpool));
+ SVN_ERR (svn_wc_adm_open (&entry_access, adm_access, entrypath,
+ TRUE, adm_access->pool));
err = svn_wc_remove_from_revision_control (entry_access,
SVN_WC_ENTRY_THIS_DIR,
destroy_wf, subpool);
@@ -1519,7 +1516,6 @@
}
else if (err)
return err;
- SVN_ERR (svn_wc_adm_close (entry_access));
}
}
Index: ./subversion/libsvn_wc/adm_files.c
===================================================================
--- ./subversion/libsvn_wc/adm_files.c
+++ ./subversion/libsvn_wc/adm_files.c Sun Jul 14 00:32:26 2002
@@ -1107,7 +1107,7 @@
/* Lock it immediately. Theoretically, no compliant wc library
would ever consider this an adm area until a README file were
present... but locking it is still appropriately paranoid. */
- SVN_ERR (svn_wc_adm_open (&adm_access, path, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, path, TRUE, pool));
/** Make subdirectories. ***/
Index: ./subversion/libsvn_wc/update_editor.c
===================================================================
--- ./subversion/libsvn_wc/update_editor.c
+++ ./subversion/libsvn_wc/update_editor.c Sun Jul 14 00:32:26 2002
@@ -492,7 +492,7 @@
svn_wc_adm_access_t *adm_access;
svn_stringbuf_t *log_item = svn_stringbuf_create ("", pool);
- SVN_ERR (svn_wc_adm_open (&adm_access, pb->path, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, pb->path, TRUE, pool));
SVN_ERR (svn_wc__open_adm_file (&log_fp,
pb->path,
SVN_WC__ADM_LOG,
@@ -758,7 +758,7 @@
svn_stringbuf_t *entry_accum = svn_stringbuf_create ("", db->pool);
/* Lock down the administrative area */
- SVN_ERR (svn_wc_adm_open (&adm_access, db->path, TRUE, db->pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, db->path, TRUE, db->pool));
/* Open log file */
SVN_ERR (svn_wc__open_adm_file (&log_fp,
@@ -1192,7 +1192,7 @@
/* Lock the parent directory while we change things. If for some
reason the parent isn't under version control, this function will
bomb out. */
- SVN_ERR (svn_wc_adm_open (&adm_access, parent_dir, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, parent_dir, TRUE, pool));
/*
When this function is called on file F, we assume the following
Index: ./subversion/libsvn_wc/lock.c
===================================================================
--- ./subversion/libsvn_wc/lock.c
+++ ./subversion/libsvn_wc/lock.c Sun Jul 14 00:32:26 2002
@@ -59,17 +59,33 @@
return svn_wc__remove_adm_file (path, pool, SVN_WC__ADM_LOCK, NULL);
}
+static svn_wc_adm_access_t *
+svn_wc__adm_access_alloc (enum svn_wc__adm_access_type type,
+ svn_wc_adm_access_t *parent,
+ const char *path,
+ apr_pool_t *pool)
+{
+ svn_wc_adm_access_t *lock = apr_palloc (pool, sizeof (*lock));
+ lock->type = type;
+ lock->parent = parent;
+ lock->children = NULL;
+ lock->lock_exists = FALSE;
+ /* ### Some places lock with a path that is not canonical, we need
+ ### cannonical paths for reliable parent-child determination */
+ lock->path = svn_path_canonicalize_nts (apr_pstrdup (pool, path), pool);
+ lock->pool = pool;
+ return lock;
+}
+
svn_error_t *
svn_wc__adm_steal_write_lock (svn_wc_adm_access_t **adm_access,
const char *path,
apr_pool_t *pool)
{
svn_error_t *err;
- svn_wc_adm_access_t *lock = apr_palloc (pool, sizeof (**adm_access));
- lock->type = svn_wc_adm_access_write_lock;
- lock->lock_exists = FALSE;
- lock->path = apr_pstrdup (pool, path);
- lock->pool = pool;
+ svn_wc_adm_access_t *lock
+ = svn_wc__adm_access_alloc (svn_wc__adm_access_write_lock, NULL, path,
+ pool);
err = svn_wc__lock (lock, 0, pool);
if (err)
@@ -87,25 +103,68 @@
svn_error_t *
svn_wc_adm_open (svn_wc_adm_access_t **adm_access,
+ svn_wc_adm_access_t *parent_access,
const char *path,
svn_boolean_t write_lock,
apr_pool_t *pool)
{
- svn_wc_adm_access_t *lock = apr_palloc (pool, sizeof (**adm_access));
- lock->lock_exists = FALSE;
- lock->path = apr_pstrdup (pool, path);
- lock->pool = pool;
+ svn_wc_adm_access_t *lock;
+
+ if (parent_access)
+ {
+ const char *name;
+
+ /* PATH must be a child. We only need the result, not the allocated
+ name. */
+ name = svn_path_is_child (parent_access->path, path, pool);
+ if (!name)
+ return svn_error_createf (SVN_ERR_WC_INVALID_LOCK, 0, NULL, pool,
+ "lock path is not a child (%s)",
+ path);
+
+ if (parent_access->children)
+ {
+ lock = apr_hash_get (parent_access->children, path,
+ APR_HASH_KEY_STRING);
+ if (lock)
+ {
+ *adm_access = lock;
+ return SVN_NO_ERROR;
+ }
+ }
+ }
+ /* Need to create a new lock */
if (write_lock)
{
- lock->type = svn_wc_adm_access_write_lock;
+ lock = svn_wc__adm_access_alloc (svn_wc__adm_access_write_lock,
+ parent_access, path, pool);
SVN_ERR (svn_wc__lock (lock, 0, pool));
lock->lock_exists = TRUE;
}
else
{
- /* ### Read-lock not yet implemented */
- lock->type = svn_wc_adm_access_unlocked;
+ /* ### Read-lock not yet implemented. Since no physical lock gets
+ ### created we must check PATH is not a file. */
+ enum svn_node_kind node_kind;
+ SVN_ERR (svn_io_check_path (path, &node_kind, pool));
+ if (node_kind != svn_node_dir)
+ return svn_error_createf (SVN_ERR_WC_INVALID_LOCK, 0, NULL, pool,
+ "lock path is not a directory (%s)",
+ path);
+
+ lock = svn_wc__adm_access_alloc (svn_wc__adm_access_unlocked,
+ parent_access, path, pool);
+ }
+
+ lock->parent = parent_access;
+ if (lock->parent)
+ {
+ if (! lock->parent->children)
+ lock->parent->children = apr_hash_make (lock->parent->pool);
+
+ apr_hash_set (lock->parent->children, lock->path, APR_HASH_KEY_STRING,
+ lock);
}
*adm_access = lock;
@@ -115,7 +174,25 @@
svn_error_t *
svn_wc_adm_close (svn_wc_adm_access_t *adm_access)
{
- if (adm_access->type == svn_wc_adm_access_write_lock)
+ apr_hash_index_t *hi;
+
+ /* Close children */
+ if (adm_access->children)
+ {
+ for (hi = apr_hash_first (adm_access->pool, adm_access->children);
+ hi;
+ hi = apr_hash_next (hi))
+ {
+ void *val;
+ svn_wc_adm_access_t *child_access;
+ apr_hash_this (hi, NULL, NULL, &val);
+ child_access = val;
+ SVN_ERR (svn_wc_adm_close (child_access));
+ }
+ }
+
+ /* Physically unlock if required */
+ if (adm_access->type == svn_wc__adm_access_write_lock)
{
if (adm_access->lock_exists)
{
@@ -123,16 +200,21 @@
adm_access->lock_exists = FALSE;
}
/* Reset to prevent further use of the write lock. */
- adm_access->type = svn_wc_adm_access_unlocked;
+ adm_access->type = svn_wc__adm_access_unlocked;
}
+ /* Detach from parent */
+ if (adm_access->parent)
+ apr_hash_set (adm_access->parent->children, adm_access->path,
+ APR_HASH_KEY_STRING, NULL);
+
return SVN_NO_ERROR;
}
svn_error_t *
svn_wc_adm_write_check (svn_wc_adm_access_t *adm_access)
{
- if (adm_access->type == svn_wc_adm_access_write_lock)
+ if (adm_access->type == svn_wc__adm_access_write_lock)
{
if (adm_access->lock_exists)
{
Index: ./subversion/libsvn_client/externals.c
===================================================================
--- ./subversion/libsvn_client/externals.c
+++ ./subversion/libsvn_client/externals.c Sun Jul 14 00:32:26 2002
@@ -232,7 +232,7 @@
svn_error_t *err;
svn_wc_adm_access_t *adm_access;
- SVN_ERR (svn_wc_adm_open (&adm_access, path, TRUE, pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, path, TRUE, pool));
err = svn_wc_remove_from_revision_control (adm_access,
SVN_WC_ENTRY_THIS_DIR,
TRUE,
@@ -378,7 +378,7 @@
svn_error_t *err;
svn_wc_adm_access_t *adm_access;
- SVN_ERR (svn_wc_adm_open (&adm_access, path, TRUE, ib->pool));
+ SVN_ERR (svn_wc_adm_open (&adm_access, NULL, path, TRUE, ib->pool));
/* We don't use relegate_external() here, because we know that
nothing else in this externals description (at least) is
Index: ./subversion/libsvn_client/commit_util.c
===================================================================
--- ./subversion/libsvn_client/commit_util.c
+++ ./subversion/libsvn_client/commit_util.c Sun Jul 14 00:40:22 2002
@@ -60,8 +60,15 @@
if (! apr_hash_get (locked_dirs, dir, APR_HASH_KEY_STRING))
{
- svn_wc_adm_access_t *adm_access;
- SVN_ERR (svn_wc_adm_open (&adm_access, dir, TRUE, hash_pool));
+ /* Short lived, don't use hash pool */
+ const char *parent_dir = svn_path_remove_component_nts (dir, pool);
+ svn_wc_adm_access_t *adm_access, *parent_access;
+
+ /* Get parent, NULL is OK */
+ parent_access = apr_hash_get (locked_dirs, parent_dir,
+ APR_HASH_KEY_STRING);
+ SVN_ERR (svn_wc_adm_open (&adm_access, parent_access, dir, TRUE,
+ hash_pool));
apr_hash_set (locked_dirs, apr_pstrdup (hash_pool, dir),
APR_HASH_KEY_STRING, adm_access);
}
Index: ./subversion/libsvn_client/commit.c
===================================================================
--- ./subversion/libsvn_client/commit.c
+++ ./subversion/libsvn_client/commit.c Sun Jul 14 00:32:26 2002
@@ -919,15 +919,12 @@
svn_path_split_nts (item->path, &adm_access_path, NULL, pool);
adm_access = apr_hash_get (locked_dirs, adm_access_path,
APR_HASH_KEY_STRING);
-
- if ( ! adm_access )
- {
- SVN_ERR (svn_wc_adm_open (&adm_access, adm_access_path, TRUE,
- pool));
- apr_hash_set (locked_dirs, apr_pstrdup(pool, adm_access_path),
- APR_HASH_KEY_STRING, adm_access);
- }
-
+ if (! adm_access)
+ /* This should *never* happen */
+ return svn_error_createf(SVN_ERR_GENERAL, 0, NULL, pool,
+ "BUG: no lock for %s",
+ item->path);
+
if ((item->state_flags & SVN_CLIENT_COMMIT_ITEM_ADD)
&& (item->kind == svn_node_dir)
&& (item->copyfrom_url))
Index: ./subversion/tests/clients/cmdline/copy_tests.py
===================================================================
--- ./subversion/tests/clients/cmdline/copy_tests.py
+++ ./subversion/tests/clients/cmdline/copy_tests.py Sun Jul 14 01:11:13 2002
@@ -470,6 +470,39 @@
return 0
+# Takes out working-copy locks for A/B2 and child A/B2/E. At one stage
+# during issue 749 the second lock cause an already-locked error.
+def copy_modify_commit(sbox):
+ "copy a directory hierarchy and modify before commit"
+
+ if sbox.build():
+ return 1
+
+ wc_dir = sbox.wc_dir
+ outlines, errlines = svntest.main.run_svn(None, 'cp',
+ wc_dir + '/A/B', wc_dir + '/A/B2',
+ '-m', 'fooogle')
+ if errlines:
+ print "Whoa, failed to copy A/B to A/B2"
+ return 1
+
+ alpha_path = os.path.join(wc_dir, 'A', 'B2', 'E', 'alpha')
+ svntest.main.file_append(alpha_path, "modified alpha")
+
+ expected_output = svntest.wc.State(wc_dir, {
+ 'A/B2' : Item(verb='Adding'),
+ 'A/B2/E/alpha' : Item(verb='Sending'),
+ })
+
+ if svntest.actions.run_and_verify_commit (wc_dir,
+ expected_output,
+ None,
+ None,
+ None, None,
+ None, None,
+ wc_dir):
+ return 1
+
########################################################################
# Run the tests
@@ -481,6 +514,7 @@
receive_copy_in_update,
resurrect_deleted_dir,
no_copy_overwrites,
+ copy_modify_commit,
]
if __name__ == '__main__':
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 14 02:32:02 2002