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!
Received on 2012-11-06 15:02:41 CET