Philip Martin wrote:
> I like this patch, I'll approve it for the trunk subject to the
> minor cosmetic changes below:
[snip]
Patch with your requested changes is attached. It's against trunk r8891.
Any chance of getting this into 1.0.1?
There are other two things I want to do, but these shouldn't stop
you from applying this patch:
1) DJ spotted a subtle bug in my original patch (fixed in this patch).
I will write a test-case for this so that in future a "make check"
would spot it.
2) I'd like to reduce further the number of locks that are taken.
This patch still errs on the side of locking too much, which gives
correct output but is not as fast as possible. It locks much less
than before, though. This looks like it will require careful
thought and testing - which is one reason to write the test-cases
first. (This is the FIXME in the patch).
Kind regards,
Jon Foster
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 8891)
+++ subversion/include/svn_wc.h (working copy)
@@ -76,14 +76,18 @@
* to the set containing @a associated. @a associated can be @c NULL, in
* which case @a adm_access is the start of a new set.
*
- * If @a tree_lock is @c TRUE then the working copy directory hierarchy under
- * @a path will be locked. All the access batons will become part of the set
- * containing @a adm_access. This is an all-or-nothing option, if it is not
- * possible to lock the entire tree then an error will be returned and
- * @a adm_access will be invalid, with the exception that subdirectories of
- * @a path that are missing from the physical filesystem will not be locked
- * and will not cause an error. The error @c SVN_ERR_WC_LOCKED will be
- * returned if a subdirectory of @a path is already write locked.
+ * @a depth specifies how much to lock. Zero means just the specified
+ * directory. Any negative value means to lock the entire working copy
+ * directory hierarchy under @a path. A positive value indicates the number of
+ * levels of directories to lock - 1 means just immediate subdirectories, 2
+ * means immediate subdirectories and their subdirectories, etc. All the
+ * access batons will become part of the set containing @a adm_access. This
+ * is an all-or-nothing option, if it is not possible to lock all the
+ * requested directories then an error will be returned and @a adm_access will
+ * be invalid, with the exception that subdirectories of @a path that are
+ * missing from the physical filesystem will not be locked and will not cause
+ * an error. The error @c SVN_ERR_WC_LOCKED will be returned if a
+ * subdirectory of @a path is already write locked.
*
* @a pool will be used to allocate memory for the baton and any subsequently
* cached items. If @a adm_access has not been closed when the pool is
@@ -95,6 +99,19 @@
* the longest lifetime of all the batons in the set. This implies it must be
* the root of the hierarchy.
*/
+svn_error_t *svn_wc_adm_open_depth (svn_wc_adm_access_t **adm_access,
+ svn_wc_adm_access_t *associated,
+ const char *path,
+ svn_boolean_t write_lock,
+ int depth,
+ apr_pool_t *pool);
+
+/**
+ * Similar to svn_wc_adm_open_depth(). @a depth is set to -1 if @a tree_lock is
+ * @c TRUE, else 0.
+ *
+ * Provided for backward compatibility with the Subversion 1.0.0 API.
+ */
svn_error_t *svn_wc_adm_open (svn_wc_adm_access_t **adm_access,
svn_wc_adm_access_t *associated,
const char *path,
@@ -104,14 +121,27 @@
/** Checks the working copy to determine the node type of @a path. If
* @a path is a versioned directory then the behaviour is like that of
- * @c svn_wc_adm_open, otherwise, if @a path is a file or does not
- * exist, then the behaviour is like that of @c svn_wc_adm_open with
+ * @c svn_wc_adm_open_depth, otherwise, if @a path is a file or does not
+ * exist, then the behaviour is like that of @c svn_wc_adm_open_depth with
* @a path replaced by the parent directory of @a path. If @a path is
* an unversioned directory, the behaviour is also like that of
- * @c svn_wc_adm_open on the parent, except that if the open fails,
+ * @c svn_wc_adm_open_depth on the parent, except that if the open fails,
* then the returned SVN_ERR_WC_NOT_DIRECTORY error refers to @a path,
* not to @a path's parent.
*/
+svn_error_t *svn_wc_adm_probe_open_depth (svn_wc_adm_access_t **adm_access,
+ svn_wc_adm_access_t *associated,
+ const char *path,
+ svn_boolean_t write_lock,
+ int depth,
+ apr_pool_t *pool);
+
+/**
+ * Similar to svn_wc_adm_probe_open_depth(). @a depth is set to -1 if
+ * @a tree_lock is @c TRUE, else 0.
+ *
+ * Provided for backward compatibility with the Subversion 1.0.0 API.
+ */
svn_error_t *svn_wc_adm_probe_open (svn_wc_adm_access_t **adm_access,
svn_wc_adm_access_t *associated,
const char *path,
@@ -149,8 +179,8 @@
*
* First, try to obtain @a *adm_access via @c svn_wc_adm_probe_retrieve(),
* but if this fails because @a associated can't give a baton for
- * @a path or @a path's parent, then try @c svn_wc_adm_probe_open(),
- * this time passing @a write_lock and @a tree_lock. If there is
+ * @a path or @a path's parent, then try @c svn_wc_adm_probe_open_depth(),
+ * this time passing @a write_lock and @a depth. If there is
* still no access because @a path is not a versioned directory, then
* just set @a *adm_access to null and return success. But if it is
* because @a path is locked, then return the error @c SVN_ERR_WC_LOCKED,
@@ -163,6 +193,19 @@
*
* Use @a pool only for local processing, not to allocate @a *adm_access.
*/
+svn_error_t *svn_wc_adm_probe_try_depth (svn_wc_adm_access_t **adm_access,
+ svn_wc_adm_access_t *associated,
+ const char *path,
+ svn_boolean_t write_lock,
+ int depth,
+ apr_pool_t *pool);
+
+/**
+ * Similar to svn_wc_adm_probe_try_depth(). @a depth is set to -1 if
+ * @a tree_lock is @c TRUE, else 0.
+ *
+ * Provided for backward compatibility with the Subversion 1.0.0 API.
+ */
svn_error_t *svn_wc_adm_probe_try (svn_wc_adm_access_t **adm_access,
svn_wc_adm_access_t *associated,
const char *path,
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c (revision 8891)
+++ subversion/libsvn_wc/lock.c (working copy)
@@ -331,7 +331,7 @@
svn_wc_adm_access_t *associated,
const char *path,
svn_boolean_t write_lock,
- svn_boolean_t tree_lock,
+ int depth,
svn_boolean_t under_construction,
apr_pool_t *pool)
{
@@ -396,12 +396,16 @@
SVN_ERR (maybe_upgrade_format (lock, pool));
}
- if (tree_lock)
+ if (depth != 0)
{
apr_hash_t *entries;
apr_hash_index_t *hi;
apr_pool_t *subpool = svn_pool_create (pool);
+ /* Reduce depth since we are about to recurse */
+ if (depth > 0)
+ depth--;
+
/* Ask for the deleted entries because most operations request them
at some stage, getting them now avoids a second file parse. */
SVN_ERR (svn_wc_entries_read (&entries, lock, TRUE, subpool));
@@ -429,7 +433,7 @@
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, tree_lock,
+ err = do_open (&entry_access, lock, entry_path, write_lock, depth,
FALSE, lock->pool);
if (err)
{
@@ -496,6 +500,7 @@
return SVN_NO_ERROR;
}
+/* To preserve API compatibility with Subversion 1.0.0 */
svn_error_t *
svn_wc_adm_open (svn_wc_adm_access_t **adm_access,
svn_wc_adm_access_t *associated,
@@ -504,7 +509,19 @@
svn_boolean_t tree_lock,
apr_pool_t *pool)
{
- return do_open (adm_access, associated, path, write_lock, tree_lock, FALSE,
+ return svn_wc_adm_open_depth (adm_access, associated, path, write_lock,
+ (tree_lock ? -1 : 0), pool);
+}
+
+svn_error_t *
+svn_wc_adm_open_depth (svn_wc_adm_access_t **adm_access,
+ svn_wc_adm_access_t *associated,
+ const char *path,
+ svn_boolean_t write_lock,
+ int depth,
+ apr_pool_t *pool)
+{
+ return do_open (adm_access, associated, path, write_lock, depth, FALSE,
pool);
}
@@ -513,10 +530,11 @@
const char *path,
apr_pool_t *pool)
{
- return do_open (adm_access, NULL, path, TRUE, FALSE, TRUE, pool);
+ return do_open (adm_access, NULL, path, TRUE, 0, TRUE, pool);
}
-
+
+/* To preserve API compatibility with Subversion 1.0.0 */
svn_error_t *
svn_wc_adm_probe_open (svn_wc_adm_access_t **adm_access,
svn_wc_adm_access_t *associated,
@@ -525,6 +543,19 @@
svn_boolean_t tree_lock,
apr_pool_t *pool)
{
+ return svn_wc_adm_probe_open_depth (adm_access, associated, path,
+ write_lock, (tree_lock ? -1 : 0), pool);
+}
+
+
+svn_error_t *
+svn_wc_adm_probe_open_depth (svn_wc_adm_access_t **adm_access,
+ svn_wc_adm_access_t *associated,
+ const char *path,
+ svn_boolean_t write_lock,
+ int depth,
+ apr_pool_t *pool)
+{
svn_error_t *err;
const char *dir;
int wc_format;
@@ -532,14 +563,14 @@
SVN_ERR (probe (&dir, path, &wc_format, pool));
/* If we moved up a directory, then the path is not a directory, or it
- is not under version control. In either case, the notion of a tree_lock
+ is not under version control. In either case, the notion of a depth
does not apply to the provided path. Disable it so that we don't end
up trying to lock more than we need. */
if (dir != path)
- tree_lock = FALSE;
+ depth = 0;
- err = svn_wc_adm_open (adm_access, associated, dir, write_lock, tree_lock,
- pool);
+ err = svn_wc_adm_open_depth (adm_access, associated, dir, write_lock,
+ depth, pool);
if (err)
{
svn_error_t *err2;
@@ -636,6 +667,7 @@
}
+/* To preserve API compatibility with Subversion 1.0.0 */
svn_error_t *
svn_wc_adm_probe_try (svn_wc_adm_access_t **adm_access,
svn_wc_adm_access_t *associated,
@@ -644,6 +676,18 @@
svn_boolean_t tree_lock,
apr_pool_t *pool)
{
+ return svn_wc_adm_probe_try_depth (adm_access, associated, path, write_lock,
+ (tree_lock ? -1 : 0), pool);
+}
+
+svn_error_t *
+svn_wc_adm_probe_try_depth (svn_wc_adm_access_t **adm_access,
+ svn_wc_adm_access_t *associated,
+ const char *path,
+ svn_boolean_t write_lock,
+ int depth,
+ apr_pool_t *pool)
+{
svn_error_t *err;
err = svn_wc_adm_probe_retrieve (adm_access, associated, path, pool);
@@ -654,9 +698,9 @@
if (err && (err->apr_err == SVN_ERR_WC_NOT_LOCKED))
{
svn_error_clear (err);
- err = svn_wc_adm_probe_open (adm_access, associated,
- path, write_lock, tree_lock,
- svn_wc_adm_access_pool (associated));
+ err = svn_wc_adm_probe_open_depth (adm_access, associated,
+ path, write_lock, depth,
+ svn_wc_adm_access_pool (associated));
/* If the path is not a versioned directory, we just return a
null access baton with no error. Note that of the errors we
Index: subversion/libsvn_client/status.c
===================================================================
--- subversion/libsvn_client/status.c (revision 8891)
+++ subversion/libsvn_client/status.c (working copy)
@@ -121,10 +121,17 @@
/* Close up our ADM area. We'll be re-opening soon. */
SVN_ERR (svn_wc_adm_close (adm_access));
- /* Need to lock the tree as even a non-recursive status requires the
- immediate directories to be locked. */
- SVN_ERR (svn_wc_adm_probe_open (&adm_access, NULL, anchor,
- FALSE, TRUE, pool));
+ /* Need to lock the tree. A recursive status requires us to lock the whole
+ tree. A non-recursive status requires the target directory and immediate
+ subdirectories to be locked. However, if the user does "svn status
+ --non-recursive dir1/dir2", then we start locking from dir1, so in order
+ to lock the children of dir2 we need a depth of 2.
+ ### FIXME: In the non-recursive case this always locks too much. This
+ ### is a performance bug. (But this is better than locking too
+ ### little, which would be a correctness bug).
+ */
+ SVN_ERR (svn_wc_adm_probe_open_depth (&adm_access, NULL, anchor,
+ FALSE, (descend ? -1 : 2), pool));
/* Get the status edit, and use our wrapping status function/baton
as the callback pair. */
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Mar 5 00:11:11 2004