Greg Stein <gstein@lyra.org> writes:
> A couple bugs and some varied suggestions...
All the suggestions are good, but this basically boils down to "Today
is config day, not wc day." :-). I'll address a few of them below,
that are directly related to the svn_io_check_path() change. That
change was made because I was writing new calls to svn_io_check_path
in the config code, and wanted to avoid generating *new* stringbuf
lossage. Old lossage is still lossage, of course, but at least no
more than was there before.
> On Tue, Mar 19, 2002 at 06:22:40PM -0600, kfogel@tigris.org wrote:
> >...
> > +++ trunk/subversion/libsvn_wc/props.c Tue Mar 19 18:22:37 2002
> >...
> > @@ -778,7 +775,7 @@
> > *props = apr_hash_make (pool);
> >
> > /* Check validity of PATH */
> > - SVN_ERR( svn_io_check_path (pathbuf, &kind, pool) );
> > + SVN_ERR( svn_io_check_path (pathbuf->data, &kind, pool) );
>
> That should just be 'path' rather than pathbuf->data.
Yah. But `pathbuf' is still used in that function :-(. Since can't
eliminate it, I'd prefer to just use it consistently rather than make
a larger code change. Could go either way.
> >...
> > @@ -910,7 +907,7 @@
> > SVN_ERR (svn_wc__prop_path (&prop_path, pathbuf, 0, pool));
> >
> > /* Does the property file exist? */
> > - SVN_ERR (svn_io_check_path (prop_path, &pkind, pool));
> > + SVN_ERR (svn_io_check_path (prop_path->data, &pkind, pool));
>
> This should be 'path' with the corresponding elimination of 'pathbuf'
Can't eliminate `pathbuf', it's still used two lines above. :-)
> With this change, 'src_wprop' and 'src_bprop' are never used as a stringbuf.
> If svn_wc__prop_path() were to return a 'const char **', then you could
> eliminate the usage of a stringbuf.
True.
One has to draw the line somewhere, though. What if changing
svn_wc__prop_path() leads to more follow-up changes, just like
svn_io_check_path() did? :-)
> >...
> > +++ trunk/subversion/libsvn_wc/util.c Tue Mar 19 18:22:37 2002
> > @@ -38,7 +38,7 @@
> > {
> > enum svn_node_kind kind;
> > svn_stringbuf_t *npath = svn_stringbuf_dup (path, pool);
> > - svn_error_t *err = svn_io_check_path (npath, &kind, pool);
> > + svn_error_t *err = svn_io_check_path (npath->data, &kind, pool);
>
> The 'npath' variable isn't needed. Just use the 'path' parameter.
`npath' is used in other places in that function (and not just for
npath->data). Sure, they could all be changed, but... See above. :)
> >...
> > +++ trunk/subversion/libsvn_wc/adm_crawler.c Tue Mar 19 18:22:37 2002
> > @@ -193,7 +193,7 @@
> > svn_wc__text_base_path (svn_stringbuf_create ((char *)key, pool),
> > TRUE, pool);
> >
> > - SVN_ERR (svn_io_check_path (tmpfile_path, &kind, pool));
> > + SVN_ERR (svn_io_check_path (tmpfile_path->data, &kind, pool));
>
> This changes the last reference of tmpfile_path to be non-buf. If
> svn_wc__text_base_path() were to return a 'const char *', then you wouldn't
> need a buf at all.
Yes. Ditto on the rest.
I'm not sure it's worth scheduling a WC cleanup milestone, but if we
did, we'd certainly have enough to do. :-) If there were an issue for
this, your email should be linked from it.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 20 02:43:06 2002