Re: svn commit: r14849 - in trunk/subversion: tests/clients/cmdline

["C. Michael Pilato" <cm...@collab.net>]

fitz@tigris.org writes:

> Modified: trunk/subversion/libsvn_client/locking_commands.c
> ==============================================================================
> --- trunk/subversion/libsvn_client/locking_commands.c	(original)
> +++ trunk/subversion/libsvn_client/locking_commands.c	Thu May 26 11:58:53 2005

[...]

> @@ -63,9 +64,10 @@
>    svn_wc_adm_access_t *adm_access;
>    const char *abs_path;
>    svn_wc_notify_t *notify;
> +  char *path = apr_hash_get(lb->urls_to_paths, rel_url, APR_HASH_KEY_STRING);

space-before-paren.

> @@ -200,10 +210,15 @@
>                          do_lock ? (const void *) invalid_revnum
>                                  : (const void *) "");
>          }
> +      *rel_urls_p = NULL;
>      }

(I'm only looking at the patch, not the context.)  Any reason not to
just initialize this at the top of the function?

>    else  /* common parent is a local path */
>      {
>        int max_depth = 0;
> +      apr_array_header_t *rel_urls;
> +      apr_array_header_t *urls = apr_array_make(pool, 1, sizeof(const char *));
> +      apr_hash_t *urls_hash = apr_hash_make(pool);
> +      const char *common_url;
>  
>        /* Calculate the maximum number of components in the rel_targets, which
>           is the depth to which we need to lock the WC. */

More space-before-paren stuff here.

> @@ -255,14 +269,71 @@
>              return svn_error_createf (SVN_ERR_ENTRY_MISSING_URL, NULL,
>                                        _("'%s' has no URL"),
>                                        svn_path_local_style (target, pool));
> -          
> +
> +          (*((const char **)(apr_array_push (urls)))) = apr_pstrdup 
> +            (pool, entry->url);
> +
> +
> +        }
> +
> +      /* Condense our absolute urls and get the relative urls. */
> +      SVN_ERR (svn_path_condense_targets (&common_url, &rel_urls, urls, 
> +                                          FALSE, pool));
> +
> +      /* svn_path_condense_targets leaves paths empty if TARGETS only had
> +         1 member, so we special case that (again). */
> +      if (apr_is_empty_array (rel_urls))
> +        {
> +          char *base_name = svn_path_basename (common_url, pool);
> +          common_url = svn_path_dirname (common_url, pool);
> +          APR_ARRAY_PUSH(rel_urls, char *) = base_name;
> +        }
> +      
> +      /* If we have no common URL parent, bail (cross-repos lock attempt) */
> +      if (common_url == NULL || (common_url)[0] == '\0')

This may just be a personal style thing, but I prefer:

        if (! (common_url && *common_url))

> +        return svn_error_create
> +          (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> +           _("Unable to lock/unlock across multiple repositories"));
> +
> +      if (rel_urls->nelts != rel_targets->nelts)
> +        return svn_error_create
> +          (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> +           _("Error condensing Lock/unlock targets."));

Stray capitalization of "Lock" here.  Also, why does this error get an
SVN_ERR_UNSUPPORTED_FEATURE?

> +      for (i = 0; i < rel_urls->nelts; i++)
> +        {
> +          const char *target = ((const char **) (rel_targets->elts))[i];
> +          const char *rel_url = ((const char **) (rel_urls->elts))[i];

Might want to use these macros:

   APR_ARRAY_IDX(rel_targets, i, const char *);
   APR_ARRAY_IDX(rel_urls, i, const char *);

> +      for (i = 0; i < rel_targets->nelts; i++)
> +        {
> +          const svn_wc_entry_t *entry;
> +          const char *target = ((const char **) (rel_targets->elts))[i];
> +          const char *url = ((const char **) (rel_urls->elts))[i];

Here, too (macros).


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org