Jon, could you include an updated log message w/ every post of the
patch? Even if the log message hasn't changed since the last post,
this saves people from having to go dig it up from the archives.
Thanks,
-Karl
Jon Foster <jon@jon-foster.co.uk> writes:
> 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Mar 6 01:27:44 2004