On Tue, 2004-12-07 at 07:40, kfogel@collab.net wrote:
> VK Sameer <sameer@collab.net> writes:
>
> Run 'svn log' on the Subversion trunk to see examples of our usual log
> formatting style. See also the section "Writing log messages" in the
> HACKING file.
Sorry, I didn't read it carefully enough.
> Here is how I would write it:
>
> Resolve issue #1954: Error on add or import of a path that is
> invalid in Subversion.
>
> ####################################################
> ### ###
> ### Note: This is a draft patch for review only, ###
> ### please do not commit it. ###
> ### ###
> ####################################################
Definitely missed this :)
> * subversion/include/svn_path.h,
> subversion/libsvn_subr/path.c
> (svn_path_is_valid_in_svn): New function.
Ah, a much nicer format than some of the 'svn log' output on trunk.
Multiple functions were not listed on separate lines.
[snip]
> (My comments will be similar to what others have said already in
> response to this patch.)
>
> Your doc string describes the implementation. Instead, it should
> describe the *semantics* -- the behavior of the function. For
> example, what happens if the path is not valid? Is an error returned,
> and if so, which error code?
OK, make sense.
> Here's a new interface, with a new doc string:
>
> /**
> * @since New in 1.2.
> *
> * Set @a *is_valid to TRUE if @a path is a valid Subversion path,
> * else set it to FALSE. Use @a pool for temporary allocation.
> *
> * Valid Subversion paths are UTF-8 strings with no control characters
> * except for SPACE (0x20). "Valid" means Subversion can store
> * it in a repository. There may be other, OS-specific limitations
> * on what paths can be represented in a working copy.
> */
> svn_error_t *svn_path_is_valid (svn_boolean_t *is_valid,
> const char *path,
> apr_pool_t *pool);
>
> (Note also that I shortened the function name.)
OK, will change.
> I'm not sure whether you need to return 'svn_error_t *' at all or not
> -- it partly depends on what you'll be calling. If it turns out that
> an error can happen, then add a paragraph describing the
> circumstances, and (ideally) the error codes.
Changing to return svn_boolean_t.
> The reason I changed the prototype is that the path being invalid is
> *not* an error :-). After all, determining whether or not a path is
> valid is exactly what function is meant do. So if it successfully
> determines that, that shouldn't be an error condition.
I'll implement the error generating function you've suggested below as a
way to raise errors and just return a TRUE/FALSE value here.
> More practically, every svn_error_t we manufacture is allocated in a
> global pool. If someone were to call your function many times on
> invalid paths, then we'd have needless memory growth.
That's good to know. I'd been proceeding on the assumption that a client
would error-exit, but just realized that a GUI client would leak memory.
[snip]
> You don't need to say "for svn_ctype_()", it's obvious from the header
> name.
OK.
> You've already seen Philip's comments about testing byte-by-byte.
> UTF-8 is a multibyte encoding. I'm not sure how familiar you are with
> it, but http://svn.red-bean.com/repos/main/3bits/utf8_xml.txt
> summarizes how it works. I'm posting it here because I've been hoping
> Brane or someone would proofread it anyway :-).
Thank you for the link, I was way off-base in my use of svn_ctype_*
functions :( Well, at least for multi-byte UTF8 characters.
> Anyway, you should use the existing svn_utf_* functions, and even
> write new ones if you need them.
Looks like I'll need to add a new public svn_utf_ function around
check_cstring_utf8 then.
> Why wrap the error in another error that contains basically the same
> information? Just "return err;" :-).
Caught me :) trying to decide between returning a boolean and an error.
> I don't think you need these comments explaining each call anyway (the
> name of the function does that quite well). But if you are going to
> put a comment, then write it in terms of semantics, not
> implementation, i.e.,
>
> /* Make sure this path is valid. */
OK.
> > + SVN_ERR (svn_io_check_path (path, &kind, pool));
>
> You appear to have added this call to svn_io_check_path(). Is it part
> of some other change? It's not mentioned in the log message.
Sorry, missed taking it out (part of attempt to check
characters at the lowest-possible level, turned out to be too
restrictive)
> Thanks! And have patience... several rounds of review are not
> unusual :-).
Thanks for the careful review. Considering the number of scattered
changes, I'm glad the code is being inspected exhaustively.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Dec 8 02:20:54 2004