Greg Stein <gstein@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
Received on Wed May 22 19:17:08 2002