Re: [PATCH] Implement '--include-externals' option to 'svn list'

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!
>