HuiHuang <yellow.flying_at_yahoo.com.cn> writes:
> +/** Given @a *path, find its strictly working copy root path @a **wc_root_path
> + Allocate the result in @a result_pool.
> + Use @a scratch_pool for temporary allocations.
> + The error @c SVN_ERR_WC_NOT_DIRECTORY will be returned if
> + * @a path is not a versioned directory.*/
> +svn_error_t *
> +svn_wc__find_wc_root(const char **wc_root_path,
> + const char *path,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool);
> +
I haven't really been following the discussion about why you are
writing this particular function, but the experience from the pre-1.0
days is that interfaces based around paths don't work very well. The
problem is that one needs to read the WC metadata before the paths can
be manipulated and that data then gets thrown away, this makes the
function expensive and slow.
In the longer term, assuming WC-NG ends up with a single database in
the root, it's hard to see why one would want this interface at all.
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 38192)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -5324,6 +5324,59 @@
> }
>
>
> +svn_error_t *
> +svn_wc__find_wc_root(const char **wc_root_path, const char *path,
> + apr_pool_t *result_pool,
> + apr_pool_t *scratch_pool)
> +{
> + svn_wc_adm_access_t *adm_access;
> + svn_boolean_t wc_root;
> + apr_pool_t *iterpool;
> + svn_node_kind_t kind;
> + const char *absolute;
> + iterpool = svn_pool_create(scratch_pool);
> +
> + /* convert path from local style to internal style */
> + path = svn_dirent_internal_style(path, scratch_pool);
A comment like the one above doesn't help because it doesn't add any
information, the sole purpose of the function is to do what the
comment states. All the comments in this function are the same: they
take up space but don't add anything.
> + path = svn_dirent_canonicalize(path, scratch_pool);
> + /* if path is a relative path, get its absolute path */
> + SVN_ERR(svn_dirent_get_absolute(&absolute, path, scratch_pool));
Now this is interesting because switching to absolute paths is
unusual. Note the comment doesn't tell me *why* you doing it.
> + path = absolute;
Personally I'd avoid assigning absolute to path; I'd prefer to use
absolute, rather than path, in the code below.
> +
> + /* check path's kind */
> + SVN_ERR(svn_io_check_path(path, &kind, scratch_pool));
> + /* If path is a file, we get its parent dir */
> + if (kind == svn_node_file)
> + path = svn_dirent_dirname(path, scratch_pool);
Doing svn_io_check_path like that isn't good from a performance point
of view, it tends to be a consequence of using path based interfaces.
From an efficiency point of view one could catch the first open3 call
returning NOT_DIRECTORY and do a stat at that point; in practice that
might not be practical.
I'm not really clear why you are doing this (the comments state what
but not why) but I suppose this allows you to do
svn_wc__find_wc_root("foo/bar")
where bar is a file. However it seems to be inconsistent. If bar is
an unversioned file in a versioned directory the function succeeds,
but if it's a versioned file that's missing from disk the function
fails. bar could also be a versioned or unversioned symlink or it
could be an unversioned directory obstructing a versioned item.
> +
> + while(1)
> + {
> + svn_pool_clear(iterpool);
> + SVN_ERR(svn_wc_adm_open3(&adm_access, NULL, path,
> + FALSE, /* Write lock */
> + 0, /* lock levels */
> + NULL, NULL, iterpool));
> +
> + /* Here, ADM_ACCESS refers to PATH. */
> + SVN_ERR(svn_wc_is_wc_root(&wc_root, path, adm_access,
> + iterpool));
> + svn_wc_adm_close2(adm_access, iterpool);
That leaks an error.
> +
> + if (wc_root)
> + {
> + *wc_root_path = apr_pstrdup(result_pool, path);
> + break;
path is absolute, so this function is returning an absolute path.
That defintely needs to be documented in the header file. The
question is why? I guess you want to get to roots that are outside
the current working directory?
> + }
> +
> + /* path point to its parent now*/
> + path = svn_dirent_dirname(path, result_pool);
> + }
> + svn_pool_destroy(iterpool);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> svn_error_t*
> svn_wc__strictly_is_wc_root(svn_boolean_t *wc_root,
> const char *path,
>
> log message:
>
> [[[
>
> * subversion/include/private/svn_wc_private.h
> (svn_wc__find_wc_root): new API. Given a path, it return the
> working copy root path. If path is not a versioned directory,
> The error SVN_ERR_WC_NOT_DIRECTORY will be returned.
The NOT_DIRECTORY bit seems to contradict the code: if path is a file
you take the parent directory.
> * subversion/libsvn_wc/update_editor.c
> ((svn_wc__find_wc_root): new function. It is implement of
> API svn_wc__find_wc_root in svn_wc_private.h
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2365330
Received on 2009-06-25 17:13:35 CEST