Re: svn commit: rev 1993 - trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/libsvn_subr trunk/subversion/libsvn_client trunk/subversion/libsvn_ra_dav

[Greg Stein <gs...@lyra.org>]

On Tue, May 21, 2002 at 05:58:09PM -0500, kfogel@tigris.org wrote:
>...
> +++ trunk/subversion/include/svn_string.h	Tue May 21 17:58:08 2002
>...
> + * If *ARRAY is null, allocate a new array in POOL; else append the
> + * substrings onto existing *ARRAY (in which case note that the copies
> + * are still allocated in POOL, though *ARRAY itself grows in its own
> + * pool).

Eek. Functions like this are always dangerous. People will always forget to
set the value to NULL first. Recall all the stupid problems we had when
apr_file_open() would take an existing file record, or NULL? What a bother
that was.

I would suggest returning the signature to its previous state (return an
array), and adding a new function: svn_cstring_split_append(). Of course,
the "return an array" can be implemented in terms of appending to a new
array.

Consider me around a -0.9 on leaving this "if it is NULL" thing.

>...
> +++ trunk/subversion/libsvn_client/checkout.c	Tue May 21 17:58:09 2002
>...
> +process_externals (svn_stringbuf_t *path, apr_pool_t *pool)
> +{
> +  const svn_string_t *externals;
> +
> +  SVN_ERR (svn_wc_get_wc_prop (path->data, SVN_PROP_EXTERNALS,
> +                               &externals, pool));

Ha! Sucker... you got fooled.

svn_wc_get_wc_prop() is ONLY for WC properties. Fun, huh? :-)

My current position is that svn_wc_get_wc_prop() and svn_wc_set_wc_prop()
should simply go away. The *only* caller is in libsvn_client/ra.c. But those
functions should just call svn_wc_prop_get/set() and that latter function
should use svn_wc_is_wc_prop() to dispatch properly.

And note that svn_wc_get/set_wc_prop() are stupid wrappers around
svn_wc__wcprop_get/set() (meaning that svn_wc_prop_get/set would just call
the internal functions once it has identified the type by name).

> +
> +
> +  if (externals)
> +    {
> +      /* handle_externals_description() needs non-const input */
> +      svn_stringbuf_t *dup = svn_stringbuf_create (externals->data, pool);
> +      SVN_ERR (handle_externals_description (dup->data, path->data, pool));

Euh... the prototype for handle_externals_description takes a 'const char *'
And if you wanted non-const, then screw the stringbuf. Just use apr_pstrdup.

>...
> +        apr_hash_this (hi, &key, &klen, &val);

If klen is not needed, then pass NULL.

> +        ent = (svn_wc_entry_t *) val;

The cast here isn't needed. Casting from void* is automatic.

>...
> +        if ((ent->kind == svn_node_dir)
> +            && (strcmp (ent->name->data, SVN_WC_ENTRY_THIS_DIR) != 0))
> +          {
> +            svn_path_add_component (path, ent->name);
> +            SVN_ERR (process_externals (path, pool));
> +            svn_path_remove_component (path);

I think that process_externals should take a 'const char *' rather than
modifying its argument.

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

Re: svn commit: rev 1993 - trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/libsvn_subr trunk/subversion/libsvn_client trunk/subversion/libsvn_ra_dav

[Karl Fogel <kf...@newton.ch.collab.net>]

Greg Stein <gs...@lyra.org> writes:
> Eek. Functions like this are always dangerous. People will always forget to
> set the value to NULL first. Recall all the stupid problems we had when
> apr_file_open() would take an existing file record, or NULL? What a bother
> that was.
> 
> I would suggest returning the signature to its previous state (return an
> array), and adding a new function: svn_cstring_split_append(). Of course,
> the "return an array" can be implemented in terms of appending to a new
> array.
> 
> Consider me around a -0.9 on leaving this "if it is NULL" thing.

I like your solution better -- will do, thanks!

> Ha! Sucker... you got fooled.
> 
> svn_wc_get_wc_prop() is ONLY for WC properties. Fun, huh? :-)
> 
> My current position is that svn_wc_get_wc_prop() and svn_wc_set_wc_prop()
> should simply go away. The *only* caller is in libsvn_client/ra.c. But those
> functions should just call svn_wc_prop_get/set() and that latter function
> should use svn_wc_is_wc_prop() to dispatch properly.
> 
> And note that svn_wc_get/set_wc_prop() are stupid wrappers around
> svn_wc__wcprop_get/set() (meaning that svn_wc_prop_get/set would just call
> the internal functions once it has identified the type by name).

Aaaaaaargh.  Thanks, Greg :-).

> > +  if (externals)
> > +    {
> > +      /* handle_externals_description() needs non-const input */
> > +      svn_stringbuf_t *dup = svn_stringbuf_create (externals->data, pool);
> > +      SVN_ERR (handle_externals_description (dup->data, path->data, pool));
> 
> Euh... the prototype for handle_externals_description takes a 'const char *'
> And if you wanted non-const, then screw the stringbuf. Just use apr_pstrdup.

Sounds like a braino; will relook at that and DTRT.

> >...
> > +        apr_hash_this (hi, &key, &klen, &val);
> 
> If klen is not needed, then pass NULL.

   self.receive(clue);

> > +        if ((ent->kind == svn_node_dir)
> > +            && (strcmp (ent->name->data, SVN_WC_ENTRY_THIS_DIR) != 0))
> > +          {
> > +            svn_path_add_component (path, ent->name);
> > +            SVN_ERR (process_externals (path, pool));
> > +            svn_path_remove_component (path);
> 
> I think that process_externals should take a 'const char *' rather than
> modifying its argument.

I think I like that more too, even though less efficient in some
trivial way that we shouldn't care about :-).

Will do.

Thanks for the review!

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org