On Tue, Mar 19, 2002 at 07:51:21PM -0600, Karl Fogel wrote:
> 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.
Sure. But a commit is still a reviewable commit :-)
> > 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.
Using pathbuf->data hides the true origin of the data. It also continues to
promote 'pathbuf' in making it look like it is still used.
> > >...
> > > @@ -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. :-)
Ah. Good point. Well, it should still be 'path' at least, for the same
reasons as 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? :-)
Just pointing it out. Not suggesting that you continue doing the ripple
effect :-) But maybe somebody else will view this as a bite-sized task and
go and make the ripple tweaks.
> > >...
> > > +++ 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. :)
They should be changed. Come on, Karl. I did an inspection and saw that
'npath' wasn't needed. And it was pretty clear that it wasn't used. If you
want to challenge even the most obvious changes, then why should I continue
with suggestions?
> > >...
> > > +++ 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.
The WC is littered with stringbufs, and it is continuing to infect the
development that we're doing right now. We're adding more usage of
stringbuf, which is just creating more work for us down the line. Your
change to svn_io_check_path() was instigated because the use of stringbuf
was hampering your own work.
I'm beginning to think that we should clear out most of our stringbuf usage
for M12. There are three primary culpris: WC, libsvn_client, and a bit of
subr. There will be ripple back into clients/cmdline/, and down into RA and
repos.
I've found that with stringbuf changes, the easiest thing to do is start at
the bottom. Callers can always pass ->data if they have a stringbuf.
It is also possible to go from the top, change an API, and just create a
stringbuf internally for further calls. However, that creates the "aliasing"
effect like I pointed out above for path vs pathbuf->data. Aliasing is
generally a bad thing (IMO) for long term maintenance, readability, and
understanding.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Mar 20 03:08:49 2002