You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2005/08/17 18:55:16 UTC
Re: A proposed solution for svn admin directory names
Branko Čibej <br...@xbc.nu> writes:
> The initial patch is below. It's a quick mock-up, but it removes the
> need for a public symbol for the adm dir name -- except for one place
> in libsvn_subr, where I've left the current code in because we'd get a
> circular dependency between libsvn_wc and libsvn_subr, which I don't
> quite know how to handle yet.
>
> The point is that if/when this is in, we can keep the knowledge about
> the actual admin directory name inside libsvn_wc/adm_files.c. If we
> decide that getenv is appropriate for 1.x, we can still cache the
> result in the local variable (using apr_atomc_casptr to change the
> value; we seem to have forgotten that APR has had atomics since at
> least 0.9.5, if not earlier, so this kind of local caching can be
> entirely safe).
I like this patch. If we can resolve the issues mentioned below, I'd
be +1 on applying it. It's quite more than a "quick mock-up", it's
almost complete except for the issues you noted below.
> Tests pass on Windows with this patch.
>
> [[[
> Remove knowledge about the admin directory name from the public
> API, preparing the way for run-time parameterisation.
>
> * subversion/include/svn_wc.h (svn_wc_is_adm_dir): New prototype.
> (SVN_WC_ADM_DIR_NAME): Deprecate.
>
> * subversion/libsvn_wc/adm_files.c (DEFAULT_ADM_DIR_NAME): New constant.
> (adm_dir_name) New file-scope variable.
> (svn_wc_is_adm_dir): Implement.
> (v_extend_with_adm_name): Use adm_dir_name instead of SVN_WC_ADM_DIR_NAME.
>
> * subversion/libsvn_wc/copy.c (post_copy_cleanup): Call svn_wc__adm_path
> to construct the path to the admin directory.
>
> * subversion/libsvn_wc/adm_ops.c (erase_from_wc),
> subversion/libsvn_wc/status.c (get_dir_status),
> subversion/libsvn_wc/update_editor.c (add_directory),
> subversion/libsvn_client/add.c (add_dir_recursive),
> subversion/libsvn_client/commit.c (import_dir, svn_client_import2):
> Call svn_wc_is_adm_dir instead of strcmp'ing with SVN_WC_ADM_DIR_NAME.
>
> * subversion/libsvn_subr/opt.c (svn_opt_args_to_target_array2):
> Mark the place where a similar change should be made.
> ]]]
>
>
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 15318)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -337,6 +337,15 @@
> apr_pool_t *pool);
> +/**
> + * Return @c TRUE if @a name is the name of the WC administrative
> + * directory (usually ".svn"). Use @a pool for any temporary allocations.
> + *
> + * @since New in 1.3.
> + */
> +svn_boolean_t svn_wc_is_adm_dir (const char *name, apr_pool_t *pool);
> +
> +
>
> /** Traversal information is information gathered by a working copy
> * crawl or update. For example, the before and after values of the
> @@ -1023,6 +1032,8 @@
> * who knew the adm subdir's name). However, import wants to protect
> * against importing administrative subdirs, so now the name is a
> * matter of public record.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> */
> #define SVN_WC_ADM_DIR_NAME ".svn"
> Index: subversion/libsvn_wc/copy.c
> ===================================================================
> --- subversion/libsvn_wc/copy.c (revision 15318)
> +++ subversion/libsvn_wc/copy.c (working copy)
> @@ -291,7 +291,7 @@
> hidden. */
> #ifdef APR_FILE_ATTR_HIDDEN
> {
> - const char *adm_dir = svn_path_join (path, SVN_WC_ADM_DIR_NAME, pool);
> + const char *adm_dir = svn_wc__adm_path (path, FALSE, pool, NULL);
> const char *path_apr;
> apr_status_t status;
> SVN_ERR (svn_path_cstring_from_utf8 (&path_apr, adm_dir, pool));
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 15318)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -770,7 +770,7 @@
> name = key;
> /* The admin directory will show up, we don't want to
> delete it */
> - if (!strcmp (name, SVN_WC_ADM_DIR_NAME))
> + if (svn_wc_is_adm_dir (name, pool))
> continue;
> /* Versioned directories will show up, don't delete those
> either */
> Index: subversion/libsvn_wc/status.c
> ===================================================================
> --- subversion/libsvn_wc/status.c (revision 15318)
> +++ subversion/libsvn_wc/status.c (working copy)
> @@ -869,8 +869,8 @@
> /* Skip versioned things, and skip the administrative
> directory. */
> - if ((apr_hash_get (entries, key, klen)) - || (strcmp
> (key, SVN_WC_ADM_DIR_NAME) == 0))
> + if (apr_hash_get (entries, key, klen)
> + || svn_wc_is_adm_dir (key, subpool))
> continue;
> /* Clear the iteration subpool. */
Oops, your mailer is wrapping long lines I think?
> Index: subversion/libsvn_wc/adm_files.c
> ===================================================================
> --- subversion/libsvn_wc/adm_files.c (revision 15318)
> +++ subversion/libsvn_wc/adm_files.c (working copy)
> @@ -42,7 +42,25 @@
>
> /*** File names in the adm area. ***/
> +/* The default name of the WC admin directory. This name is always
> + checked by svn_wc_is_adm_dir. */
> +#define DEFAULT_ADM_DIR_NAME ".svn"
> +/* The name that is actually used for the WC admin directory. The
> + commonest case where this won't be the default is in Windows
> + ASP.NET development environments, which choke on ".svn". */
> +static const char *adm_dir_name = DEFAULT_ADM_DIR_NAME;
> +
> +
> +svn_boolean_t
> +svn_wc_is_adm_dir (const char *name, apr_pool_t *pool)
> +{
> + (void)pool; /* Silence compiler warnings about unused parameter */
> + return (0 == strcmp (name, adm_dir_name)
> + || 0 == strcmp (name, DEFAULT_ADM_DIR_NAME));
> +}
> +
> +
> /* Return the path to something in PATH's administrative area.
> * * First, the adm subdir is appended to PATH as a component, then
> the
> @@ -66,7 +84,7 @@
> const char *this;
> /* Tack on the administrative subdirectory. */
> - path = svn_path_join (path, SVN_WC_ADM_DIR_NAME, pool);
> + path = svn_path_join (path, adm_dir_name, pool);
> /* If this is a tmp file, name it into the tmp area. */
> if (use_tmp)
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 15318)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -1050,7 +1050,7 @@
> svn_path_local_style (db->path, pool));
> /* It may not be named the same as the administrative directory. */
> - if (strcmp (svn_path_basename (path, pool), SVN_WC_ADM_DIR_NAME) == 0)
> + if (svn_wc_is_adm_dir (svn_path_basename (path, pool), pool))
> return svn_error_createf
> (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> _("Failed to add directory '%s': object of the same name as the "
> Index: subversion/libsvn_subr/opt.c
> ===================================================================
> --- subversion/libsvn_subr/opt.c (revision 15318)
> +++ subversion/libsvn_subr/opt.c (working copy)
> @@ -654,6 +654,10 @@
> target a SVN admin dir unless svn_wc_check_wc passes on
> the target, too? */
> base_name = svn_path_basename (target, pool);
> + /* FIXME: How to avoid the circular dependency between
> + linsvn_wc and libsvn_subr here?
> + if (svn_wc_is_adm_dir (base_name, pool))
> + */
> if (! strcmp (base_name, SVN_WC_ADM_DIR_NAME))
> continue;
> }
Yeah, this is a problem.
We could break the svn_opt stuff out into its own library. While it
would be a bit odd for libsvn_opt to depend on libsvn_wc, it wouldn't
actually be circular.
Another solution would be for svn_opt_args_to_target_array2() to stop
skipping these, and for its callers to instead call some new svn_wc
function that takes a target array and removes the ".svn" (etc)
members from it. At svn_opt_args_to_target_array2(), we'd simply
document that calling svn_wc_clean_targets() (or whatever) is highly
recommended.
Thoughts?
> Index: subversion/libsvn_client/add.c
> ===================================================================
> --- subversion/libsvn_client/add.c (revision 15318)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -327,7 +327,7 @@
> SVN_ERR (ctx->cancel_func (ctx->cancel_baton));
> /* Skip over SVN admin directories. */
> - if (strcmp (this_entry.name, SVN_WC_ADM_DIR_NAME) == 0)
> + if (svn_wc_is_adm_dir (this_entry.name, subpool))
> continue;
> /* Skip entries for this dir and its parent. */
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 15318)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -335,7 +335,7 @@
> if (ctx->cancel_func)
> SVN_ERR (ctx->cancel_func (ctx->cancel_baton));
> - if (strcmp (filename, SVN_WC_ADM_DIR_NAME) == 0)
> + if (svn_wc_is_adm_dir (filename, subpool))
> {
> /* If someone's trying to import a directory named the same
> as our administrative directories, that's probably not
> @@ -748,14 +748,15 @@
> /* The repository doesn't know about the reserved administrative
> directory. */
> if (new_entries->nelts && - (strcmp (APR_ARRAY_IDX
> (new_entries, - new_entries->nelts - 1, -
> const char *), SVN_WC_ADM_DIR_NAME) == 0))
> + svn_wc_is_adm_dir (temp = APR_ARRAY_IDX (new_entries, +
> new_entries->nelts - 1, +
> const char *),
> + pool))
> return svn_error_createf
> (SVN_ERR_CL_ADM_DIR_RESERVED, NULL,
> _("'%s' is a reserved name and cannot be imported"),
> /* ### Is svn_path_local_style() really necessary for this? */
> - svn_path_local_style (SVN_WC_ADM_DIR_NAME, pool));
> + svn_path_local_style (temp, pool));
> /* If an error occurred during the commit, abort the edit and return
Uck, the in-call assignment of temp bothers me, but not enough to get
up and do anything about it :-).
Nice change, overall, if we can get that circular-dependency issue
resolved.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: A proposed solution for svn admin directory names
Posted by Branko Čibej <br...@xbc.nu>.
Jonathan Malek wrote:
> Just doing a quick check to see what the status of this is? I am
> excited to see a change like this introduced.
It's sitting in my working copy. I'll clean up the patch and commit it
before 1.3.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: A proposed solution for svn admin directory names
Posted by Jonathan Malek <jo...@gmail.com>.
Just doing a quick check to see what the status of this is? I am excited to
see a change like this introduced.
Thanks,
Jonathan Malek
On 8/17/05, Branko Čibej <br...@xbc.nu> wrote:
>
> kfogel@collab.net wrote:
>
> >Branko Čibej <br...@xbc.nu> writes:
> >
> >
> >>Index: subversion/libsvn_subr/opt.c
> >>===================================================================
> >>--- subversion/libsvn_subr/opt.c (revision 15318)
> >>+++ subversion/libsvn_subr/opt.c (working copy)
> >>@@ -654,6 +654,10 @@
> >> target a SVN admin dir unless svn_wc_check_wc passes on
> >> the target, too? */
> >> base_name = svn_path_basename (target, pool);
> >>+ /* FIXME: How to avoid the circular dependency between
> >>+ linsvn_wc and libsvn_subr here?
> >>+ if (svn_wc_is_adm_dir (base_name, pool))
> >>+ */
> >> if (! strcmp (base_name, SVN_WC_ADM_DIR_NAME))
> >> continue;
> >> }
> >>
> >>
> >
> >Yeah, this is a problem.
> >
> >We could break the svn_opt stuff out into its own library. While it
> >would be a bit odd for libsvn_opt to depend on libsvn_wc, it wouldn't
> >actually be circular.
> >
> >
> That would break ABI compatiblity, wouldn't it?
>
> Although, if we can work around that, I'd rather create the
> libsvn_cmdline I proposed ages ago. All the svn_cmdline and svn_opt
> stuff would go there.
>
> (And I can now take immense pleasure and say "I told you so" to everyone
> who opposed the libsvn_cmdline idea in the first place. :)
>
>
> >Another solution would be for svn_opt_args_to_target_array2() to stop
> >skipping these, and for its callers to instead call some new svn_wc
> >function that takes a target array and removes the ".svn" (etc)
> >members from it. At svn_opt_args_to_target_array2(), we'd simply
> >document that calling svn_wc_clean_targets() (or whatever) is highly
> >recommended.
> >
> >Thoughts?
> >
> >
> I don't think that's a good idea. It would be changing our API
> semantics, and not for a bug fix.
>
>
> >>+ svn_wc_is_adm_dir (temp = APR_ARRAY_IDX (new_entries, +
> >>new_entries->nelts - 1, +
> >>const char *),
> >>+ pool))
> >>
> >>
> >Uck, the in-call assignment of temp bothers me, but not enough to get
> >up and do anything about it :-).
> >
> >
> Oh, I can fix that, I suppose, although the change would make the code
> less elegant. :)
>
> >Nice change, overall, if we can get that circular-dependency issue
> >resolved.
> >
> >
> I suppose that, for 1.x, we could cheat. Indeed,
> svn_opt_args_to_target_array should filter out all possible admin
> directory names, and we do want to keep that list short (probably just
> .svn and _svn). Maybe we should just maintain the list in two places for
> now, with ample comments and warnings in both.
>
> -- Brane
>
>
Re: A proposed solution for svn admin directory names
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>Branko Čibej <br...@xbc.nu> writes:
>
>
>>Index: subversion/libsvn_subr/opt.c
>>===================================================================
>>--- subversion/libsvn_subr/opt.c (revision 15318)
>>+++ subversion/libsvn_subr/opt.c (working copy)
>>@@ -654,6 +654,10 @@
>> target a SVN admin dir unless svn_wc_check_wc passes on
>> the target, too? */
>> base_name = svn_path_basename (target, pool);
>>+ /* FIXME: How to avoid the circular dependency between
>>+ linsvn_wc and libsvn_subr here?
>>+ if (svn_wc_is_adm_dir (base_name, pool))
>>+ */
>> if (! strcmp (base_name, SVN_WC_ADM_DIR_NAME))
>> continue;
>> }
>>
>>
>
>Yeah, this is a problem.
>
>We could break the svn_opt stuff out into its own library. While it
>would be a bit odd for libsvn_opt to depend on libsvn_wc, it wouldn't
>actually be circular.
>
>
That would break ABI compatiblity, wouldn't it?
Although, if we can work around that, I'd rather create the
libsvn_cmdline I proposed ages ago. All the svn_cmdline and svn_opt
stuff would go there.
(And I can now take immense pleasure and say "I told you so" to everyone
who opposed the libsvn_cmdline idea in the first place. :)
>Another solution would be for svn_opt_args_to_target_array2() to stop
>skipping these, and for its callers to instead call some new svn_wc
>function that takes a target array and removes the ".svn" (etc)
>members from it. At svn_opt_args_to_target_array2(), we'd simply
>document that calling svn_wc_clean_targets() (or whatever) is highly
>recommended.
>
>Thoughts?
>
>
I don't think that's a good idea. It would be changing our API
semantics, and not for a bug fix.
>>+ svn_wc_is_adm_dir (temp = APR_ARRAY_IDX (new_entries, +
>>new_entries->nelts - 1, +
>>const char *),
>>+ pool))
>>
>>
>Uck, the in-call assignment of temp bothers me, but not enough to get
>up and do anything about it :-).
>
>
Oh, I can fix that, I suppose, although the change would make the code
less elegant. :)
>Nice change, overall, if we can get that circular-dependency issue
>resolved.
>
>
I suppose that, for 1.x, we could cheat. Indeed,
svn_opt_args_to_target_array should filter out all possible admin
directory names, and we do want to keep that list short (probably just
.svn and _svn). Maybe we should just maintain the list in two places for
now, with ample comments and warnings in both.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org