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

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