You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jonathan Malek <jo...@gmail.com> on 2005/06/24 19:03:53 UTC
A proposed solution for svn admin directory names
I know there is quite a bit of resistance to dealing with the issue of
.svn vs. _svn (or really any other name) as admin directory names,
especially given the presence of other work-arounds, but it still
continues to present a roadblock to some of us (especially those who
have to deal with contractors and developers while also maintaining a
decent automated build environment).
So while running my usual recompile to create modified executables
that support "_svn", I decided to try something different. I am
aiming for something that doesn't change the way the vast majority of
SVN users work, but allows the poor fellow who really needs "_svn" to
continue to work.
Instead of changing svn_wc.h with the usual:
#define SVN_WC_ADM_DIR_NAME ".svn"
I decided to fetch the dirname from the environment:
#define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
"SVN_ADM_DIR") ) : (".svn") )
This works just fine for anyone who hasn't set the environment
variable, but allows the rest of us to change it without recompiling.
I'm sure some other devs can come up with better suggestions, but--how
about it?
Thanks,
Jonathan Malek
---------------------------------------------------------------------
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>.
Marcus Rueckert wrote:
>On 2005-07-12 00:05:07 +0200, Branko Čibej wrote:
>
>
>>So: the only reason SVN_WC_ADM_DIR_NAME is public is because "svn
>>import" wants to avoid importing anything that's called ".svn". If we
>>change this definition, then suddenly "svn import" (and probably add,
>>and others) _could_ end up importing the ".svn" dir.
>>
>>All of which convinced me that we should deprecate SVN_WC_ADM_DIR_NAME,
>>and replace all its uses outside libsvn_wc (which, not surprisingly, are
>>all string compares against this constant) with calls to a new function
>>in libsvn_wc, e.g., svn_wc_is_adm_dir(const char*).
>>
>>Then we can deal with using different admin dir names privately inside
>>libsvn_wc, where such decisions belong.
>>
>>
>
>How do you plan to handle the case that someone checks out a working
>copy with svn admin dir set to ".svn" and imports with setting them to
>"_svn" or vice versa?
>
>
Two ways:
* .svn will always be reserved, regardless of the name you're
actually using.
* You would *not* be allowed to choose any name at all, but only
select froma short list (probably just .svn and _svn). Then we
could make all of these names reserved.
-- 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 Marcus Rueckert <da...@web.de>.
On 2005-07-12 00:05:07 +0200, Branko Čibej wrote:
> So: the only reason SVN_WC_ADM_DIR_NAME is public is because "svn
> import" wants to avoid importing anything that's called ".svn". If we
> change this definition, then suddenly "svn import" (and probably add,
> and others) _could_ end up importing the ".svn" dir.
>
> All of which convinced me that we should deprecate SVN_WC_ADM_DIR_NAME,
> and replace all its uses outside libsvn_wc (which, not surprisingly, are
> all string compares against this constant) with calls to a new function
> in libsvn_wc, e.g., svn_wc_is_adm_dir(const char*).
>
> Then we can deal with using different admin dir names privately inside
> libsvn_wc, where such decisions belong.
How do you plan to handle the case that someone checks out a working
copy with svn admin dir set to ".svn" and imports with setting them to
"_svn" or vice versa?
Marcus
---------------------------------------------------------------------
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 Charles Bailey <ba...@gmail.com>.
On 7/12/05, Branko Čibej <br...@xbc.nu> wrote:> Charles Bailey wrote:> > >On 7/11/05, Branko Čibej <br...@xbc.nu> wrote:> >===================================================================> >> >> >>--- 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);> >>> >>> >> >Is it worthwhile tomake svn_wc__adm_path() public as well, so that a> >hypothetical tool layered on Subversion can find the admin dir to add> >its own files?> >> >> I don't think that's safe. We'd have to introduce some kind of global> tool registry in order to avoid name clashes in the adm directory, and> that's flaky at best.
Fair enough, though it'd also be nice to avoid proliferation of admindirs. As an alternative, perhaps you'd want to let the applicationset the admin dir (on its own head be it if it changes valuesmid-stream or doesn't take care of thread safety), to allow for tool"foo" built on the Subversion APIs to make .foo its admin dir, andtell Subversion to use .foo/svn.
-- Regards,Charles BaileyLists: bailey _dot_ charles _at_ gmail _dot_ comOther: bailey _at_ newman _dot_ upenn _dot_ edu
Re: A proposed solution for svn admin directory names
Posted by Branko Čibej <br...@xbc.nu>.
Charles Bailey wrote:
>On 7/11/05, Branko Čibej <br...@xbc.nu> wrote:
>===================================================================
>
>
>>--- 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);
>>
>>
>
>Is it worthwhile tomake svn_wc__adm_path() public as well, so that a
>hypothetical tool layered on Subversion can find the admin dir to add
>its own files?
>
>
I don't think that's safe. We'd have to introduce some kind of global
tool registry in order to avoid name clashes in the adm directory, and
that's flaky at best.
-- 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 Charles Bailey <ba...@gmail.com>.
On 7/11/05, Branko Čibej <br...@xbc.nu> wrote:===================================================================> --- 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);
Is it worthwhile tomake svn_wc__adm_path() public as well, so that ahypothetical tool layered on Subversion can find the admin dir to addits own files?-- Regards,Charles BaileyLists: bailey _dot_ charles _at_ gmail _dot_ comOther: bailey _at_ newman _dot_ upenn _dot_ edu
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
Re: A proposed solution for svn admin directory names
Posted by kf...@collab.net.
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>.
Branko Čibej wrote:
> All of which convinced me that we should deprecate
> SVN_WC_ADM_DIR_NAME, and replace all its uses outside libsvn_wc
> (which, not surprisingly, are all string compares against this
> constant) with calls to a new function in libsvn_wc, e.g.,
> svn_wc_is_adm_dir(const char*).
>
> Then we can deal with using different admin dir names privately inside
> libsvn_wc, where such decisions belong.
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).
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. */
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;
}
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
-- 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 "Brian W. Fitzpatrick" <fi...@collab.net>.
On Tue, 2005-07-12 at 00:05 +0200, Branko Čibej wrote:
> So: the only reason SVN_WC_ADM_DIR_NAME is public is because "svn
> import" wants to avoid importing anything that's called ".svn". If we
> change this definition, then suddenly "svn import" (and probably add,
> and others) _could_ end up importing the ".svn" dir.
>
> All of which convinced me that we should deprecate SVN_WC_ADM_DIR_NAME,
> and replace all its uses outside libsvn_wc (which, not surprisingly, are
> all string compares against this constant) with calls to a new function
> in libsvn_wc, e.g., svn_wc_is_adm_dir(const char*).
+1
-Fitz
---------------------------------------------------------------------
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:
>I know there is quite a bit of resistance to dealing with the issue of
>.svn vs. _svn (or really any other name) as admin directory names,
>especially given the presence of other work-arounds, but it still
>continues to present a roadblock to some of us (especially those who
>have to deal with contractors and developers while also maintaining a
>decent automated build environment).
>
>So while running my usual recompile to create modified executables
>that support "_svn", I decided to try something different. I am
>aiming for something that doesn't change the way the vast majority of
>SVN users work, but allows the poor fellow who really needs "_svn" to
>continue to work.
>
>Instead of changing svn_wc.h with the usual:
>
>#define SVN_WC_ADM_DIR_NAME ".svn"
>
>I decided to fetch the dirname from the environment:
>
>#define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
>"SVN_ADM_DIR") ) : (".svn") )
>
>This works just fine for anyone who hasn't set the environment
>variable, but allows the rest of us to change it without recompiling.
>I'm sure some other devs can come up with better suggestions, but--how
>about it?
>
>
I've finally figured out why this solution won't fly, and incidentally,
why patched Subversion binaries are evil. The reason lies in the comment
above that #define, which saith thusly:
/** Administrative subdir.
*
* Ideally, this would be completely private to wc internals (in fact,
* it used to be that adm_subdir() in adm_files.c was the only function
* 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.
*/
So: the only reason SVN_WC_ADM_DIR_NAME is public is because "svn
import" wants to avoid importing anything that's called ".svn". If we
change this definition, then suddenly "svn import" (and probably add,
and others) _could_ end up importing the ".svn" dir.
All of which convinced me that we should deprecate SVN_WC_ADM_DIR_NAME,
and replace all its uses outside libsvn_wc (which, not surprisingly, are
all string compares against this constant) with calls to a new function
in libsvn_wc, e.g., svn_wc_is_adm_dir(const char*).
Then we can deal with using different admin dir names privately inside
libsvn_wc, where such decisions belong.
-- 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 Brass Tilde <br...@insightbb.com>.
> equivalent of your posited getConfigOption() function, which means it's
> not available to the expansion of the SVN_WC_ADM_DIR_NAME macro. (Also,
> I don't think we even pass the config object to functions in libsvn_wc
> at the moment.)
That's what I get for making suggestions without looking at the code.
---------------------------------------------------------------------
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 Greg Hudson <gh...@MIT.EDU>.
On Wed, 2005-06-29 at 13:52 -0400, Brass Tilde wrote:
> I'll presume for the sake of argument that the options in the configuration file are read when the svn program starts, and saved in
> some sort of structure to be available when needed. I'll further presume that there is some method, such as a "getConfigOption()"
> function, for sections of code to retrieve relevant items from this structure when needed.
These things are true, but we don't use global variables (much) in the
Subversion library. The config object has to be passed to the
equivalent of your posited getConfigOption() function, which means it's
not available to the expansion of the SVN_WC_ADM_DIR_NAME macro. (Also,
I don't think we even pass the config object to functions in libsvn_wc
at the moment.)
---------------------------------------------------------------------
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 Brass Tilde <br...@insightbb.com>.
> > Calling getenv() that often is horrible.
>
> I think if we're going to reject this solution on the basis of
> performance, the burden is on us to measure a performance penalty.
>
> The other objections raised (environment variables aren't our preferred
> method of configuration, and we'd be breaking source compatibility) are
> more serious, though.
Without looking at the code reading configuration options and using them (and likely not understanding it if I did), what about
combining the two approaches?
I'll presume for the sake of argument that the options in the configuration file are read when the svn program starts, and saved in
some sort of structure to be available when needed. I'll further presume that there is some method, such as a "getConfigOption()"
function, for sections of code to retrieve relevant items from this structure when needed.
Instead of calling getenv() in the #define, call something that reads the configuration structure and gets the SVN_ADM_DIR value.
You could then put it in the configuration file *without* modifying every piece of code that uses the #define.
So, to modify Joseph Galbraith's snippet a wee bit:
char* getadmdir()
{
static char* szAdmDir = 0;
if ( szAdmDir == 0 )
{
szAdmDir = getConfigOption("SVN_ADM_DIR");
if ( szAdmDir == 0 )
szAdmDir = ".svn";
}
return szAdmDir;
}
#define SVN_WC_ADM_DIR_NAME getadmdir()
No massive code changes (unless that #define is present in every source file that uses it). No dependence on environment. Still
depends on static variables though, and my experience with multi-threaded issues is...paltry.
Brad
---------------------------------------------------------------------
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 Philip Martin <ph...@codematters.co.uk>.
Greg Hudson <gh...@MIT.EDU> writes:
> On Wed, 2005-06-29 at 17:22 +0100, Philip Martin wrote:
>> Calling getenv() that often is horrible.
>
> I think if we're going to reject this solution on the basis of
> performance, the burden is on us to measure a performance penalty.
It appears to make 'svn st' on a Subversion trunk about 1% slower.
--
Philip Martin
---------------------------------------------------------------------
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 Greg Hudson <gh...@MIT.EDU>.
On Wed, 2005-06-29 at 17:22 +0100, Philip Martin wrote:
> Calling getenv() that often is horrible.
I think if we're going to reject this solution on the basis of
performance, the burden is on us to measure a performance penalty.
getenv() does do a linear search, but it's a memory operation and the
environment almost always fits into L1 cache. I bet you won't be able
to tell the difference.
The other objections raised (environment variables aren't our preferred
method of configuration, and we'd be breaking source compatibility) are
more serious, though.
---------------------------------------------------------------------
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 Garrett Rooney <ro...@electricjellyfish.net>.
Jonathan Malek wrote:
> I know this is kind of a thread off the main topic, but there is a
> pthreads implementation for win32, with simple linking to a dll
> (http://sources.redhat.com/pthreads-win32/) (along with support for
> pthreads in a surprising number of places, including netware). Is
> this idea still DOA?
It's highly unlikely that any solution that involves emulating pthreads
on win32 would be committed.
-garrett
---------------------------------------------------------------------
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>.
I know this is kind of a thread off the main topic, but there is a
pthreads implementation for win32, with simple linking to a dll
(http://sources.redhat.com/pthreads-win32/) (along with support for
pthreads in a surprising number of places, including netware). Is
this idea still DOA?
On 6/29/05, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> Jonathan Malek wrote:
> > I may have read it wrong, but I thought apr used pthread.h? It may be
> > that's not the pthread library I know...
>
> On unix it does, but on windows it uses win32 threads, on Netware it
> uses something else, on OS/2 and OS/390 who knows what it uses. You
> can't depend on pthreads being there, that's the whole reason we have a
> portability library.
>
> -garrett
>
---------------------------------------------------------------------
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 Joseph Galbraith <ga...@vandyke.com>.
Joseph Galbraith wrote:
> C. Michael Pilato wrote:
>
>> Philip Martin <ph...@codematters.co.uk> writes:
>>
>>
>>> Ben Collins-Sussman <su...@collab.net> writes:
>>>
>>>
>>>> On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
>>>>
>>>>> #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
>>>>> "SVN_ADM_DIR") ) : (".svn") )
>>>
>>>
>>>> IANTSP -- "I am not the subversion project". Do any other developers
>>>> have opinions about this?
>>>
>>>
>>> Calling getenv() that often is horrible.
>
> Well, maybe this is better, or perhaps more horrible (excuse
> the C++ism and pseudo-hungarianisms:
>
> char* getadmdir()
> {
> static bool bHaveDir = false;
> static char* szAdmDir = 0;
> if ( ! bHaveDir )
> {
> bHaveDir = true;
> szAdmDir = ::getevn("SVN_ADM_DIR");
> if ( szAdmDir == 0 )
> szAdmDir = ".svn";
> }
>
> return szAdmDir;
> }
>
> #define SVN_WC_ADM_DIR_NAME getadmdir()
Argh... one should always re-read code-- or how about
this more consise snippet with fewer typos to boot:
char* getadmdir()
{
static char* szAdmDir = 0;
if ( szAdmDir == 0 )
{
szAdmDir = ::getenv("SVN_ADM_DIR");
if ( szAdmDir == 0 )
szAdmDir = ".svn";
}
return szAdmDir;
}
#define SVN_WC_ADM_DIR_NAME getadmdir()
Of course getadmdir() has to be put someplace it won't cause multiple
definitions and can be found everyplace SVN_WC_ADM_DIR_NAME is used...
but surely there is someplace that meets these requirements?
Thanks,
Joseph
---------------------------------------------------------------------
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 Garrett Rooney <ro...@electricjellyfish.net>.
Jonathan Malek wrote:
> I may have read it wrong, but I thought apr used pthread.h? It may be
> that's not the pthread library I know...
On unix it does, but on windows it uses win32 threads, on Netware it
uses something else, on OS/2 and OS/390 who knows what it uses. You
can't depend on pthreads being there, that's the whole reason we have a
portability library.
-garrett
---------------------------------------------------------------------
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>.
I may have read it wrong, but I thought apr used pthread.h? It may be
that's not the pthread library I know...
On 6/29/05, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> Jonathan Malek wrote:
>
> > As to (2), we could use pthread (used by apr) and something like:
> >
> > pthread_mutex_t svn_admin_dir_mutex = PTHREAD_MUTEX_INITIALIZER ;
> >
> > char * getadmdir()
> > {
> > static char * szAdmDir = 0 ;
> >
> > pthread_mutex_lock( &svn_admin_dir_mutex ) ;
> > if( szAdmDir == 0 )
> > {
> > szAdmDir = getenv( "SVN_ADM_DIR" ) ;
> > if( szAdmDir == 0 )
> > szAdmDir = ".svn" ;
> > }
> > pthread_mutex_unlock( &svn_admin_dir_mutex ) ;
> >
> > return szAdmDir ;
> > }
>
> Actually, APR mutexes don't have static initializers like pthread
> mutexes, so you need to initialize them from some sort of init function
> before they can be used. So for correct locking there has to be some
> sort of global initialization function called before getadmdir is used.
> Yes, I agree this sucks, but those are the breaks...
>
> -garrett
>
---------------------------------------------------------------------
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 Garrett Rooney <ro...@electricjellyfish.net>.
Jonathan Malek wrote:
> As to (2), we could use pthread (used by apr) and something like:
>
> pthread_mutex_t svn_admin_dir_mutex = PTHREAD_MUTEX_INITIALIZER ;
>
> char * getadmdir()
> {
> static char * szAdmDir = 0 ;
>
> pthread_mutex_lock( &svn_admin_dir_mutex ) ;
> if( szAdmDir == 0 )
> {
> szAdmDir = getenv( "SVN_ADM_DIR" ) ;
> if( szAdmDir == 0 )
> szAdmDir = ".svn" ;
> }
> pthread_mutex_unlock( &svn_admin_dir_mutex ) ;
>
> return szAdmDir ;
> }
Actually, APR mutexes don't have static initializers like pthread
mutexes, so you need to initialize them from some sort of init function
before they can be used. So for correct locking there has to be some
sort of global initialization function called before getadmdir is used.
Yes, I agree this sucks, but those are the breaks...
-garrett
---------------------------------------------------------------------
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>.
I wanted to thank everyone for the feedback on the suggestion. This
is my understanding of the chain so far:
1) The getenv() call reduces performance by 1% in the hyper-often call
scenario (the first suggestion)
2) A static with some kind of synchronization, using Joseph's
suggestion would reduce this performance penalty
As to (2), we could use pthread (used by apr) and something like:
pthread_mutex_t svn_admin_dir_mutex = PTHREAD_MUTEX_INITIALIZER ;
char * getadmdir()
{
static char * szAdmDir = 0 ;
pthread_mutex_lock( &svn_admin_dir_mutex ) ;
if( szAdmDir == 0 )
{
szAdmDir = getenv( "SVN_ADM_DIR" ) ;
if( szAdmDir == 0 )
szAdmDir = ".svn" ;
}
pthread_mutex_unlock( &svn_admin_dir_mutex ) ;
return szAdmDir ;
}
Thanks,
Jonathan
On 6/29/05, Joseph Galbraith <ga...@vandyke.com> wrote:
> Duane Griffin wrote:
> > On Wed, 2005-06-29 at 18:26, Joseph Galbraith wrote:
> >
> >>char* getadmdir()
> >>{
> >> static char* szAdmDir = 0;
> >> if ( szAdmDir == 0 )
> >> {
> >> szAdmDir = ::getenv("SVN_ADM_DIR");
> >> if ( szAdmDir == 0 )
> >> szAdmDir = ".svn";
> >> }
> >>
> >> return szAdmDir;
> >>}
> >
> >
> > Aside from the possibility of non-atomic pointer assignment, there is
> > another failure case where the function returns NULL:
> >
> > Consider the situation with two threads A and B where SVN_ADM_DIR is not
> > set. A and B both enter the function and the first if block. Thread A
> > continues into the second if block and does the assignment. Now thread B
> > executes the getenv line. Thread A executes the return statement,
> > returning the current global value of szAdmDir, which has been set by
> > thread B to NULL. Bang.
> >
> > It really isn't possible to do this sort of thing in a thread-safe
> > manner without synchronization.
>
> Well, given that the non-atomic pointer assignment does seem to kill
> it in the portable sense, it probably isn't worthwhile continuing this
> discussion too much further. (And the objection to the use of
> environment variables for config in general-- though there is precednet
> in the SVN_SSH environment variable.)
>
> But, wouldn't the introduction of a temporary resolve the
> problem you point out above?
>
> char* szEnv = ::getenv("SVN_ADM_DIR");
> if ( szEnv == 0 )
> szAdmDir = ".svn"
> else
> szAdmDir = szEnv;
>
> Now szAdmDir is written exactly one time (in each thread), always
> with a valid value.
>
> Thanks for the careful analysis, by the way. I'll keep an eye
> out for that particular mistaken pattern in my own code.
>
> Thanks,
>
> Joseph
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
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 Duane Griffin <d....@psenterprise.com>.
On Wed, 2005-06-29 at 19:51, Joseph Galbraith wrote:
> Well, given that the non-atomic pointer assignment does seem to kill
> it in the portable sense, it probably isn't worthwhile continuing this
> discussion too much further. (And the objection to the use of
> environment variables for config in general-- though there is precednet
> in the SVN_SSH environment variable.)
>
> But, wouldn't the introduction of a temporary resolve the
> problem you point out above?
Yep, that looks like it would do it, assuming atomic assignment. Or at
least any problems are too subtle for me to pick out. :)
Cheers,
Duane.
--
Duane Griffin
Senior Software Developer
Process Systems Enterprise Limited - "The model company"
Bridge Studios, 107a Hammersmith Bridge Road, London W6 9DA
Call +44 (0)20 8563 0888 or visit us at www.psenterprise.com
---------------------------------------------------------------------
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 Joseph Galbraith <ga...@vandyke.com>.
Duane Griffin wrote:
> On Wed, 2005-06-29 at 18:26, Joseph Galbraith wrote:
>
>>char* getadmdir()
>>{
>> static char* szAdmDir = 0;
>> if ( szAdmDir == 0 )
>> {
>> szAdmDir = ::getenv("SVN_ADM_DIR");
>> if ( szAdmDir == 0 )
>> szAdmDir = ".svn";
>> }
>>
>> return szAdmDir;
>>}
>
>
> Aside from the possibility of non-atomic pointer assignment, there is
> another failure case where the function returns NULL:
>
> Consider the situation with two threads A and B where SVN_ADM_DIR is not
> set. A and B both enter the function and the first if block. Thread A
> continues into the second if block and does the assignment. Now thread B
> executes the getenv line. Thread A executes the return statement,
> returning the current global value of szAdmDir, which has been set by
> thread B to NULL. Bang.
>
> It really isn't possible to do this sort of thing in a thread-safe
> manner without synchronization.
Well, given that the non-atomic pointer assignment does seem to kill
it in the portable sense, it probably isn't worthwhile continuing this
discussion too much further. (And the objection to the use of
environment variables for config in general-- though there is precednet
in the SVN_SSH environment variable.)
But, wouldn't the introduction of a temporary resolve the
problem you point out above?
char* szEnv = ::getenv("SVN_ADM_DIR");
if ( szEnv == 0 )
szAdmDir = ".svn"
else
szAdmDir = szEnv;
Now szAdmDir is written exactly one time (in each thread), always
with a valid value.
Thanks for the careful analysis, by the way. I'll keep an eye
out for that particular mistaken pattern in my own code.
Thanks,
Joseph
---------------------------------------------------------------------
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 Duane Griffin <d....@psenterprise.com>.
On Wed, 2005-06-29 at 18:26, Joseph Galbraith wrote:
> char* getadmdir()
> {
> static char* szAdmDir = 0;
> if ( szAdmDir == 0 )
> {
> szAdmDir = ::getenv("SVN_ADM_DIR");
> if ( szAdmDir == 0 )
> szAdmDir = ".svn";
> }
>
> return szAdmDir;
> }
Aside from the possibility of non-atomic pointer assignment, there is
another failure case where the function returns NULL:
Consider the situation with two threads A and B where SVN_ADM_DIR is not
set. A and B both enter the function and the first if block. Thread A
continues into the second if block and does the assignment. Now thread B
executes the getenv line. Thread A executes the return statement,
returning the current global value of szAdmDir, which has been set by
thread B to NULL. Bang.
It really isn't possible to do this sort of thing in a thread-safe
manner without synchronization.
Regards,
Duane.
--
Duane Griffin
Senior Software Developer
Process Systems Enterprise Limited - "The model company"
Bridge Studios, 107a Hammersmith Bridge Road, London W6 9DA
Call +44 (0)20 8563 0888 or visit us at www.psenterprise.com
---------------------------------------------------------------------
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 Garrett Rooney <ro...@electricjellyfish.net>.
C. Michael Pilato wrote:
> I'll demonstrate a little ignorance here, perhaps, but isn't thread
> safety all about making sure different threads wouldn't get different
> data or stomp on each other's data? It would seem that this wouldn't
> really be likely to happen here. If you can safely assume that all
> threads of a single application would have the same environment (which
> might not be a safe assumption?), then at any given time, each thread
> would either have szAdmDir set to 0 or to some string which would have
> be the same string across all threads anyway.
>
> Now, about that ignorance -- what did I miss?
And if pointer updates are atomic on the platform in question you're
right, it doesn't matter one way or the other. But what if they aren't?
Then you can read that pointer and get a half-updated value, which is
no longer zero, so you say "oh, i follow this to get the value" and go
off into never-never land and crash. It's not likely to happen, pointer
updates are generally atomic, but it is platform specific and not
required by the standard.
-garrett
---------------------------------------------------------------------
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 Joseph Galbraith <ga...@vandyke.com>.
C. Michael Pilato wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
>
>>Joseph Galbraith <ga...@vandyke.com> writes:
>>
>>
>>>Garrett Rooney wrote:
>>>
>>>>Static variables like that are not especially thread safe.
>>>
>>>Ahh... I think my second code snippet address this problem
>>>as it does away with the bool and only uses the szAdmDir...
>>>I'll quote it here for clarity:
>>>
>>>char* getadmdir()
>>>{
>>> static char* szAdmDir = 0;
>>> if ( szAdmDir == 0 )
>>> {
>>
>>Pointers are probably atomic on all the platforms that run Subversion,
>>but strictly speaking it's not guaranteed and depends on the
>>platform.
>
>
> I'll demonstrate a little ignorance here, perhaps, but isn't thread
> safety all about making sure different threads wouldn't get different
> data or stomp on each other's data? It would seem that this wouldn't
> really be likely to happen here. If you can safely assume that all
> threads of a single application would have the same environment (which
> might not be a safe assumption?), then at any given time, each thread
> would either have szAdmDir set to 0 or to some string which would have
> be the same string across all threads anyway.
>
> Now, about that ignorance -- what did I miss?
If pointers aren't atomic, a thread might see szAdmDir as part 0 and
part some new value that is only half written. And fall down go boom!
Do we have either:
a.) A initialization function, etc., where we are know we aren't
multithreaded (i.e., a higher layer is responsible for either not
having started addtional threads or providing the locking)
b.) A portable interlocked exchange function?
Thanks,
Joseph
---------------------------------------------------------------------
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 "C. Michael Pilato" <cm...@collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> Joseph Galbraith <ga...@vandyke.com> writes:
>
> > Garrett Rooney wrote:
> >> Static variables like that are not especially thread safe.
> >
> > Ahh... I think my second code snippet address this problem
> > as it does away with the bool and only uses the szAdmDir...
> > I'll quote it here for clarity:
> >
> > char* getadmdir()
> > {
> > static char* szAdmDir = 0;
> > if ( szAdmDir == 0 )
> > {
>
> Pointers are probably atomic on all the platforms that run Subversion,
> but strictly speaking it's not guaranteed and depends on the
> platform.
I'll demonstrate a little ignorance here, perhaps, but isn't thread
safety all about making sure different threads wouldn't get different
data or stomp on each other's data? It would seem that this wouldn't
really be likely to happen here. If you can safely assume that all
threads of a single application would have the same environment (which
might not be a safe assumption?), then at any given time, each thread
would either have szAdmDir set to 0 or to some string which would have
be the same string across all threads anyway.
Now, about that ignorance -- what did I miss?
---------------------------------------------------------------------
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 Philip Martin <ph...@codematters.co.uk>.
Joseph Galbraith <ga...@vandyke.com> writes:
> Garrett Rooney wrote:
>> Static variables like that are not especially thread safe.
>
> Ahh... I think my second code snippet address this problem
> as it does away with the bool and only uses the szAdmDir...
> I'll quote it here for clarity:
>
> char* getadmdir()
> {
> static char* szAdmDir = 0;
> if ( szAdmDir == 0 )
> {
Pointers are probably atomic on all the platforms that run Subversion,
but strictly speaking it's not guaranteed and depends on the platform.
> szAdmDir = ::getenv("SVN_ADM_DIR");
> if ( szAdmDir == 0 )
> szAdmDir = ".svn";
> }
>
> return szAdmDir;
> }
--
Philip Martin
---------------------------------------------------------------------
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 Joseph Galbraith <ga...@vandyke.com>.
Garrett Rooney wrote:
> Joseph Galbraith wrote:
>
>> C. Michael Pilato wrote:
>>
>>> Philip Martin <ph...@codematters.co.uk> writes:
>>>
>>>
>>>> Ben Collins-Sussman <su...@collab.net> writes:
>>>>
>>>>
>>>>> On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
>>>>>
>>>>>> #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
>>>>>> "SVN_ADM_DIR") ) : (".svn") )
>>>>
>>>>
>>>>
>>>>> IANTSP -- "I am not the subversion project". Do any other developers
>>>>> have opinions about this?
>>>>
>>>>
>>>>
>>>> Calling getenv() that often is horrible.
>>>
>>>
>>>
>>>
>>> Well ... that answers one question in my previous email. :-)
>>
>>
>>
>> Well, maybe this is better, or perhaps more horrible (excuse
>> the C++ism and pseudo-hungarianisms:
>>
>> char* getadmdir()
>> {
>> static bool bHaveDir = false;
>> static char* szAdmDir = 0;
>> if ( ! bHaveDir )
>> {
>> bHaveDir = true;
>> szAdmDir = ::getevn("SVN_ADM_DIR");
>> if ( szAdmDir == 0 )
>> szAdmDir = ".svn";
>> }
>>
>> return szAdmDir;
>> }
>>
>> #define SVN_WC_ADM_DIR_NAME getadmdir()
>
> Static variables like that are not especially thread safe.
Ahh... I think my second code snippet address this problem
as it does away with the bool and only uses the szAdmDir...
I'll quote it here for clarity:
char* getadmdir()
{
static char* szAdmDir = 0;
if ( szAdmDir == 0 )
{
szAdmDir = ::getenv("SVN_ADM_DIR");
if ( szAdmDir == 0 )
szAdmDir = ".svn";
}
return szAdmDir;
}
#define SVN_WC_ADM_DIR_NAME getadmdir()
In this case, there is no observable instance
where szAdmDir isn't either 0 or valid. In
all cases where it is 0, the thread sets it
to the correct thing (if another thread beat
it to the punch it might be resetting it, almost
certainly to an identical value, but it will
still be valid at all observable points.)
Of course, in the n threaded worst case
scenario, ::genenv() could be called n
times.
> And you're
> still depending on an environment variable, which seems like a rather
> fragile way to do configuration options like this.
Ahh... well, I wasn't addressing that issue... but, it can probably
be addressed similarly if you'd like to.
In other words, a static variable that is either 0 or a valid
value and code to read a config file that can harmlessly
execute on multiple threads simultaneously. (i.e., is stack
or heap based, doesn't modify globals itself, and allows the
file to be read by multiple readers.) It might be worth
introducing some synchronzation though to avoid reading
the file multiple times... or if there is some point
where we know we are single threaded (an initialization
routine?) we could trigger the code there to ensure initialization
is only preformed on one thread at a time.
Personally, I think the environment variable is a good
solution in that it solves the problem simply.
Thanks,
Joseph
---------------------------------------------------------------------
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 Garrett Rooney <ro...@electricjellyfish.net>.
Joseph Galbraith wrote:
> C. Michael Pilato wrote:
>
>> Philip Martin <ph...@codematters.co.uk> writes:
>>
>>
>>> Ben Collins-Sussman <su...@collab.net> writes:
>>>
>>>
>>>> On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
>>>>
>>>>> #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
>>>>> "SVN_ADM_DIR") ) : (".svn") )
>>>
>>>
>>>> IANTSP -- "I am not the subversion project". Do any other developers
>>>> have opinions about this?
>>>
>>>
>>> Calling getenv() that often is horrible.
>>
>>
>>
>> Well ... that answers one question in my previous email. :-)
>
>
> Well, maybe this is better, or perhaps more horrible (excuse
> the C++ism and pseudo-hungarianisms:
>
> char* getadmdir()
> {
> static bool bHaveDir = false;
> static char* szAdmDir = 0;
> if ( ! bHaveDir )
> {
> bHaveDir = true;
> szAdmDir = ::getevn("SVN_ADM_DIR");
> if ( szAdmDir == 0 )
> szAdmDir = ".svn";
> }
>
> return szAdmDir;
> }
>
> #define SVN_WC_ADM_DIR_NAME getadmdir()
Static variables like that are not especially thread safe. And you're
still depending on an environment variable, which seems like a rather
fragile way to do configuration options like this.
-garrett
---------------------------------------------------------------------
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 Joseph Galbraith <ga...@vandyke.com>.
C. Michael Pilato wrote:
> Philip Martin <ph...@codematters.co.uk> writes:
>
>
>>Ben Collins-Sussman <su...@collab.net> writes:
>>
>>
>>>On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
>>>
>>>>#define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
>>>>"SVN_ADM_DIR") ) : (".svn") )
>>
>>>IANTSP -- "I am not the subversion project". Do any other developers
>>>have opinions about this?
>>
>>Calling getenv() that often is horrible.
>
>
> Well ... that answers one question in my previous email. :-)
Well, maybe this is better, or perhaps more horrible (excuse
the C++ism and pseudo-hungarianisms:
char* getadmdir()
{
static bool bHaveDir = false;
static char* szAdmDir = 0;
if ( ! bHaveDir )
{
bHaveDir = true;
szAdmDir = ::getevn("SVN_ADM_DIR");
if ( szAdmDir == 0 )
szAdmDir = ".svn";
}
return szAdmDir;
}
#define SVN_WC_ADM_DIR_NAME getadmdir()
Thanks,
Joseph
---------------------------------------------------------------------
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 "C. Michael Pilato" <cm...@collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> Ben Collins-Sussman <su...@collab.net> writes:
>
> > On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
> >>
> >> #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
> >> "SVN_ADM_DIR") ) : (".svn") )
>
> > IANTSP -- "I am not the subversion project". Do any other developers
> > have opinions about this?
>
> Calling getenv() that often is horrible.
Well ... that answers one question in my previous email. :-)
---------------------------------------------------------------------
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 Philip Martin <ph...@codematters.co.uk>.
Ben Collins-Sussman <su...@collab.net> writes:
> On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
>>
>> #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
>> "SVN_ADM_DIR") ) : (".svn") )
> IANTSP -- "I am not the subversion project". Do any other developers
> have opinions about this?
Calling getenv() that often is horrible.
--
Philip Martin
---------------------------------------------------------------------
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>.
steveking wrote:
> Example:
> On Windows, it's common to use the _T() macro for UNICODE strings. And
> every program using the SVN_WC_ADM_DIR_NAME define inside such a _T()
> macro would fail to build!
Any program that assumes that Subversion's macros will expand to string
literals definitely deserves that.
-- 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 steveking <st...@gmx.ch>.
Ben Collins-Sussman wrote:
>
> On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
>
>>
>> #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
>> "SVN_ADM_DIR") ) : (".svn") )
>>
>> Thanks for taking the time to look at this Ben--I know it's been
>> hashed and rehashed, but I think this is a simple solution that works.
>
> The 'cons' of this proposal previously were based on the assumption
> that making the value of SVN_ADM_DIR into a runtime option would have
> involved using ~/.subversion/config, which would have been a huge
> coding change. But now it seems like a tweak to the already existing
> #define will accomplish the same task.
>
> The only other 'con' listed was that ASP.NET working copies would be
> non-portable. But frankly, that's the world we already live in. There
> are already lots of shops out there using 'special' TortoiseSVN builds
> which use '_svn' directories. So from where I stand, this proposal
> seems like a net gain. Nothing gets any worse, but it means Tortoise
> can stop making special releases.
>
> IANTSP -- "I am not the subversion project". Do any other developers
> have opinions about this?
Well, changing that define would break the compatibility (source, not
binary), so it would have to wait until 2.0 ?
Example:
On Windows, it's common to use the _T() macro for UNICODE strings. And
every program using the SVN_WC_ADM_DIR_NAME define inside such a _T()
macro would fail to build!
So, I suggest introducing something like
#ifdef ADMIN_DIR_CONFIGURABLE
# define SVN_WC_ADM_DIR_NAME (( getenv( "SVN_ADM_DIR") ) ? (
getenv("SVN_ADM_DIR") ) : (".svn") )
#else
# define SVN_WC_ADM_DIR_NAME ".svn"
#endif
and then let the client decide which one to use by setting the define
ADMIN_DIR_CONFIGURABLE accordingly.
Of course, if source compatibility isn't important, changing the define
wouldn't hurt much - some simple changes in the source will do to make
it compile again.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
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 Philip Martin <ph...@codematters.co.uk>.
"C. Michael Pilato" <cm...@collab.net> writes:
> * What the performance penalties of calling getenv() as many times
> as it would be called by our already-slow libsvn_wc
> implementation?
It probably doesn't involve any disk IO, but it still doesn't look
good. I don't know what sort of algorithms are used, but it might
involve a linear search through all the environment variables
associated with the process looking for a matching name.
--
Philip Martin
---------------------------------------------------------------------
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 "C. Michael Pilato" <cm...@collab.net>.
Ben Collins-Sussman <su...@collab.net> writes:
> On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
> >
> > #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
> > "SVN_ADM_DIR") ) : (".svn") )
> >
> > Thanks for taking the time to look at this Ben--I know it's been
> > hashed and rehashed, but I think this is a simple solution that works.
>
>
> The 'cons' of this proposal previously were based on the assumption
> that making the value of SVN_ADM_DIR into a runtime option would have
> involved using ~/.subversion/config, which would have been a huge
> coding change. But now it seems like a tweak to the already existing
> #define will accomplish the same task.
>
> The only other 'con' listed was that ASP.NET working copies would be
> non-portable. But frankly, that's the world we already live in.
> There are already lots of shops out there using 'special' TortoiseSVN
> builds which use '_svn' directories. So from where I stand, this
> proposal seems like a net gain. Nothing gets any worse, but it means
> Tortoise can stop making special releases.
>
> IANTSP -- "I am not the subversion project". Do any other developers
> have opinions about this?
* What the performance penalties of calling getenv() as many times
as it would be called by our already-slow libsvn_wc
implementation?
* Of all the forms of runtime configurability, I'm a little bothered
that this one chooses one of least predictable/hardest to debug
ones available. Subversion runs its own hooks and such with
basically clean environments, which has caused a number of folks
to stumble over hooks that don't seem to work. Now, hooks might
not often do working copy operations -- I brought that up solely
because it illustrates (I think) how precarious the use of
environment variables can be. I know the .subversion/config thing
was a huge coding change, but frankly I think it's a far better
one.
---------------------------------------------------------------------
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 Ben Collins-Sussman <su...@collab.net>.
On Jun 29, 2005, at 10:51 AM, Jonathan Malek wrote:
>
> #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
> "SVN_ADM_DIR") ) : (".svn") )
>
> Thanks for taking the time to look at this Ben--I know it's been
> hashed and rehashed, but I think this is a simple solution that works.
The 'cons' of this proposal previously were based on the assumption
that making the value of SVN_ADM_DIR into a runtime option would have
involved using ~/.subversion/config, which would have been a huge
coding change. But now it seems like a tweak to the already existing
#define will accomplish the same task.
The only other 'con' listed was that ASP.NET working copies would be
non-portable. But frankly, that's the world we already live in.
There are already lots of shops out there using 'special' TortoiseSVN
builds which use '_svn' directories. So from where I stand, this
proposal seems like a net gain. Nothing gets any worse, but it means
Tortoise can stop making special releases.
IANTSP -- "I am not the subversion project". Do any other developers
have opinions about this?
---------------------------------------------------------------------
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>.
Andrew, thanks very much for bringing this up. I was getting ready to
send another email.
Ben, yes, it's #1, but the solution is simple, and you're right, the
con no longer exists. The code functions, passes the tests, I've been
using a build to do my work for the past week. The value of this
change is that it affects all clients built on these libraries, and
gives us quite a flexible solution. At some point, you could
introduce a more complicated process, but for now calling getenv()
from the macro works, is simple, and adds flexibility.
It's a tiny change:
#define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
"SVN_ADM_DIR") ) : (".svn") )
Thanks for taking the time to look at this Ben--I know it's been
hashed and rehashed, but I think this is a simple solution that works.
Jonathan Malek
---------------------------------------------------------------------
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 Ben Collins-Sussman <su...@collab.net>.
On Jun 29, 2005, at 8:10 AM, Andrew Thompson wrote:
> Did I miss the discussion about this, or did it not happen?
Since we've discussed this to death in the past, I doubt folks felt
like rehashing over it again. :-)
Here are some notes I have written down from the older discussions:
----------------
1. Make '.svn' into a runtime option.
The dirname is '.svn' by default, or something else if you set a
particular runtime option (either in ~/.subversion/config, or in
an environment variable.)
PROS:
- ASP.NET users have a convenient workaround for IIS: just
set the runtime option to '_svn' or whatever they want.
CONS:
- ASP.NET working copies become non-portable (unless Unix
clients temporarily set the runtime option to the same
thing.)
- large amount of coding work. (converting dirname from a
compile-time variable to a run-time one.)
2. Publish/advertise a one-line patch.
The patch just changes the single '#define .svn' line.
PROS:
- ASP.NET users have a workaround for IIS: recompile with the
patch, or use a 'special' win32 client binary.
- no coding needed.
CONS:
- Every win32 client (svn, tortoisesvn, svnup, etc.),
must now publish 'special' ASP.NET binaries along side the
'normal' binaries.
- ASP.NET working copies become non-portable.
3. "Be strict in what we create, tolerant in what we read."
On win32, create "_svn" (or something similarly visible). On
all other systems, create ".svn". When reading a working copy,
look for both.
PROS:
- ASP.NET users require no workarounds at all, and happily
interoperate with Unix.
- easy to code.
CONS:
- libsvn_wc will do *two* disk stats to figure out if a
directory is under version control. (but only if it's a
Unix client looking at a Win32 working copy, or vice versa.)
4. Go back to using 'SVNADM/' or similar.
PROS:
- works everywhere, full interoperability.
- a one line code change.
CONS:
- no more hidden dirs on Unix. (My sense is that *most* --
but not all -- unix users still prefer hidden directories.)
------------------
So what's being proposed is #1 again. What's interesting is that the
con of 'large amount of coding work' may not be true anymore, if it's
really possible to call getenv() from a macro.
---------------------------------------------------------------------
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 Andrew Thompson <an...@aktzero.com>.
Did I miss the discussion about this, or did it not happen?
Jonathan Malek wrote:
> I know there is quite a bit of resistance to dealing with the issue of
> .svn vs. _svn (or really any other name) as admin directory names,
> especially given the presence of other work-arounds, but it still
> continues to present a roadblock to some of us (especially those who
> have to deal with contractors and developers while also maintaining a
> decent automated build environment).
>
> So while running my usual recompile to create modified executables
> that support "_svn", I decided to try something different. I am
> aiming for something that doesn't change the way the vast majority of
> SVN users work, but allows the poor fellow who really needs "_svn" to
> continue to work.
>
> Instead of changing svn_wc.h with the usual:
>
> #define SVN_WC_ADM_DIR_NAME ".svn"
>
> I decided to fetch the dirname from the environment:
>
> #define SVN_WC_ADM_DIR_NAME ( ( getenv( "SVN_ADM_DIR") ) ? ( getenv(
> "SVN_ADM_DIR") ) : (".svn") )
>
> This works just fine for anyone who hasn't set the environment
> variable, but allows the rest of us to change it without recompiling.
> I'm sure some other devs can come up with better suggestions, but--how
> about it?
>
> Thanks,
> Jonathan Malek
--
Andrew Thompson
http://aktzero.com/