[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: A proposed solution for svn admin directory names

From: <kfogel_at_collab.net>
Date: 2005-08-17 20:55:16 CEST

Branko Čibej <brane@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
Received on Wed Aug 17 21:54:04 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.