On Tue, 2004-12-14 at 14:35, Peter N. Lundblad wrote:
> On Tue, 14 Dec 2004, VK Sameer wrote:
>
> In addition to Karl's review:
>
> + * returns SVN_NO_ERROR if valid
> + * returns SVN_ERR_FS_PATH_SYNTAX if invalid
>
>
> Not sure how this will be formatted in the doxygen output. I assume it
> won't take the line breaks into account, but I haven't checked.
Yes, you're right, it won't. Changed to one sentence.
>
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 12257)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -29,6 +29,7 @@
> #include "svn_private_config.h" /* for SVN_PATH_LOCAL_SEPARATOR */
> #include "svn_utf.h"
> #include "svn_io.h" /* for svn_io_stat() */
> +#include "svn_ctype.h"
>
>
> /* The canonical empty path. Can this be changed? Well, change the empty
> @@ -1248,3 +1249,22 @@
> else
> return svn_utf_cstring_to_utf8 (path_utf8, path_apr, pool);
> }
> +
> +svn_error_t*
>
>
> Space after *.
Hmm, didn't see it, unless you meant before? Did add that.
> +svn_path_check_valid (const char *path, apr_pool_t *pool)
> +{
> + const char *c;
> +
> + for (c = path; *c; c++)
> + {
> + if (svn_ctype_iscntrl(*c))
> + {
> + return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, NULL,
> + "Invalid control char. '%x' in path '%s'",
> + *c,
> + svn_path_local_style (path, pool));
>
>
> Since 1.1, svn is internationalized, so you need to put strings that
> should be translated into the users' native language inside _() macro
> calls. + }
OK.
> Index: subversion/libsvn_ra_local/ra_plugin.c
> ===================================================================
> --- subversion/libsvn_ra_local/ra_plugin.c (revision 12257)
> +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> @@ -688,6 +688,9 @@
> svn_fs_root_t *root;
> const char *abs_path = sbaton->fs_path;
>
> + if (path)
> + SVN_ERR (svn_path_check_valid (path, pool));
> +
>
>
> svn_ra_plugin_t->check_path, which this implements, takes a path, checks
> if it exists (in a particular revision) and what kind it is. So you don't
> need the above.
OK, thanks for double-checking the call sites.
> I see that you are adding the checks to both libsvn_wc and libsvn_client.
> Isn't it enough to add them to libsvn_wc (except in the import code)? For
> example, wc_to_wc_copy it seems redundant. Or maybe you are plugging some
> hole somewhere else?
No, I haven't found a hole. Initially, the plan was to put the checks
only in the libsvn_client layer to catch errors as close to the user as
possible. Since I hadn't reviewed all the paths carefully, I added
libsvn_wc based on Jon's patch. If libsvn_wc is a better layer in terms
of greater coverage in fewer call sites, I can remove the checks from
the libsvn_client layer.
Thanks for the review.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 14 12:14:22 2004