Re: svn commit: r21277 - in trunk/subversion: include libsvn_client libsvn_subr tests/libsvn_subr

[David James <dj...@collab.net>]

Some review comments.

On 8/26/06, lgo@tigris.org <lg...@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