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

Thanks Julian.

This updated patch has all the changes.

I have removed some redundant portion of code in print_dirent() and
print_dirent_xml() in svn/list-cmd.c.

Thanks & Regards,
Vijayaguru

On Tuesday 27 November 2012 03:19 AM, Julian Foad wrote:
> vijay <vijay_at_collab.net> wrote:
>
>> On Tuesday 13 November 2012 03:02 AM, Stefan Sperling wrote:
>> The two extra list_func() calls has gone away!
>>
>> Attaching the updated patch that addresses your review comments.
>
> Hi Vijay. I read through the patch and it looks good, I just have a few minor comments:
>
>> Index: subversion/include/svn_client.h
>> ===================================================================
>> - * @since New in 1.4.
>> + * If svn_client_list3() was called with @a include_externals set to TRUE,
>> + * @a external_parent_url and @a external_target will be set.
>> + * @a external_parent_url is url of the directory which has the
>> + * externals definitions. @a external_target is the target subdirectory of
>> + * externals definitions.
>
> Is @a external_target an abspath, or a 'relpath' relative to @a path, or relative to the root of the entire 'list' operation, or what?
>
>> + *
>> + * If external_parent_url and external_target are defined, the item being
>> + * listed is part of the external described by external_parent_url and
>> + * external_target. Else, the item is not part of any external.
>> + * Moreover, we will never mix items which are part of separate
>> + * externals, and will always finish listing an external before listing
>> + * the next one.
>> +
>> + * @a pool may be used for temporary allocations.
>> + *
>> + * @since New in 1.8.
>> */
>> +typedef svn_error_t *(*svn_client_list_func2_t)(
>
>> Index: subversion/libsvn_client/deprecated.c
>> ===================================================================
>> +
>> +static void
>
> Please put a doc string on each new function. It can be brief for a simple function such as this.
>
>> +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));
>
> This is allocating memory that will be returned as the result of this function, so the pool shouldn't be 'scratch_pool' but 'result_pool'.
>
>> +
>> + /* 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;
>> +}
>
>> Index: subversion/libsvn_client/client.h
>> ===================================================================
>> +/* 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
>
> The implementation treats the hash values as 'svn_string_t *' not 'const char *'.
>
>> + (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);
>
>> Index: subversion/libsvn_client/externals.c
>> ===================================================================
>> +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*));
>
> There's no need to initialize 'external_items', as the first parameter of svn_wc_parse_externals_description3() is a pure output parameter. That is, the function creates a new array.
>
>> + 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/svn/main.c
>> ===================================================================
>> {"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")},
>
> That change loses information that was previously shown for the 'commit' command. We have a way to display a different description text for the same option for a specific subcommand. It's done by adding a bit at the end of the subcommand definition -- see the lines starting with '{{' in main.c.
>
> - Julian
>