On Wed, 2009-09-30 at 12:54 +0100, Julian Foad wrote:
> Lieven Govaerts wrote:
> > what's the difference between upper and lower case drive letters on
> > Windows? I thought Windows' use of drive letters was case insensitve?
> >
> > On Wed, Sep 30, 2009 at 10:08 AM, Bert Huijben <rhuijben_at_sharpsvn.net> wrote:
> > > Author: rhuijben
> > > Date: Wed Sep 30 01:08:37 2009
> > > New Revision: 39690
> > >
> > > Log:
> > > Make svn_dirent_is_absolute() only return true when
> > > svn_dirent_get_absolute() called on that same path returns the same value.
>
> But svn_dirent_get_absolute() doesn't specify what it does with a
> lower-case drive letter. Sure, you know what one particular
> implementation of it does.
>
> > > This resolves an issue with lower cased drive letters in Windows, that
> > > where found.
> > >
> > > Found by: rdonch
> > >
> > > * subversion/libsvn_subr/dirent_uri.c
> > > (svn_dirent_is_absolute): Don't assume a lowercase drive letter is absolute.
> > >
> > > * subversion/tests/libsvn_subr/dirent_uri-test.c
> > > (test_dirent_is_absolute): Add two test values, verifying the new behavior and
> > > add verification on svn_dirent_get_absolute() value being the same for absolute
> > > paths.
> > > (test_dirent_get_absolute): Add three test values.
> > >
> > > Modified:
> > > trunk/subversion/libsvn_subr/dirent_uri.c
> > > trunk/subversion/tests/libsvn_subr/dirent_uri-test.c
> > >
> > > Modified: trunk/subversion/libsvn_subr/dirent_uri.c
> > > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_subr/dirent_uri.c?pathrev=39690&r1=39689&r2=39690
> > > ==============================================================================
> > > --- trunk/subversion/libsvn_subr/dirent_uri.c Wed Sep 30 00:21:55 2009 (r39689)
> > > +++ trunk/subversion/libsvn_subr/dirent_uri.c Wed Sep 30 01:08:37 2009 (r39690)
> > > @@ -1499,8 +1499,7 @@ svn_dirent_is_absolute(const char *diren
> > > /* On Windows, dirent is also absolute when it starts with 'H:' or 'H:/'
> > > where 'H' is any letter. */
>
> That comment doesn't match the code: the code requires the "/". (I know
> you didn't change this in this patch.)
I fixed that comment.
> > > #if defined(WIN32) || defined(__CYGWIN__)
> > > - if (((dirent[0] >= 'A' && dirent[0] <= 'Z') ||
> > > - (dirent[0] >= 'a' && dirent[0] <= 'z')) &&
> > > + if (((dirent[0] >= 'A' && dirent[0] <= 'Z')) &&
> > > (dirent[1] == ':') && (dirent[2] == '/'))
> > > return TRUE;
> > > #endif /* WIN32 or Cygwin */
> > >
> > > Modified: trunk/subversion/tests/libsvn_subr/dirent_uri-test.c
> > > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/libsvn_subr/dirent_uri-test.c?pathrev=39690&r1=39689&r2=39690
> > > ==============================================================================
> > > --- trunk/subversion/tests/libsvn_subr/dirent_uri-test.c Wed Sep 30 00:21:55 2009 (r39689)
> > > +++ trunk/subversion/tests/libsvn_subr/dirent_uri-test.c Wed Sep 30 01:08:37 2009 (r39690)
> > > @@ -151,6 +151,8 @@ test_dirent_is_absolute(apr_pool_t *pool
> > > { "/", FALSE },
> > > { "X:/foo", TRUE },
> > > { "X:/", TRUE },
> > > + { "x:/", FALSE },
> > > + { "x:/foo", FALSE },
> >
> > It just seems wrong to me to handle dirents differently on Windows
> > when only their case differs.
>
> It would be OK to make dirent_is_absolute() be undefined or abort on
> non-canonical input, but it is wrong to test that it returns FALSE as
> that is not part of the promised behaviour, just an implementation
> detail.
I mean: I think we should specify in the API doc-string (or just
implicitly) that it is "undefined" for non-canonical input. The
implementation could return FALSE, but I don't think we should be mixing
tests for implementation details with tests for promised behaviour. If
we do, it then becomes very hard to understand the test suite later:
"Why are we testing for THAT?"
> > > { "//srv/shr", TRUE },
> > > { "//srv/shr/fld", TRUE },
> > > { "//srv/s r", TRUE },
> > > @@ -178,6 +180,25 @@ test_dirent_is_absolute(apr_pool_t *pool
> > > "svn_dirent_is_absolute (%s) returned %s instead of %s",
> > > tests[i].path, retval ? "TRUE" : "FALSE",
> > > tests[i].result ? "TRUE" : "FALSE");
> > > +
>
> + /* Check that svn_dirent_get_absolute() returns the path
> + unchanged if it's absolute or changed if it's relative. */
>
> > > + /* Don't get absolute paths for the UNC paths, because this will
> > > + always fail */
>
> Can't svn_dirent_get_absolute() just return a UNC path unchanged?
I mean: is there any reason why we should not define
svn_dirent_get_absolute() as treating a UNC path as "absolute" and
therefore returning it unchanged? And if we agree on that, then we
implement it, and then that exception in the test above can go away.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2402098
Received on 2009-09-30 16:11:24 CEST