David Glasser wrote:
> On 5/16/07, hwright@tigris.org <hwright@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
Received on Thu May 17 22:53:47 2007