vijay <vi...@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
