Re: [PATCH] Issue #1776 - Create parent directories on url with 'svn mkdir'

"S.Ramaswamy" <ramaswamy@collab.net> writes:

> --- subversion/libsvn_client/add.c (revision 13284)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -37,6 +37,7 @@
> #include "client.h"
>
> #include "svn_private_config.h"
> +#include "svn_sorts.h"
>
>
> 
> @@ -483,10 +484,64 @@
> SVN_INVALID_REVNUM, pool, dir_baton);
> }
>
> +/* Set 'existing_url' to the portion of 'url' that exists, and 'new_path'
> + to the non-existent portion of the 'url'*/
> +static svn_error_t *
> +svn_client__url_paths(const char **existing_url,
> + const char **new_path,
> + const char *url,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_ra_session_t *ra_session;
> + svn_node_kind_t kind;
> + const char *dir;
> + const char *base;
> + const char *pcommon;
> + apr_array_header_t *new_paths;
> + apr_array_header_t *temp_paths = apr_array_make(pool,2,
> + sizeof(const char*));
>
> + APR_ARRAY_PUSH (temp_paths, const char *) = url;
> +
> + do
> + {
> + SVN_ERR(svn_client__open_ra_session (&ra_session, url, NULL,
> + NULL, NULL, FALSE, TRUE,
> + ctx, pool));
> +
> + SVN_ERR(svn_ra_check_path (ra_session, "", SVN_INVALID_REVNUM,
> + &kind, pool));
> + if (kind == svn_node_none)
> + {
> + svn_path_split (url, &dir, &base, pool);
> + url = dir;
> + }
> + } while (kind == svn_node_none);

Your code looks very different from the code in svn_client_import but
does something similar. Perhaps you could factor the import code into
a function and call that?

If not then your code should use a subpool, call the cancellation
function, etc. Does it really need to open multiple sessions?

> +
> + if (kind == svn_node_dir)
> + {
> + *existing_url = (const char *)url;

Why the cast?

> + if (url == APR_ARRAY_IDX (temp_paths, 0, const char*))
> + {
> + *new_path = NULL;
> + }
> + else
> + {
> + APR_ARRAY_PUSH (temp_paths, const char *) = url;
> + SVN_ERR (svn_path_condense_targets (&pcommon, &new_paths,
> + temp_paths,FALSE,pool));
> + *new_path = APR_ARRAY_IDX (new_paths, 0, const char *);
> + }

That looks like overkill, there are only two paths, you know what they
are, is svn_path_condense_targets necessary? Are the apr_array_header_t
necessary?

> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *
> mkdir_urls (svn_client_commit_info_t **commit_info,
> const apr_array_header_t *paths,
> + svn_boolean_t make_parents,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> @@ -499,41 +554,143 @@
> svn_error_t *err;
> const char *common;
> int i;
> + const char *target_prefix;
> + apr_array_header_t *valid_urls = apr_array_make(pool,
> + paths->nelts,
> + sizeof(const char *));
> + apr_array_header_t *prefix;
> + apr_array_header_t *new_paths = apr_array_make(pool,
> + paths->nelts,
> + sizeof(const char*));
> + int j;
> + const char *target_path;
> + apr_hash_t *target_hash;
> + apr_hash_index_t *hi;
> + int nelts;
> + apr_pool_t *subpool;
> + apr_pool_t *iterpool;
>
> - /* Condense our list of mkdir targets. */
> - SVN_ERR (svn_path_condense_targets (&common, &targets, paths, FALSE, pool));
> - if (! targets->nelts)
> + if (make_parents)
> {
> - const char *bname;
> - svn_path_split (common, &common, &bname, pool);
> - APR_ARRAY_PUSH (targets, const char *) = bname;
> - }
> - else
> - {
> - svn_boolean_t resplit = FALSE;
> + const char *existing_url;
> + const char *new_path;
> +
> + target_hash = apr_hash_make(pool);
> + subpool = svn_pool_create(pool);

Whitespace: this file uses "function (" not "function(". This applies
to lots of your code.

>
> - /* We can't "mkdir" the root of an editor drive, so if one of
> - our targets is the empty string, we need to back everything
> - up by a path component. */
> - for (i = 0; i < targets->nelts; i++)
> + for(i=0; i < paths->nelts; i++)

Look at the different whitespace, please be consistent with the
existing code.

> {
> - const char *path = APR_ARRAY_IDX (targets, i, const char *);
> - if (! *path)
> + const char *path = APR_ARRAY_IDX (paths, i, const char*);
> +
> + svn_pool_clear (subpool);
> + SVN_ERR (svn_client__url_paths (&existing_url,
> + &new_path,
> + path,
> + ctx,
> + subpool));
> +
> + if (new_path == NULL)
> {
> - resplit = TRUE;
> - break;
> + return svn_error_createf
> + (SVN_ERR_FS_ALREADY_EXISTS, NULL,
> + _("URL '%s' already exists"), path);
> }
> + APR_ARRAY_PUSH (valid_urls, const char *) =
> + apr_pstrdup (pool, existing_url);
> + APR_ARRAY_PUSH (new_paths, const char*) =
> + apr_pstrdup (pool, new_path);
> }
> - if (resplit)
> + svn_pool_destroy (subpool);
> + /* Find common url from list of valid urls; prefix is the array of
> + paths to be prefixed to targets
> + */
> + SVN_ERR(svn_path_condense_targets (&common, &prefix, valid_urls, FALSE,
> + pool));
> + /* From list of paths to be created, get the targets */
> + iterpool = svn_pool_create(pool);
> + for(j = 0; j < new_paths->nelts; j++)
> {
> + apr_array_header_t *path_pieces;
> +
> + svn_pool_clear(iterpool);
> + path_pieces = svn_path_decompose (APR_ARRAY_IDX(new_paths,
> + j, const char*),
> + iterpool);
> + if (prefix->nelts)
> + {
> + target_prefix = APR_ARRAY_IDX (prefix, j, const char *);
> + }
> + else if ((!prefix->nelts)|| (svn_path_is_empty (APR_ARRAY_IDX
> + (prefix, j, const char *))))
> + {
> + target_prefix = "";
> + }
> +
> + for(i=0; i < path_pieces->nelts; i++)
> + {
> + target_path = svn_path_join (target_prefix, APR_ARRAY_IDX(
> + path_pieces, i, const char *),
> + pool);
> + apr_hash_set (target_hash, target_path, APR_HASH_KEY_STRING,
> + (const char*)common);
> + target_prefix = target_path;
> + }
> + }
> + svn_pool_destroy(iterpool);
> +
> + nelts = apr_hash_count (target_hash);
> + targets = apr_array_make (pool, nelts, sizeof(const char *));
> + for(hi = apr_hash_first (pool, target_hash); hi != NULL;
> + hi = apr_hash_next (hi))
> + {
> + const void *pname;
> + void *pval;
> +
> + apr_hash_this (hi, &pname, NULL, &pval);
> + APR_ARRAY_PUSH (targets, const char *) = pname;
> + }
> +
> + qsort (targets->elts, targets->nelts,
> + targets->elt_size, svn_sort_compare_paths);
> + }
> + else if (!make_parents)
> + {
> + /* Condense our list of mkdir targets. */
> + SVN_ERR (svn_path_condense_targets (&common, &targets, paths,
> + FALSE, pool));
> + if (! targets->nelts)
> + {
> const char *bname;
> svn_path_split (common, &common, &bname, pool);
> + APR_ARRAY_PUSH (targets, const char *) = bname;
> + }
> + else
> + {
> + svn_boolean_t resplit = FALSE;
> +
> + /* We can't "mkdir" the root of an editor drive, so if one of
> + our targets is the empty string, we need to back everything
> + up by a path component. */
> for (i = 0; i < targets->nelts; i++)
> {
> const char *path = APR_ARRAY_IDX (targets, i, const char *);
> - path = svn_path_join (bname, path, pool);
> - APR_ARRAY_IDX (targets, i, const char *) = path;
> + if (! *path)
> + {
> + resplit = TRUE;
> + break;
> + }
> }
> + if (resplit)
> + {
> + const char *bname;
> + svn_path_split (common, &common, &bname, pool);
> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *path = APR_ARRAY_IDX (targets, i, const char *);
> + path = svn_path_join (bname, path, pool);
> + APR_ARRAY_IDX (targets, i, const char *) = path;
> + }
> + }
> }

This function looks like it's getting very long, perhaps it should be
broken up?

> }
>
> @@ -598,19 +755,28 @@
> return SVN_NO_ERROR;
> }
>
> -
> svn_error_t *
> svn_client_mkdir (svn_client_commit_info_t **commit_info,
> const apr_array_header_t *paths,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> + return svn_client_mkdir2(commit_info, paths, FALSE, ctx, pool);
> +}
> +
> +svn_error_t *
> +svn_client_mkdir2 (svn_client_commit_info_t **commit_info,
> + const apr_array_header_t *paths,
> + svn_boolean_t make_parents,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> if (! paths->nelts)
> return SVN_NO_ERROR;
>
> if (svn_path_is_url (APR_ARRAY_IDX (paths, 0, const char *)))
> {
> - SVN_ERR (mkdir_urls (commit_info, paths, ctx, pool));
> + SVN_ERR (mkdir_urls (commit_info, paths, make_parents, ctx, pool));
> }
> else
> {
> @@ -648,3 +814,4 @@
>
> return SVN_NO_ERROR;
> }
> +

You don't appear to have included changes to svn_client.h, or the
command line client. I guess you intend your patch to apply on top of
the one in the issue, but it's more difficult to review if you do it
like that.

Could you add a test to the testsuite?

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