[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Mon, 26 Nov 2012 21:49:35 +0000 (GMT)

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
Received on 2012-11-26 22:50:11 CET

This is an archived mail posted to the Subversion Dev mailing list.