On Fri, 28 Oct 2005, Philip Martin wrote:
> Daniel Rall <dlr@finemaltcoding.com> writes:
>
> > Turns out the call to svn_path_basename() from svn_wc_is_adm_dir() was
> > a local hack I had in my working copy which was requested by the
> > Subclipse folks (I didn't see any comments on the patch when I last
> > posted it to list, but could've missed them). Any objections to me
> > committing both patches? I do have some concern that the previously
> > super-thin svn_wc_is_adm_dir() routines becomes heavier because of
> > this change. Thoughts?
>
> Subeclipse can call svn_path_basename before calling
> svn_wc_is_adm_dir, is there some reason that it's better for
> Subversion to do it?
Well, Subclipse actually has no access to svn_path_basename(), and in
SVN 1.3 there is not even documentation indicating that this is
required. What Subclipse can do is use java.io.File to acquire the
base name of the directory:
http://java.sun.com/j2se/1.3/docs/api/java/io/File.html#getName()
Alternately, the bindings could each perform svn_path_basename() (or
the language-specific equivalent). I'm not particularly in favor of
the irregularity nor extra code involved in this approach.
> On the whole I tend to favour the existing interface.
This came up in the first place because svn_wc_is_adm_dir() was not
documented as to whether it acted on a path or single component of a
directory name. Both Mark Phippard and myself individually ran into
this question while interacting with the new API. I have subsequently
clarified the API's doc string in trunk.
> Obviously you need to update the header file as well.
>
> > Index: subversion/libsvn_wc/adm_files.c
> > ===================================================================
> > --- subversion/libsvn_wc/adm_files.c (revision 17065)
> > +++ subversion/libsvn_wc/adm_files.c (working copy)
> > @@ -57,7 +57,7 @@
> > svn_boolean_t
> > svn_wc_is_adm_dir (const char *name, apr_pool_t *pool)
>
> Use 'path' rather than 'name'.
>
> > {
> > - (void)pool; /* Silence compiler warnings about unused parameter. */
> > + name = svn_path_basename (name, pool);
> > return (0 == strcmp (name, adm_dir_name)
> > || 0 == strcmp (name, default_adm_dir_name));
> > }
Yeah, both these changes would be necessary, thanks Philip. However,
with the documentation for the function fixed, I'm also ambivalent on
this change. Seems like with as often as the function is called,
there are some moderate performance benefits to leaving it as-is.
Mark, do you think this is still necessary?
--
Daniel Rall
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Oct 28 23:05:58 2005