You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@mit.edu> on 2007/05/17 14:43:30 UTC
Re: svn commit: r25049 - in trunk/subversion: include libsvn_client svn tests/cmdline
On 5/16/07, hwright@tigris.org <hw...@tigris.org> wrote:
Very nice feature, Hyrum! A couple questions:
> +/* Go up the directory tree, looking for a versioned directory. If found,
> + add all the intermediate directories. Otherwise, return SVN_ERR_XXX */
> +static svn_error_t *
> +add_parent_dirs(const char *path,
> + svn_wc_adm_access_t **parent_access,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_wc_adm_access_t *adm_access;
> + svn_error_t *err;
> +
> + err = svn_wc_adm_open3(&adm_access, NULL, path, TRUE, 0,
> + ctx->cancel_func, ctx->cancel_baton, pool);
> +
> + if (err && err->apr_err == SVN_ERR_WC_NOT_DIRECTORY)
> + {
> + if (svn_dirent_is_root(path, strlen(path)))
> + {
> + svn_error_clear(err);
> +
> + return svn_error_create
> + (SVN_ERR_CLIENT_NO_VERSIONED_PARENT, NULL, NULL);
> + }
> + else
> + {
> + const char *parent_path = svn_path_dirname(path, pool);
> +
> + svn_error_clear(err);
> + SVN_ERR(add_parent_dirs(parent_path, &adm_access, ctx, pool));
> + SVN_ERR(svn_wc_adm_retrieve(&adm_access, adm_access, parent_path,
> + pool));
> + SVN_ERR(svn_wc_add2(path, adm_access, NULL, SVN_INVALID_REVNUM,
> + ctx->cancel_func, ctx->cancel_baton,
> + ctx->notify_func2, ctx->notify_baton2, pool));
> + }
> + }
> + else if (err)
> + {
> + return err;
> + }
> +
> + if (parent_access)
> + *parent_access = adm_access;
> +
> + return SVN_NO_ERROR;
> +}
So it's recursive, not iterative, but it's still essentially an
unbounded iteration... should there be some pool clearing? I guess
it's a bit tricky since each call is passing a structure to the next
one. And it's bounded by the depth of the filesystem, which hopefully
shouldn't be enormous. And you're calling from a subpool anyway. So
maybe it doesn't matter.
> --- trunk/subversion/svn/main.c (original)
> +++ trunk/subversion/svn/main.c Wed May 16 15:56:38 2007
> @@ -203,7 +203,7 @@
> " "
> "using the name=value format")},
> {"parents", svn_cl__parents_opt, 0,
> - N_("make intermediate directories")},
> + N_("make and/or add intermediate directories")},
> {0, 0, 0, 0}
> };
I don't really like the and/or in this: if I understand correctly, for
some commands --parents makes the intermediate directories, and for
some commands it adds them, right?
Our infrastructure does support per-option help descriptions: see -F
in propset, for example. Maybe have the default say "make" and
override add's to say "add"?
--dave
--
David Glasser | glasser@mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r25049 - in trunk/subversion: include libsvn_client
svn tests/cmdline
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
David Glasser wrote:
> On 5/16/07, hwright@tigris.org <hw...@tigris.org> wrote:
>
> Very nice feature, Hyrum! A couple questions:
>
>> +/* Go up the directory tree, looking for a versioned directory. If
>> found,
>> + add all the intermediate directories. Otherwise, return
>> SVN_ERR_XXX */
>> +static svn_error_t *
>> +add_parent_dirs(const char *path,
>> + svn_wc_adm_access_t **parent_access,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *pool)
>> +{
>> + svn_wc_adm_access_t *adm_access;
>> + svn_error_t *err;
>> +
>> + err = svn_wc_adm_open3(&adm_access, NULL, path, TRUE, 0,
>> + ctx->cancel_func, ctx->cancel_baton, pool);
>> +
>> + if (err && err->apr_err == SVN_ERR_WC_NOT_DIRECTORY)
>> + {
>> + if (svn_dirent_is_root(path, strlen(path)))
>> + {
>> + svn_error_clear(err);
>> +
>> + return svn_error_create
>> + (SVN_ERR_CLIENT_NO_VERSIONED_PARENT, NULL, NULL);
>> + }
>> + else
>> + {
>> + const char *parent_path = svn_path_dirname(path, pool);
>> +
>> + svn_error_clear(err);
>> + SVN_ERR(add_parent_dirs(parent_path, &adm_access, ctx, pool));
>> + SVN_ERR(svn_wc_adm_retrieve(&adm_access, adm_access,
>> parent_path,
>> + pool));
>> + SVN_ERR(svn_wc_add2(path, adm_access, NULL,
>> SVN_INVALID_REVNUM,
>> + ctx->cancel_func, ctx->cancel_baton,
>> + ctx->notify_func2, ctx->notify_baton2,
>> pool));
>> + }
>> + }
>> + else if (err)
>> + {
>> + return err;
>> + }
>> +
>> + if (parent_access)
>> + *parent_access = adm_access;
>> +
>> + return SVN_NO_ERROR;
>> +}
>
> So it's recursive, not iterative, but it's still essentially an
> unbounded iteration... should there be some pool clearing? I guess
> it's a bit tricky since each call is passing a structure to the next
> one. And it's bounded by the depth of the filesystem, which hopefully
> shouldn't be enormous. And you're calling from a subpool anyway. So
> maybe it doesn't matter.
Yeah, I had the same concern -- and the same line of reasoning is what
led me to use a subpool for the whole iteration. There may be a more
complex method which allows for tighter memory usage, but I think the
number of recursions is likely to be quite small.
>
>> --- trunk/subversion/svn/main.c (original)
>> +++ trunk/subversion/svn/main.c Wed May 16 15:56:38 2007
>> @@ -203,7 +203,7 @@
>> " "
>> "using the name=value format")},
>> {"parents", svn_cl__parents_opt, 0,
>> - N_("make intermediate directories")},
>> + N_("make and/or add intermediate directories")},
>> {0, 0, 0, 0}
>> };
>
> I don't really like the and/or in this: if I understand correctly, for
> some commands --parents makes the intermediate directories, and for
> some commands it adds them, right?
>
> Our infrastructure does support per-option help descriptions: see -F
> in propset, for example. Maybe have the default say "make" and
> override add's to say "add"?
Good point, I forgot we have customizable switch descriptions. Updated
in r25054.
Thanks for the review,
-Hyrum