[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] issue 1954 - try 2

From: <kfogel_at_collab.net>
Date: 2004-12-10 22:55:35 CET

Philip Martin <philip@codematters.co.uk> writes:
> The reason to have a function return a boolean is because we expect
> the "error" to occur, we intend to handle it, and we don't want the
> overhead of creating and destroying an error. I don't think that
> applies to this stuff, the error is likely to be irrecoverable. So I
> guess we don't need the boolean function.

We can dispense with the boolean function for now, and just have one
public interface, that returns a specific SVN_ERR_ error if the path
is invalid. If someday we want to ask the question in a
non-error-generating way, we can add the boolean interface then.

> The function name svn_path_error_if_invalid is ugly.

How about svn_path_check_valid()?

I would prefer not to use up the intuitive boolean interface name
"svn_path_is_valid" on an error-generating function. The vast
majority of Subversion uses

   svn_foo_is_bar()

for a standard boolean-by-reference form. If we ever want to add a
boolean interface for path validity, we'll want that name available.
Therefore, let's give the error-generating version a different name.

It doesn't have to be "svn_path_error_if_invalid". As long as it's
*not* "svn_path_is_valid", I'm happy :-). "svn_path_check_valid" is
fine, no doubt there are many other possible names.

> I'm guessing we don't need the check_utf8 flag. You have already
> added a new public interface to the existing check-UTF-8 function, so
> all we really need is a new check-control-character-function, one that
> returns an error. Where it is know that the path is UTF-8 just call
> the second, if the path is not known to be UTF-8 call both.

I think it would be good to have a single svn_path_foo() function to
do this. I'm assuming it would look something like this:

   svn_error_t *
   svn_path_check_valid (const char *path, apr_pool_t *pool)
   {
     svn_error_t *err;

     err = svn_utf_check_cstring_utf8 (path, pool);
     if (err && (err->apr_err == SVN_ERR_BLAH_NOT_UTF8))
       {
         return svn_error_createf
                (SVN_ERR_PATH_INVALID,
                "Path '%s' is not encoded in UTF8",
                svn_some_fuzzy_conversion_function (path, pool));
       }
     else if (err)
       {
         return err;
       }

     err = svn_utf_check_cstring_no_controls (path, pool);
     if (err && (err->apr_err == SVN_ERR_BLAH_HAS_CONTROL_CHARS))
       {
         return svn_error_createf
                (SVN_ERR_PATH_INVALID,
                "Path '%s' is not encoded in UTF8",
                svn_some_fuzzy_conversion_function (path, pool));
       }
     else if (err)
       {
         return err;
       }

     return SVN_NO_ERROR;
   }

(Untested code, and the error names are improvised, of course).

True, this ends up resulting in three new public interfaces:

   svn_utf_check_cstring_utf8()
   svn_utf_check_cstring_no_controls()
   svn_path_check_valid()

But the calling sites would become very readable -- they wouldn't even
need comments, because the name of the function will say everything
that needs to be said. That seems like a bargain.

-Karl

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Dec 10 22:56:47 2004

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.