On Tue, Jun 23, 2009 at 08:22:19PM +0800, yellow.flying wrote:
> Hey Stefan,
>
> 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.
> * 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
> ]]]
Nice patch, I've reviewed it, see below.
Index: subversion/include/private/svn_wc_private.h
===================================================================
--- subversion/include/private/svn_wc_private.h (版本 38138)
+++ subversion/include/private/svn_wc_private.h (工作副本)
@@ -219,6 +219,12 @@
svn_wc_adm_access_t *adm_access,
apr_pool_t *pool);
+/** Given @a *path, find its working copy root path @a **wc_root_path */
+svn_error_t *
+svn_wc__find_wc_root(const char **wc_root_path,
+ const char *path,
+ apr_pool_t *pool);
+
In this docstring, you should also mention what pool is used for.
Is pool used for temporary allocations? Only for allocation of the result?
For example:
Use @a pool for temporary allocations.
Or:
Allocate the result in @a pool.
Your new function needs both, so consider passing two pool arguments,
like this:
svn_wc__find_wc_root(const char **wc_root_path,
const char *path,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
Then the docstring should say:
/** Given @a *path, find its working copy root path @a **wc_root_path
Allocate the result in @a result_pool.
Use @a scratch_pool for temporary allocations. */
Also, the docstring should explain what happens if path is not
a working copy path (for example "D:\TEMP").
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/libsvn_wc/update_editor.c
===================================================================
--- subversion/libsvn_wc/update_editor.c (版本 38138)
+++ subversion/libsvn_wc/update_editor.c (工作副本)
@@ -5324,6 +5324,38 @@
}
+svn_error_t *
+svn_wc__find_wc_root(const char **wc_root_path, const char *path,
So instead of this line:
+ apr_pool_t *pool)
You need:
apr_pool_t *result_pool,
apr_pool_t *scratch_pool)
+{
+ svn_wc_adm_access_t *adm_access;
+ svn_boolean_t wc_root;
Here you need this:
apr_pool_t *iterpool;
iterpool = svn_pool_create(scratch_pool);
Below, I will explain why.
+
+ while(1)
+ {
Here you need:
svn_pool_clear(iterpool);
Again, see below for explanation.
+ SVN_ERR(svn_wc_adm_open3(&adm_access, NULL, path,
+ FALSE, /* Write lock */
+ 0, /* lock levels */
+ NULL, NULL, pool));
This should be:
NULL, NULL, iterpool));
Because when using the pool within a loop like you did,
the pool will allocate new memory with each iteration.
So if the loop takes, for example, 5 times to complete,
we will allocate new memory 5 times. If the loop takes,
say, 30 times to complete, we will allocate new memory 30 times.
This wastes memory.
Instead, you need what we call an "iterpool" (iteration pool).
This pool is used for temporary allocations which need to be
made once with each iteration of the loop.
Above I've shown you how to create the iterpool, and also
how we re-use the memory managed by the iterpool by clearing
the iterpool. Once the pool has been cleared, its memory is marked
for re-use, and the pool can use the same memory again instead of
getting new memory for new allocations.
So now, no matter how often the loop iterates, we will always use
the same amount of memory.
See also: http://subversion.tigris.org/hacking.html#apr-pools
+
+ /* Here, ADM_ACCESS refers to PATH. */
+ svn_wc_is_wc_root(&wc_root, path, adm_access, pool);
+ svn_wc_adm_close2(adm_access, pool);
For the above two function calls, you should also use the iterpool
+
+ if (wc_root)
+ {
+ *wc_root_path = path;
Here, you should allocate a new copy of the path in the result pool.
When people use a function which takes a pointer to a pointer (like
**wc_root_path), they will assume that the result will be freshly
allocated. Since this is the result of the function, we allocate
it in result_pool:
*wc_root_path = apr_pstrdup(result_pool, path);
+ break;
+ }
+
+ /* path point to its parent now*/
+ path = svn_dirent_dirname(path, pool);
Again, use the iterpool here.
+ }
Here, the loop is done, so we don't need the iterpool anymore.
Make sure to destroy it!
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,
Stefan
Received on 2009-06-23 16:11:43 CEST