Thanks stefan for your detailed review.
I will keep in mind all your review comments. I will correct my mistakes
in future patches.
Attached the updated patch and log message.
Thanks & Regards,
Vijayaguru
On Tuesday 06 November 2012 07:31 PM, Stefan Sperling wrote:
On Mon, Nov 05, 2012 at 09:54:56PM +0530, vijay wrote:
Hi,
This patch implements '--include-externals' option to 'svn list'
[Issue #4225] [1].
All tests pass with 'make check' & 'make davautocheck'.
Attached the patch and log message.
Please review this patch and share your thoughts.
My review comments are below.
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1405691)
+++ subversion/include/svn_client.h (working copy)
@@ -5290,11 +5290,41 @@
* the entry's lock, if it is locked and if lock information is being
* reported by the caller; otherwise @a lock is NULL. @a abs_path is the
* repository path of the top node of the list operation; it is relative to
- * the repository root and begins with "/". @a pool may be used for
- * temporary allocations.
+ * the repository root and begins with "/".
*
- * @since New in 1.4.
+ * If svn_client_list3() was called with @a include_externals set to TRUE,
+ * @a notify_external_start, @a notify_external_end, @a external_parent_url
+ * and @a external_target will be set. @a notify_external_start and
+ * @a notify_external_end is used to control the list output of externals.
+ * @a external_parent_url is url of the directory which has the externals
+ * definitions. @a external_target is the target subdirectory of externals
+ * definitions.
The purpose of the notify_external_start/end booleans isn't clear at first
sight. The docstring doesn't explain why these flags are necessary.
Later in the patch, in a code comment you explain that you did this to
more easily support XML listing mode. Maybe mention this here, too?
+
+ * @a pool may be used for temporary allocations.
Please call this 'pool' parameter scratch_pool.
Index: subversion/libsvn_client/client.h
===================================================================
--- subversion/libsvn_client/client.h (revision 1405691)
+++ subversion/libsvn_client/client.h (working copy)
@@ -1009,7 +1009,6 @@
svn_client_ctx_t *ctx,
apr_pool_t *pool);
-
This is an unnecessary whitespace change.
/* Perform status operations on each external in EXTERNAL_MAP, a const char *
local_abspath of all externals mapping to the const char* defining_abspath.
All other options are the same as those passed to svn_client_status(). */
@@ -1024,6 +1023,21 @@
void *status_baton,
apr_pool_t *pool);
+/* List external items defined on each external in EXTERNALS, a const char *
+ externals_parent_url(url of the directory which has the externals
+ definitions) of all externals mapping to the const char * externals_desc
+ (externals description text). All other options are the same as those
+ passed to svn_client_list(). */
+svn_error_t *
+svn_client__list_externals(apr_hash_t *externals,
+ svn_depth_t depth,
+ apr_uint32_t dirent_fields,
+ svn_boolean_t fetch_locks,
+ svn_client_list_func2_t list_func,
+ void *baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
Please call this 'pool' parameter scratch_pool.
Index: subversion/libsvn_client/deprecated.c
===================================================================
--- subversion/libsvn_client/deprecated.c (revision 1405691)
+++ subversion/libsvn_client/deprecated.c (working copy)
@@ -1269,7 +1269,75 @@
}
/*** From list.c ***/
+
+/* Baton for use with wrap_list_func */
+struct list_func_wrapper_baton {
+ void *baton;
+ svn_client_list_func_t list_func;
I'd suggest using the following names instead, for clarity:
svn_client_list_func_t list_func1;
void *list_func1_baton;
+};
+
+/* This implements svn_client_list_func2_t */
+static svn_error_t *
+list_func_wrapper(void *baton,
+ const char *path,
+ const svn_dirent_t *dirent,
+ const svn_lock_t *lock,
+ const char *abs_path,
+ svn_boolean_t notify_external_start,
+ svn_boolean_t notify_external_end,
+ const char *external_parent_url,
+ const char *external_target,
+ apr_pool_t *pool)
Again, this is a scratch pool so it should be called scratch_pool.
+static void
+wrap_list_func(svn_client_list_func2_t *list_func2,
+ void **list_func2_baton,
+ svn_client_list_func_t list_func,
+ void *baton,
+ apr_pool_t *pool)
scratch_pool
Index: subversion/libsvn_client/externals.c
===================================================================
--- subversion/libsvn_client/externals.c (revision 1405691)
+++ subversion/libsvn_client/externals.c (working copy)
@@ -1180,3 +1180,95 @@
return SVN_NO_ERROR;
}
+
+svn_error_t *
+svn_client__list_externals(apr_hash_t *externals,
+ svn_depth_t depth,
+ apr_uint32_t dirent_fields,
+ svn_boolean_t fetch_locks,
+ svn_client_list_func2_t list_func,
+ void *baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
scratch_pool
+{
+ apr_pool_t *iterpool = svn_pool_create(pool);
+ apr_pool_t *sub_iterpool = svn_pool_create(pool);
Perhaps avoid having two iterpools in the same function by moving the
inner loop which uses the second iterpool into a separate helper function?
+ apr_hash_index_t *hi;
+
+ for (hi = apr_hash_first(pool, externals);
+ hi;
+ hi = apr_hash_next(hi))
+ {
+ const char *externals_parent_url = svn__apr_hash_index_key(hi);
+ svn_string_t *externals_desc = svn__apr_hash_index_val(hi);
+ const char *externals_parent_repos_root_url;
+ apr_array_header_t *items;
+ int i;
+
+ svn_pool_clear(iterpool);
+
+ items = apr_array_make(iterpool, 1, sizeof(svn_wc_external_item2_t*));
+
+ SVN_ERR(svn_client_get_repos_root(&externals_parent_repos_root_url,
+ NULL /* uuid */,
+ externals_parent_url, ctx,
+ iterpool, iterpool));
+
+ SVN_ERR(svn_wc_parse_externals_description3(&items,
+ externals_parent_url,
+ externals_desc->data,
+ FALSE, iterpool));
+
+ if (! items->nelts)
+ continue;
+
+ for (i = 0; i < items->nelts; i++)
+ {
So I mean instead of this inner loop, have a helper function, such as:
static svn_error_t *
list_external_items(apr_array_header_t *items,
...,
apr_pool_t *scratch_pool);
and call it here like this:
SVN_ERR(list_external_items(items, ..., iterpool));
The list_external_items() function would contain this loop and
would be free to reuse the name 'iterpool' instead of sub_iterpool.
+ /* Notify that we're about to handle an external. */
+ SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
+ TRUE /*notify_external_start */,
+ FALSE /*notify_external_end */,
+ externals_parent_url,
+ item->target_dir, iterpool));
Looks like you're accidentally not using the sub_iterpool here ;)
A helper function would avoid this kind of problem.
+
+ /* List the external */
+ SVN_ERR(wrap_external_error(
+ ctx, item->target_dir,
Please format the above two lines as a single line:
SVN_ERR(wrap_external_error(ctx, item->target_dir,
+ svn_client_list3(resolved_url,
+ &item->peg_revision,
+ &item->revision,
+ depth, dirent_fields,
+ fetch_locks,
+ TRUE,
+ list_func, baton, ctx,
+ sub_iterpool),
+ sub_iterpool));
+
+ /* Notify that we are done with external handling. It is helpful
+ when list is run in xml mode. */
+ SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
+ FALSE /*notify_external_start */,
+ TRUE /*notify_external_end */,
+ NULL, NULL, iterpool));
Again, wrong iterpool?
Index: subversion/libsvn_client/list.c
===================================================================
--- subversion/libsvn_client/list.c (revision 1405691)
+++ subversion/libsvn_client/list.c (working copy)
@@ -82,12 +94,27 @@
return SVN_NO_ERROR;
}
SVN_ERR(err);
+
+ if (prop_hash
+ && (prop_val = apr_hash_get(prop_hash, SVN_PROP_EXTERNALS,
+ APR_HASH_KEY_STRING)))
Please don't ever put assignments into 'if' statements.
Many programmers will expect == instead of = within if (...) and will
always double-check every occurance of =, because mistyping the ==
operator as = is a common C programmming error. The code you wrote
isn't wrong, it's just bad style.
I'd suggest to write this code as follows, for clarity:
if (prop_hash)
prop_val = apr_hash_get(prop_hash, SVN_PROP_EXTERNALS,
APR_HASH_KEY_STRING);
else
prop_val = NULL;
if (prop_val)
+ {
+ const char *url;
+ SVN_ERR(svn_ra_get_session_url(ra_session, &url, scratch_pool));
+
+ apr_hash_set(*externals, svn_path_url_add_component2(url, dir,
+ result_pool),
+ APR_HASH_KEY_STRING, svn_string_dup(prop_val,
+ result_pool));
+ }
You never call apr_hash_make() within this function so there is no point
in passing an apr_hash_t ** into it.
The caller creates the externals hash table. So the caller can pass the
externals hash table as apr_hash_t *, not apr_hash_t **.
You can get rid of the boolean include_externals parameter which is
implied anyway by a non-NULL externals hash passed by the caller:
externals == NULL means 'don't list externals'
externals != NULL means 'add externals to the hash if any'
@@ -224,14 +256,16 @@
return SVN_NO_ERROR;
}
+
svn_error_t *
-svn_client_list2(const char *path_or_url,
+svn_client_list3(const char *path_or_url,
const svn_opt_revision_t *peg_revision,
const svn_opt_revision_t *revision,
svn_depth_t depth,
apr_uint32_t dirent_fields,
svn_boolean_t fetch_locks,
- svn_client_list_func_t list_func,
+ svn_boolean_t include_externals,
+ svn_client_list_func2_t list_func,
void *baton,
svn_client_ctx_t *ctx,
apr_pool_t *pool)
@@ -242,6 +276,7 @@
const char *fs_path;
svn_error_t *err;
apr_hash_t *locks;
+ apr_hash_t *externals = apr_hash_make(pool);
So with my above idea, you'd initialise externals like this:
apr_hash_t *externals;
if (include_externals)
externals = apr_hash_make(pool);
else
externals = NULL;
@@ -284,14 +319,29 @@
+ /* We handle externals after listing entries under path_or_url, so that
+ handling external items (and any errors therefrom) doesn't delay
+ the primary operation. */
+ if (include_externals && externals && apr_hash_count(externals))
If include_externals is TRUE, then externals is never NULL, so you can
write the above line as:
if (include_externals && apr_hash_count(externals))
Index: subversion/svn/list-cmd.c
===================================================================
--- subversion/svn/list-cmd.c (revision 1405691)
+++ subversion/svn/list-cmd.c (working copy)
@@ -52,6 +52,10 @@
const svn_dirent_t *dirent,
const svn_lock_t *lock,
const char *abs_path,
+ svn_boolean_t notify_external_start,
+ svn_boolean_t notify_external_end,
+ const char *external_parent_url,
+ const char *external_target,
apr_pool_t *pool)
{
struct print_baton *pb = baton;
You should probably make the svn_client_list_func2_t API promise that
external_parent_url and external_target must either both be non-NULL
or both be NULL. So that the following assert won't trigger:
SVN_ERR_ASSERT((external_parent_url && external_target) ||
(external_parent_url == NULL && external_target == NULL));
@@ -59,6 +63,16 @@
static const char *time_format_long = NULL;
static const char *time_format_short = NULL;
+ if (notify_external_start)
+ {
+ return svn_cmdline_printf(pool, "Externals on '%s - %s':\n",
+ external_parent_url,
+ external_target);
+ }
+
+ if (notify_external_end)
+ return SVN_NO_ERROR;
+
It seems this is listing one external target, not many.
So the message you are printing is misleading because it says "Externals"
rather than "External", even if only one external target is defined.
You also forgot to mark the "Externals on..." message for translation
with the _() macro.
When returning directly with a function call that returns svn_error_t *,
never write 'return func()'. You should either write
'return svn_error_trace(func());'
or use 'SVN_ERR(func());' followed by 'return SVN_NO_ERROR;' on the next line.
So taking all that into account, I would suggest something like:
if (notify_external_start)
{
SVN_ERR(svn_cmdline_printf(pool,
_("Listing external '%s' defined on
'%s':\n"),
external_target,
external_parent_url));
return SVN_NO_ERROR;
}
Looks good to me otherwise, thanks!
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1406438)
+++ subversion/include/svn_client.h (working copy)
@@ -5280,7 +5280,7 @@
* @{
*/
-/** The type of function invoked by svn_client_list2() to report the details
+/** The type of function invoked by svn_client_list3() to report the details
* of each directory entry being listed.
*
* @a baton is the baton that was passed to the caller. @a path is the
@@ -5290,11 +5290,41 @@
* the entry's lock, if it is locked and if lock information is being
* reported by the caller; otherwise @a lock is NULL. @a abs_path is the
* repository path of the top node of the list operation; it is relative to
- * the repository root and begins with "/". @a pool may be used for
- * temporary allocations.
+ * the repository root and begins with "/".
*
- * @since New in 1.4.
+ * If svn_client_list3() was called with @a include_externals set to TRUE,
+ * @a notify_external_start, @a notify_external_end, @a external_parent_url
+ * and @a external_target will be set. @a notify_external_start and
+ * @a notify_external_end is used to control the list output of externals
+ * in XML mode. @a external_parent_url is url of the directory which has the
+ * externals definitions. @a external_target is the target subdirectory of
+ * externals definitions.
+
+ * @a scratch_pool may be used for temporary allocations.
+ *
+ * @since New in 1.8.
*/
+typedef svn_error_t *(*svn_client_list_func2_t)(
+ void *baton,
+ const char *path,
+ const svn_dirent_t *dirent,
+ const svn_lock_t *lock,
+ const char *abs_path,
+ svn_boolean_t notify_external_start,
+ svn_boolean_t notify_external_end,
+ const char *external_parent_url,
+ const char *external_target,
+ apr_pool_t *scratch_pool);
+
+/**
+ * Similar to #svn_client_list_func2_t, but without any information about
+ * externals definitions.
+ *
+ * @deprecated Provided for backward compatibility with the 1.7 API.
+ *
+ * @since New in 1.4
+ *
+ * */
typedef svn_error_t *(*svn_client_list_func_t)(void *baton,
const char *path,
const svn_dirent_t *dirent,
@@ -5318,6 +5348,10 @@
*
* If @a fetch_locks is TRUE, include locks when reporting directory entries.
*
+ * If @a include_externals is TRUE, also list all external items
+ * reached by recursion. @a depth value passed to the original list target
+ * applies for the externals also.
+ *
* Use @a pool for temporary allocations.
*
* Use authentication baton cached in @a ctx to authenticate against the
@@ -5334,8 +5368,30 @@
* otherwise simply bitwise OR together the combination of @c SVN_DIRENT_
* fields you care about.
*
+ * @since New in 1.8.
+ */
+svn_error_t *
+svn_client_list3(const char *path_or_url,
+ const svn_opt_revision_t *peg_revision,
+ const svn_opt_revision_t *revision,
+ svn_depth_t depth,
+ apr_uint32_t dirent_fields,
+ svn_boolean_t fetch_locks,
+ svn_boolean_t include_externals,
+ svn_client_list_func2_t list_func,
+ void *baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
+
+
+/** Similar to svn_client_list3(), but with @a include_externals set to FALSE,
+ * and using a #svn_client_list_func2_t as callback.
+ *
+ * @deprecated Provided for backwards compatibility with the 1.7 API.
+ *
* @since New in 1.5.
*/
+SVN_DEPRECATED
svn_error_t *
svn_client_list2(const char *path_or_url,
const svn_opt_revision_t *peg_revision,
Index: subversion/libsvn_client/client.h
===================================================================
--- subversion/libsvn_client/client.h (revision 1406438)
+++ subversion/libsvn_client/client.h (working copy)
@@ -1024,6 +1024,22 @@
void *status_baton,
apr_pool_t *pool);
+
+/* List external items defined on each external in EXTERNALS, a const char *
+ externals_parent_url(url of the directory which has the externals
+ definitions) of all externals mapping to the const char * externals_desc
+ (externals description text). All other options are the same as those
+ passed to svn_client_list(). */
+svn_error_t *
+svn_client__list_externals(apr_hash_t *externals,
+ svn_depth_t depth,
+ apr_uint32_t dirent_fields,
+ svn_boolean_t fetch_locks,
+ svn_client_list_func2_t list_func,
+ void *baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *scratch_pool);
+
/* Baton for svn_client__dirent_fetcher */
struct svn_client__dirent_fetcher_baton_t
{
Index: subversion/libsvn_client/deprecated.c
===================================================================
--- subversion/libsvn_client/deprecated.c (revision 1406438)
+++ subversion/libsvn_client/deprecated.c (working copy)
@@ -1269,7 +1269,77 @@
}
/*** From list.c ***/
+
+/* Baton for use with wrap_list_func */
+struct list_func_wrapper_baton {
+ void *list_func1_baton;
+ svn_client_list_func_t list_func1;
+};
+
+/* This implements svn_client_list_func2_t */
+static svn_error_t *
+list_func_wrapper(void *baton,
+ const char *path,
+ const svn_dirent_t *dirent,
+ const svn_lock_t *lock,
+ const char *abs_path,
+ svn_boolean_t notify_external_start,
+ svn_boolean_t notify_external_end,
+ const char *external_parent_url,
+ const char *external_target,
+ apr_pool_t *scratch_pool)
+{
+ struct list_func_wrapper_baton *lfwb = baton;
+
+ if (lfwb->list_func1)
+ return lfwb->list_func1(lfwb->list_func1_baton, path, dirent,
+ lock, abs_path, scratch_pool);
+
+ return SVN_NO_ERROR;
+}
+
+static void
+wrap_list_func(svn_client_list_func2_t *list_func2,
+ void **list_func2_baton,
+ svn_client_list_func_t list_func,
+ void *baton,
+ apr_pool_t *scratch_pool)
+{
+ struct list_func_wrapper_baton *lfwb = apr_palloc(scratch_pool,
+ sizeof(*lfwb));
+
+ /* Set the user provided old format callback in the baton. */
+ lfwb->list_func1_baton = baton;
+ lfwb->list_func1 = list_func;
+
+ *list_func2_baton = lfwb;
+ *list_func2 = list_func_wrapper;
+}
+
svn_error_t *
+svn_client_list2(const char *path_or_url,
+ const svn_opt_revision_t *peg_revision,
+ const svn_opt_revision_t *revision,
+ svn_depth_t depth,
+ apr_uint32_t dirent_fields,
+ svn_boolean_t fetch_locks,
+ svn_client_list_func_t list_func,
+ void *baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
+{
+ svn_client_list_func2_t list_func2;
+ void *list_func2_baton;
+
+ wrap_list_func(&list_func2, &list_func2_baton, list_func, baton, pool);
+
+ return svn_client_list3(path_or_url, peg_revision, revision, depth,
+ dirent_fields, fetch_locks,
+ FALSE /* include externals */,
+ list_func2, list_func2_baton, ctx, pool);
+}
+
+svn_error_t *
svn_client_list(const char *path_or_url,
const svn_opt_revision_t *peg_revision,
const svn_opt_revision_t *revision,
Index: subversion/libsvn_client/externals.c
===================================================================
--- subversion/libsvn_client/externals.c (revision 1406438)
+++ subversion/libsvn_client/externals.c (working copy)
@@ -1180,3 +1180,118 @@
return SVN_NO_ERROR;
}
+/* Walk through all the external items and list them. */
+static svn_error_t *
+list_external_items(apr_array_header_t *external_items,
+ const char *externals_parent_url,
+ svn_depth_t depth,
+ apr_uint32_t dirent_fields,
+ svn_boolean_t fetch_locks,
+ svn_client_list_func2_t list_func,
+ void *baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *scratch_pool)
+{
+ const char *externals_parent_repos_root_url;
+ apr_pool_t *iterpool;
+ int i;
+
+ SVN_ERR(svn_client_get_repos_root(&externals_parent_repos_root_url,
+ NULL /* uuid */,
+ externals_parent_url, ctx,
+ scratch_pool, scratch_pool));
+
+ iterpool = svn_pool_create(scratch_pool);
+
+ for (i = 0; i < external_items->nelts; i++)
+ {
+ const char *resolved_url;
+
+ svn_wc_external_item2_t *item =
+ APR_ARRAY_IDX(external_items, i, svn_wc_external_item2_t *);
+
+ svn_pool_clear(iterpool);
+
+ SVN_ERR(svn_wc__resolve_relative_external_url(
+ &resolved_url,
+ item,
+ externals_parent_repos_root_url,
+ externals_parent_url,
+ iterpool, iterpool));
+
+ /* Notify that we're about to handle an external. */
+ SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
+ TRUE /*notify_external_start */,
+ FALSE /*notify_external_end */,
+ externals_parent_url,
+ item->target_dir, iterpool));
+
+ /* List the external */
+ SVN_ERR(wrap_external_error(ctx, item->target_dir,
+ svn_client_list3(resolved_url,
+ &item->peg_revision,
+ &item->revision,
+ depth, dirent_fields,
+ fetch_locks,
+ TRUE,
+ list_func, baton, ctx,
+ iterpool),
+ iterpool));
+
+ /* Notify that we are done with external handling. It is helpful
+ when list is run in xml mode. */
+ SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
+ FALSE /*notify_external_start */,
+ TRUE /*notify_external_end */,
+ NULL, NULL, iterpool));
+ }
+ svn_pool_destroy(iterpool);
+
+ return SVN_NO_ERROR;
+}
+
+
+svn_error_t *
+svn_client__list_externals(apr_hash_t *externals,
+ svn_depth_t depth,
+ apr_uint32_t dirent_fields,
+ svn_boolean_t fetch_locks,
+ svn_client_list_func2_t list_func,
+ void *baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *scratch_pool)
+{
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
+ apr_hash_index_t *hi;
+
+ for (hi = apr_hash_first(scratch_pool, externals);
+ hi;
+ hi = apr_hash_next(hi))
+ {
+ const char *externals_parent_url = svn__apr_hash_index_key(hi);
+ svn_string_t *externals_desc = svn__apr_hash_index_val(hi);
+ apr_array_header_t *external_items;
+
+ svn_pool_clear(iterpool);
+
+ external_items = apr_array_make(iterpool, 1,
+ sizeof(svn_wc_external_item2_t*));
+
+ SVN_ERR(svn_wc_parse_externals_description3(&external_items,
+ externals_parent_url,
+ externals_desc->data,
+ FALSE, iterpool));
+
+ if (! external_items->nelts)
+ continue;
+
+ SVN_ERR(list_external_items(external_items, externals_parent_url, depth,
+ dirent_fields, fetch_locks, list_func,
+ baton, ctx, iterpool));
+
+ }
+ svn_pool_destroy(iterpool);
+
+ return SVN_NO_ERROR;
+}
+
Index: subversion/libsvn_client/list.c
===================================================================
--- subversion/libsvn_client/list.c (revision 1406438)
+++ subversion/libsvn_client/list.c (working copy)
@@ -47,7 +47,12 @@
LOCKS, if non-NULL, is a hash mapping const char * paths to svn_lock_t
objects and FS_PATH is the absolute filesystem path of the RA session.
- Use POOL for temporary allocations.
+ Use SCRATCH_POOL for temporary allocations.
+
+ If the caller passes EXTERNALS as non-NULL, populate the EXTERNALS
+ hash table whose keys are URLs of the directory which has externals
+ definitions, and whose values are the externals description text.
+ Allocate the hash's keys and values in RESULT_POOL.
*/
static svn_error_t *
get_dir_contents(apr_uint32_t dirent_fields,
@@ -58,23 +63,29 @@
const char *fs_path,
svn_depth_t depth,
svn_client_ctx_t *ctx,
- svn_client_list_func_t list_func,
+ apr_hash_t *externals,
+ svn_client_list_func2_t list_func,
void *baton,
- apr_pool_t *pool)
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
apr_hash_t *tmpdirents;
- apr_pool_t *iterpool = svn_pool_create(pool);
+ apr_pool_t *iterpool = svn_pool_create(scratch_pool);
apr_array_header_t *array;
svn_error_t *err;
+ apr_hash_t *prop_hash = NULL;
+ const svn_string_t *prop_val = NULL;
int i;
if (depth == svn_depth_empty)
return SVN_NO_ERROR;
-
- /* Get the directory's entries, but not its props. Ignore any
- not-authorized errors. */
- err = svn_ra_get_dir2(ra_session, &tmpdirents, NULL, NULL,
- dir, rev, dirent_fields, pool);
+
+ /* Get the directory's entries. If externals hash is non-NULL, get its
+ properties also. Ignore any not-authorized errors. */
+ err = svn_ra_get_dir2(ra_session, &tmpdirents, NULL,
+ externals ? &prop_hash : NULL,
+ dir, rev, dirent_fields, scratch_pool);
+
if (err && ((err->apr_err == SVN_ERR_RA_NOT_AUTHORIZED) ||
(err->apr_err == SVN_ERR_RA_DAV_FORBIDDEN)))
{
@@ -82,12 +93,29 @@
return SVN_NO_ERROR;
}
SVN_ERR(err);
+
+ /* Filter out svn:externals from all properties hash. */
+ if (prop_hash)
+ prop_val = apr_hash_get(prop_hash, SVN_PROP_EXTERNALS,
+ APR_HASH_KEY_STRING);
+ if (prop_val)
+ {
+ const char *url;
+ SVN_ERR(svn_ra_get_session_url(ra_session, &url, scratch_pool));
+
+ apr_hash_set(externals, svn_path_url_add_component2(url, dir,
+ result_pool),
+ APR_HASH_KEY_STRING, svn_string_dup(prop_val,
+ result_pool));
+ }
+
if (ctx->cancel_func)
SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
/* Sort the hash, so we can call the callback in a "deterministic" order. */
- array = svn_sort__hash(tmpdirents, svn_sort_compare_items_lexically, pool);
+ array = svn_sort__hash(tmpdirents, svn_sort_compare_items_lexically,
+ scratch_pool);
for (i = 0; i < array->nelts; ++i)
{
svn_sort__item_t *item = &APR_ARRAY_IDX(array, i, svn_sort__item_t);
@@ -110,12 +138,16 @@
if (the_ent->kind == svn_node_file
|| depth == svn_depth_immediates
|| depth == svn_depth_infinity)
- SVN_ERR(list_func(baton, path, the_ent, lock, fs_path, iterpool));
-
+ SVN_ERR(list_func(baton, path, the_ent, lock, fs_path, FALSE, FALSE,
+ NULL, NULL, iterpool));
+
+ /* If externals is non-NULL, populate the externals hash table
+ recursively for all directory entries. */
if (depth == svn_depth_infinity && the_ent->kind == svn_node_dir)
SVN_ERR(get_dir_contents(dirent_fields, path, rev,
- ra_session, locks, fs_path, depth, ctx,
- list_func, baton, iterpool));
+ ra_session, locks, fs_path, depth, ctx,
+ externals, list_func, baton,
+ result_pool, iterpool));
}
svn_pool_destroy(iterpool);
@@ -224,14 +256,16 @@
return SVN_NO_ERROR;
}
+
svn_error_t *
-svn_client_list2(const char *path_or_url,
+svn_client_list3(const char *path_or_url,
const svn_opt_revision_t *peg_revision,
const svn_opt_revision_t *revision,
svn_depth_t depth,
apr_uint32_t dirent_fields,
svn_boolean_t fetch_locks,
- svn_client_list_func_t list_func,
+ svn_boolean_t include_externals,
+ svn_client_list_func2_t list_func,
void *baton,
svn_client_ctx_t *ctx,
apr_pool_t *pool)
@@ -242,7 +276,13 @@
const char *fs_path;
svn_error_t *err;
apr_hash_t *locks;
+ apr_hash_t *externals;
+ if (include_externals)
+ externals = apr_hash_make(pool);
+ else
+ externals = NULL;
+
/* We use the kind field to determine if we should recurse, so we
always need it. */
dirent_fields |= SVN_DIRENT_KIND;
@@ -284,14 +324,27 @@
SVN_ERR(list_func(baton, "", dirent, locks
? (apr_hash_get(locks, fs_path,
APR_HASH_KEY_STRING))
- : NULL, fs_path, pool));
+ : NULL, fs_path, FALSE, FALSE, NULL, NULL, pool));
if (dirent->kind == svn_node_dir
&& (depth == svn_depth_files
|| depth == svn_depth_immediates
|| depth == svn_depth_infinity))
SVN_ERR(get_dir_contents(dirent_fields, "", loc->rev, ra_session, locks,
- fs_path, depth, ctx, list_func, baton, pool));
-
+ fs_path, depth, ctx, externals, list_func,
+ baton, pool, pool));
+
+ /* We handle externals after listing entries under path_or_url, so that
+ handling external items (and any errors therefrom) doesn't delay
+ the primary operation. */
+ if (include_externals && apr_hash_count(externals))
+ {
+ /* The 'externals' hash populated by get_dir_contents() is processed
+ here. */
+ SVN_ERR(svn_client__list_externals(externals, depth, dirent_fields,
+ fetch_locks, list_func, baton,
+ ctx, pool));
+ }
+
return SVN_NO_ERROR;
}
Index: subversion/svn/list-cmd.c
===================================================================
--- subversion/svn/list-cmd.c (revision 1406438)
+++ subversion/svn/list-cmd.c (working copy)
@@ -44,7 +44,7 @@
svn_client_ctx_t *ctx;
};
-/* This implements the svn_client_list_func_t API, printing a single
+/* This implements the svn_client_list_func2_t API, printing a single
directory entry in text format. */
static svn_error_t *
print_dirent(void *baton,
@@ -52,13 +52,32 @@
const svn_dirent_t *dirent,
const svn_lock_t *lock,
const char *abs_path,
- apr_pool_t *pool)
+ svn_boolean_t notify_external_start,
+ svn_boolean_t notify_external_end,
+ const char *external_parent_url,
+ const char *external_target,
+ apr_pool_t *scratch_pool)
{
struct print_baton *pb = baton;
const char *entryname;
static const char *time_format_long = NULL;
static const char *time_format_short = NULL;
+ SVN_ERR_ASSERT((external_parent_url == NULL && external_target == NULL) ||
+ (external_parent_url && external_target));
+
+ if (notify_external_start)
+ {
+ SVN_ERR(svn_cmdline_printf(scratch_pool,
+ _("Listing external '%s' defined on
'%s':\n"),
+ external_target,
+ external_parent_url));
+ return SVN_NO_ERROR;
+ }
+
+ if (notify_external_end)
+ return SVN_NO_ERROR;
+
if (time_format_long == NULL)
time_format_long = _("%b %d %H:%M");
if (time_format_short == NULL)
@@ -70,7 +89,7 @@
if (strcmp(path, "") == 0)
{
if (dirent->kind == svn_node_file)
- entryname = svn_dirent_basename(abs_path, pool);
+ entryname = svn_dirent_basename(abs_path, scratch_pool);
else if (pb->verbose)
entryname = ".";
else
@@ -110,12 +129,13 @@
timestr[0] = '\0';
/* we need it in UTF-8. */
- SVN_ERR(svn_utf_cstring_to_utf8(&utf8_timestr, timestr, pool));
+ SVN_ERR(svn_utf_cstring_to_utf8(&utf8_timestr, timestr, scratch_pool));
- sizestr = apr_psprintf(pool, "%" SVN_FILESIZE_T_FMT, dirent->size);
+ sizestr = apr_psprintf(scratch_pool, "%" SVN_FILESIZE_T_FMT,
+ dirent->size);
return svn_cmdline_printf
- (pool, "%7ld %-8.8s %c %10s %12s %s%s\n",
+ (scratch_pool, "%7ld %-8.8s %c %10s %12s %s%s\n",
dirent->created_rev,
dirent->last_author ? dirent->last_author : " ? ",
lock ? 'O' : ' ',
@@ -126,14 +146,14 @@
}
else
{
- return svn_cmdline_printf(pool, "%s%s\n", entryname,
+ return svn_cmdline_printf(scratch_pool, "%s%s\n", entryname,
(dirent->kind == svn_node_dir)
? "/" : "");
}
}
-/* This implements the svn_client_list_func_t API, printing a single dirent
+/* This implements the svn_client_list_func2_t API, printing a single dirent
in XML format. */
static svn_error_t *
print_dirent_xml(void *baton,
@@ -141,16 +161,40 @@
const svn_dirent_t *dirent,
const svn_lock_t *lock,
const char *abs_path,
- apr_pool_t *pool)
+ svn_boolean_t notify_external_start,
+ svn_boolean_t notify_external_end,
+ const char *external_parent_url,
+ const char *external_target,
+ apr_pool_t *scratch_pool)
{
struct print_baton *pb = baton;
const char *entryname;
- svn_stringbuf_t *sb;
+ svn_stringbuf_t *sb = svn_stringbuf_create_empty(scratch_pool);
+
+ SVN_ERR_ASSERT((external_parent_url == NULL && external_target == NULL) ||
+ (external_parent_url && external_target));
+ if (notify_external_start)
+ {
+ svn_xml_make_open_tag(&sb, scratch_pool, svn_xml_normal, "external",
+ "parent_url", external_parent_url,
+ "target", external_target,
+ NULL);
+
+ return svn_error_trace(svn_cl__error_checked_fputs(sb->data, stdout));
+ }
+
+ if (notify_external_end)
+ {
+ svn_xml_make_close_tag(&sb, scratch_pool, "external");
+
+ return svn_error_trace(svn_cl__error_checked_fputs(sb->data, stdout));
+ }
+
if (strcmp(path, "") == 0)
{
if (dirent->kind == svn_node_file)
- entryname = svn_dirent_basename(abs_path, pool);
+ entryname = svn_dirent_basename(abs_path, scratch_pool);
else if (pb->verbose)
entryname = ".";
else
@@ -163,48 +207,46 @@
if (pb->ctx->cancel_func)
SVN_ERR(pb->ctx->cancel_func(pb->ctx->cancel_baton));
- sb = svn_stringbuf_create_empty(pool);
-
- svn_xml_make_open_tag(&sb, pool, svn_xml_normal, "entry",
+ svn_xml_make_open_tag(&sb, scratch_pool, svn_xml_normal, "entry",
"kind", svn_cl__node_kind_str_xml(dirent->kind),
NULL);
- svn_cl__xml_tagged_cdata(&sb, pool, "name", entryname);
+ svn_cl__xml_tagged_cdata(&sb, scratch_pool, "name", entryname);
if (dirent->kind == svn_node_file)
{
svn_cl__xml_tagged_cdata
- (&sb, pool, "size",
- apr_psprintf(pool, "%" SVN_FILESIZE_T_FMT, dirent->size));
+ (&sb, scratch_pool, "size",
+ apr_psprintf(scratch_pool, "%" SVN_FILESIZE_T_FMT, dirent->size));
}
- svn_xml_make_open_tag(&sb, pool, svn_xml_normal, "commit",
+ svn_xml_make_open_tag(&sb, scratch_pool, svn_xml_normal, "commit",
"revision",
- apr_psprintf(pool, "%ld", dirent->created_rev),
+ apr_psprintf(scratch_pool, "%ld", dirent->created_rev),
NULL);
- svn_cl__xml_tagged_cdata(&sb, pool, "author", dirent->last_author);
+ svn_cl__xml_tagged_cdata(&sb, scratch_pool, "author", dirent->last_author);
if (dirent->time)
- svn_cl__xml_tagged_cdata(&sb, pool, "date",
- svn_time_to_cstring(dirent->time, pool));
- svn_xml_make_close_tag(&sb, pool, "commit");
+ svn_cl__xml_tagged_cdata(&sb, scratch_pool, "date",
+ svn_time_to_cstring(dirent->time, scratch_pool));
+ svn_xml_make_close_tag(&sb, scratch_pool, "commit");
if (lock)
{
- svn_xml_make_open_tag(&sb, pool, svn_xml_normal, "lock", NULL);
- svn_cl__xml_tagged_cdata(&sb, pool, "token", lock->token);
- svn_cl__xml_tagged_cdata(&sb, pool, "owner", lock->owner);
- svn_cl__xml_tagged_cdata(&sb, pool, "comment", lock->comment);
- svn_cl__xml_tagged_cdata(&sb, pool, "created",
+ svn_xml_make_open_tag(&sb, scratch_pool, svn_xml_normal, "lock", NULL);
+ svn_cl__xml_tagged_cdata(&sb, scratch_pool, "token", lock->token);
+ svn_cl__xml_tagged_cdata(&sb, scratch_pool, "owner", lock->owner);
+ svn_cl__xml_tagged_cdata(&sb, scratch_pool, "comment", lock->comment);
+ svn_cl__xml_tagged_cdata(&sb, scratch_pool, "created",
svn_time_to_cstring(lock->creation_date,
- pool));
+ scratch_pool));
if (lock->expiration_date != 0)
- svn_cl__xml_tagged_cdata(&sb, pool, "expires",
+ svn_cl__xml_tagged_cdata(&sb, scratch_pool, "expires",
svn_time_to_cstring
- (lock->expiration_date, pool));
- svn_xml_make_close_tag(&sb, pool, "lock");
+ (lock->expiration_date, scratch_pool));
+ svn_xml_make_close_tag(&sb, scratch_pool, "lock");
}
- svn_xml_make_close_tag(&sb, pool, "entry");
+ svn_xml_make_close_tag(&sb, scratch_pool, "entry");
return svn_cl__error_checked_fputs(sb->data, stdout);
}
@@ -225,6 +267,8 @@
struct print_baton pb;
svn_boolean_t seen_nonexistent_target = FALSE;
svn_error_t *err;
+ svn_error_t *externals_err = SVN_NO_ERROR;
+ struct svn_cl__check_externals_failed_notify_baton nwb;
SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
opt_state->targets,
@@ -266,6 +310,15 @@
if (opt_state->depth == svn_depth_unknown)
opt_state->depth = svn_depth_immediates;
+ if (opt_state->include_externals)
+ {
+ nwb.wrapped_func = ctx->notify_func2;
+ nwb.wrapped_baton = ctx->notify_baton2;
+ nwb.had_externals_error = FALSE;
+ ctx->notify_func2 = svn_cl__check_externals_failed_notify_wrapper;
+ ctx->notify_baton2 = &nwb;
+ }
+
/* For each target, try to list it. */
for (i = 0; i < targets->nelts; i++)
{
@@ -290,11 +343,12 @@
SVN_ERR(svn_cl__error_checked_fputs(sb->data, stdout));
}
- err = svn_client_list2(truepath, &peg_revision,
+ err = svn_client_list3(truepath, &peg_revision,
&(opt_state->start_revision),
opt_state->depth,
dirent_fields,
(opt_state->xml || opt_state->verbose),
+ opt_state->include_externals,
opt_state->xml ? print_dirent_xml : print_dirent,
&pb, ctx, subpool);
@@ -322,14 +376,24 @@
}
svn_pool_destroy(subpool);
+
+ if (opt_state->include_externals && nwb.had_externals_error)
+ {
+ externals_err = svn_error_create(SVN_ERR_CL_ERROR_PROCESSING_EXTERNALS,
+ NULL,
+ _("Failure occurred processing one or "
+ "more externals definitions"));
+ }
if (opt_state->xml && ! opt_state->incremental)
SVN_ERR(svn_cl__xml_print_footer("lists", pool));
if (seen_nonexistent_target)
- return svn_error_create(
- SVN_ERR_ILLEGAL_TARGET, NULL,
- _("Could not list all targets because some targets don't exist"));
+ {
+ err = svn_error_create(SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Could not list all targets because some targets don't exist"));
+ return svn_error_compose_create(externals_err, err);
+ }
else
- return SVN_NO_ERROR;
+ return svn_error_compose_create(externals_err, err);
}
Index: subversion/svn/main.c
===================================================================
--- subversion/svn/main.c (revision 1406438)
+++ subversion/svn/main.c (working copy)
@@ -356,11 +356,7 @@
" "
"Please run 'svn update' instead.")},
{"include-externals", opt_include_externals, 0,
- N_("Also commit file and dir externals reached by\n"
- " "
- "recursion. This does not include externals with a\n"
- " "
- "fixed revision. (See the svn:externals property)")},
+ N_("include externals definitions")},
{"show-inherited-props", opt_show_inherited_props, 0,
N_("retrieve target's inherited properties")},
{"search", opt_search, 1,
@@ -622,7 +618,8 @@
" If locked, the letter 'O'. (Use 'svn info URL' to see details)\n"
" Size (in bytes)\n"
" Date and time of the last commit\n"),
- {'r', 'v', 'R', opt_depth, opt_incremental, opt_xml} },
+ {'r', 'v', 'R', opt_depth, opt_incremental, opt_xml,
+ opt_include_externals} },
{ "lock", svn_cl__lock, {0}, N_
("Lock working copy paths or URLs in the repository, so that\n"
Index: subversion/tests/cmdline/externals_tests.py
===================================================================
--- subversion/tests/cmdline/externals_tests.py (revision 1406438)
+++ subversion/tests/cmdline/externals_tests.py (working copy)
@@ -2131,7 +2131,7 @@
actions.run_and_verify_update(wc_dir, expected_output, expected_disk,
expected_status, None, None, None, None, None, True, wc_dir)
-def include_externals(sbox):
+def commit_include_externals(sbox):
"commit --include-externals"
# svntest.factory.make(sbox, """
# mkdir Z
@@ -2878,6 +2878,60 @@
"OUTPUT", expected_stdout, [], 0, 'copy', repo_url + '/A/C',
sbox.ospath('External-WC-to-URL-Copy'))
+@Issue(4225)
+def list_include_externals(sbox):
+ "list with --include-externals"
+
+ externals_test_setup(sbox)
+
+ wc_dir = sbox.wc_dir
+ repo_url = sbox.repo_url
+
+ svntest.actions.run_and_verify_svn(None, None, [],
+ 'checkout',
+ repo_url, wc_dir)
+
+ B_path = sbox.ospath("A/B")
+ C_path = sbox.ospath("A/C")
+
+ B_url = repo_url + "/A/B"
+ C_url = repo_url + "/A/C"
+
+ def list_external_string(path, url):
+ string = "Listing external" + " '" + path + "' " + "defined on" + " '" + \
+ url + "'" + ":"
+ return string
+
+ expected_stdout = verify.UnorderedOutput([
+ "E/" + "\n",
+ "F/" + "\n",
+ "lambda" + "\n",
+ list_external_string("gamma", B_url ) + "\n",
+ "gamma" + "\n"])
+
+ exit_code, stdout, stderr = svntest.actions.run_and_verify_svn2(
+ "OUTPUT", expected_stdout, [], 0, 'ls', '--include-externals', B_path)
+
+ exit_code, stdout, stderr = svntest.actions.run_and_verify_svn2(
+ "OUTPUT", expected_stdout, [], 0, 'ls', '--include-externals', B_url)
+
+ expected_stdout = verify.UnorderedOutput([
+ list_external_string("exdir_G", C_url)+ "\n",
+ "pi" + "\n",
+ "rho" + "\n",
+ "tau" + "\n",
+ list_external_string("exdir_H", C_url) + "\n",
+ "chi" + "\n",
+ "omega" + "\n",
+ "psi" + "\n"])
+
+ exit_code, stdout, stderr = svntest.actions.run_and_verify_svn2(
+ "OUTPUT", expected_stdout, [], 0, 'ls', '--include-externals', C_path)
+
+ exit_code, stdout, stderr = svntest.actions.run_and_verify_svn2(
+ "OUTPUT", expected_stdout, [], 0, 'ls', '--include-externals', C_url)
+
+
########################################################################
# Run the tests
@@ -2919,12 +2973,13 @@
file_externals_different_url,
file_external_in_unversioned,
copy_file_externals,
- include_externals,
+ commit_include_externals,
include_immediate_dir_externals,
shadowing,
remap_file_external_with_prop_del,
dir_external_with_dash_r_only,
url_to_wc_copy_of_externals,
+ list_include_externals,
]
if __name__ == '__main__':
Fix issue #4225, "Add '--include-externals' option to svn list".
* subversion/include/svn_client.h
(svn_client_list_func2_t): New type used to notify externals information.
(svn_client_list_func_t): Deprecate type.
(svn_client_list3): New function, which has a new argument
include_externals.
(svn_client_list2): Deprecate it.
* subversion/libsvn_client/client.h
(svn_client__list_externals): New function.
* subversion/libsvn_client/deprecated.c
(list_func_wrapper_baton): New struct to deprecate svn_client_list2().
(list_func_wrapper, wrap_list_func): Helper functions to deprecate
svn_client_list2().
(svn_client_list2): Call svn_client_list3 with include_externals set to
FALSE, and use svn_client_list_func2_t as callback implemented by
list_func_wrapper().
* subversion/libsvn_client/externals.c
(list_external_items): New static function that walks through all the
externals under list target recursively and list them using
svn_client_list3().
(svn_client__list_externals): New function to parse externals description
and list it using list_external_items().
* subversion/libsvn_client/list.c
(get_dir_contents): Populate the hash table 'externals'. Use
svn_client_list_func2_t instead of svn_client_list_func_t to report file
and directory entries.
(svn_client_list3): New function. If include_externals is set, process all
the
externals which are populated by get_dir_contents() using
svn_client__list_externals().
* subversion/svn/list-cmd.c
(print_dirent): Implement svn_client_list_func2_t to control the output when
used with externals.
(print_dirent_xml): Implement svn_client_list_func2_t. Enclose the external
items in the element <external parent_url=.. target= ..><..></external>
(svn_cl__list): Call svn_client_list3(). Handle if there are any errors
during externals processing.
* subversion/svn/main.c
(svn_cl__options): Short description about include_externals.
(svn_cl__cmd_table): Enable include_externals for 'list'.
* subversion/tests/cmdline/externals_tests.py
(include_externals): Rename it to 'commit_include_externals'.
(list_include_externals): New test.
(test_list): Add a reference to the new test. Rename 'include_externals' to
'commit_include_externals'.
Patch by: Vijayaguru G <vijay{_AT_}collab.net>
