[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.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.