Some review comments.
On 8/26/06, lgo@tigris.org <lgo@tigris.org> wrote:
> --- trunk/subversion/libsvn_client/commit.c (original)
> +++ trunk/subversion/libsvn_client/commit.c Sat Aug 26 13:33:01 2006
> * subversion/libsvn_client/commit.c
> (svn_client_commit4): make sure to abort when the iterating through the
> parent folders passese the root folder.
> [...]
>
> while (strcmp(target, base_dir) != 0)
> {
> - if ((target[0] == '/' && target[1] == '\0') ||
> - (target[0] == '\0'))
> + if ((target[0] == '\0') ||
> + svn_path_is_root(target, strlen(target), subpool)
> + )
> abort();
Is this change really necessary? This is only a debug check. It seems
to be overkill to check for UNC paths and drive letters here,
especially given that svn_path_is_root is expensive.
> {
> return (! SVN_PATH_IS_PLATFORM_EMPTY(path, len)
> - && (len <= 1 || path[len-1] != '/'));
> + && (svn_path_is_root(path, len, NULL) ||
> + (len <= 1 || path[len-1] != '/')));
> }
This change to is_canonical makes '//server/share/' canonical.
Shouldn't the canonical version of this path be //server/share'?
Normally we do not include the trailing slash in canonical URLs.
> - if (blen == 1 && base[0] == '/')
> - blen = 0; /* Ignore base, just return separator + component */
> + add_separator = 1;
> + if (svn_path_is_root(base, blen, pool))
> + add_separator = 0; /* Ignore base, just return separator + component */
This change to svn_path_join won't work with UNC paths. For example,
if we join //server/blah and file.txt, we'll get
//server/blahfile.txt. We need a separator there.
Would this implementation work better?
if (base[blen-1] != '/' && base[blen-1] != ':')
add_separator = 1;
> - if (total_len == 1 && *base == '/')
> - base_is_root = TRUE;
> - else if (SVN_PATH_IS_EMPTY(base))
> + base_is_root = svn_path_is_root(base, total_len, pool);
> + if (!base_is_root &&
> + (SVN_PATH_IS_EMPTY(base)))
We don't need this extra check for 'base_is_root' here. If the path is
empty, it won't be a root.
> - if (len == 0 && path[0] == '/')
> - return 1;
> + /* check if the remaining segment including trailing '/' is a root path */
> + if (svn_path_is_root(path, len + 1, NULL))
> + return len + 1;
> else
> return len;
> }
This implementation is a bit strange, and will return strange results
for UNC paths. For instance, if you get the 'dirname' of
'//server/share/blah.txt', it will return '//server/share/'. Why don't
we just check for whether the last character (before the '/') in the
path is a colon?
> that non-matching byte. */
> if (((i == path1_len) && (path2[i] == '/'))
> || ((i == path2_len) && (path1[i] == '/'))
> - || ((i == path1_len) && (i == path2_len)))
> + || ((i == path1_len) && (i == path2_len))
> + || svn_path_is_root(path1, i, pool))
> return i;
This method (get_path_ancestor_length) also needs to be tweaked to
return URLs which don't end in a trailing slash (e.g. //server/share
instead of //server/share/)
Cheers,
David
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Oct 3 00:30:40 2006