Re: [PATCH]: Avoid extra stat calls during report_revisions on some filesystems

Daniel Berlin <dberlin@dberlin.org> writes:

> Index: subversion/libsvn_wc/lock.c
> ===================================================================
> --- subversion/libsvn_wc/lock.c (revision 17369)
> +++ subversion/libsvn_wc/lock.c (working copy)
> @@ -675,10 +675,89 @@ svn_wc_adm_retrieve (svn_wc_adm_access_t
> generally makes the calling code simpler as it doesn't need to check
> for NULL batons. */
> if (! *adm_access)
> - return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> - _("Working copy '%s' is missing or not locked"),
> - svn_path_local_style (path, pool));
> + {
> + const char *wcpath;
> + const svn_wc_entry_t *subdir_entry;
> + svn_node_kind_t wckind;
> + svn_node_kind_t kind;
> + svn_error_t *err;
> +
> + err = svn_wc_entry (&subdir_entry, path, associated, TRUE, pool);
> +
> + /* If we can't get an entry here, we are in pretty bad shape,
> + and will have to fall back to using just regular old paths to
> + see what's going on. */
> + if (err)
> + {
> + err = NULL;

You need to svn_error_clear if you are going to ignore err.

> + subdir_entry = NULL;
> + }
> +
> + err = svn_io_check_path (path, &kind, pool);
> +
> + /* If we can't check the path, we can't make a good error
> + message. */
> + if (err)
> + {
> + svn_error_clear (err);
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Unable to check path existence for %s"),
> + svn_path_local_style (path, pool));
> + }
> +
> + if (subdir_entry)
> + {
> + if (subdir_entry->kind == svn_node_dir
> + && kind == svn_node_file)
> + {
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Expected %s to be a directory but found a file"),
> + svn_path_local_style (path, pool));
> + }
> + else if (subdir_entry->kind == svn_node_file
> + && kind == svn_node_dir)
> + {
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Expected %s to be a file but found a directory"),
> + svn_path_local_style (path, pool));
> + }
> + }
> +
> + wcpath = svn_wc__adm_path (path, FALSE, pool, NULL);
> + err = svn_io_check_path (wcpath, &wckind, pool);

That's the second new stat() call in a function that previously did no
disk IO at all :-( You may have improved update but have other
operations have been penalised?

At the very least you should use the result of the first stat() to
avoid doing the second stat() when it must fail. The access baton
stuff already has support for "missing" directories, can you use that
to avoid some of the stat() calls?

> +
> + /* If we can't check the path, we can't make a good error
> + message. */
> + if (err)
> + {
> + svn_error_clear (err);
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Unable to check path existence for %s"),
> + svn_path_local_style (wcpath, pool));
> + }
> +
> + if (kind == svn_node_none)
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Directory '%s' is missing"),
> + svn_path_local_style (path, pool));
> +
> + else if (kind == svn_node_dir && wckind == svn_node_none)
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Directory '%s' containing working copy admin area is missing"),
> + svn_path_local_style (wcpath, pool));
> +
> + else if (kind == svn_node_dir && wckind == svn_node_dir)
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Unable to lock %s"),
> + svn_path_local_style (path, pool));
> +
> + /* If all else fails, return our useless generic error. */
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Working copy '%s' is not locked"),
> + svn_path_local_style (path, pool));
>
> +
> + }
> 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