You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "D.J. Heap" <dj...@gmail.com> on 2007/03/13 12:08:04 UTC
svnadmin dump/verify/load uses svn_path_join
I've run into a rather nasty issue with a repository using trunk code.
I've attached a test repository that works fine in 1.4.x or earlier
but croaks when loading with trunk code built on Windows.
The core issue seems to be that the dump/verify/load code uses
svn_path_join on url's and svn_path_join now seems to be assuming that
it is working with filesystem paths, not urls.
This means that in Win32 builds, 'svnadmin dump/load/verify' asserts
and aborts for repositories that have urls such as '/c:hi' (it can't
be checked out on Windows, of course, but that is a client/filesystem
issue). It is probably not as much of an issue on Unix-type OS'
because their filesystem paths look a lot like urls.
Should 'svnadmin dump/verify/load' not be using svn_path_join or
should svn_path_join work with urls?
DJ
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 3/19/07, Lieven Govaerts <sv...@mobsol.be> wrote:
> basically what they are right now. There may be no need to convert all
> of them, I'll look into that after 1.5 is branched
I thought we came to the consensus to hold off 1.5 and have everyone
focus on merge tracking? That's certainly how I interpreted the
recent threads... -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by "D.J. Heap" <dj...@gmail.com>.
On 3/19/07, D.J. Heap <dj...@gmail.com> wrote:
[snip]
>
> Yes, you're right -- from the 4 usage styles it looked like we needed
> more at first glance, but we just convert local dirents to internal at
> the outer API's and IIRC we require repos paths to be valid uris when
> passed in to API's.
Actually, we don't even convert dirents at the API level -- we expect
those to be in internal style already.
DJ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by "D.J. Heap" <dj...@gmail.com>.
On 3/19/07, Lieven Govaerts <sv...@mobsol.be> wrote:
[snip]
> Won't dirent and uri be sufficient? The uri and the paths inside the
> repository have the same structure, no need to make a difference there.
Yes, you're right -- from the 4 usage styles it looked like we needed
more at first glance, but we just convert local dirents to internal at
the outer API's and IIRC we require repos paths to be valid uris when
passed in to API's.
DJ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Lieven Govaerts <sv...@mobsol.be>.
D.J. Heap wrote:
> On 3/14/07, Lieven Govaerts <sv...@mobsol.be> wrote:
>> Justin Erenkrantz wrote:
> [snip]
>> > Is it worth to standardize on (say) _dirent_ or _uri_ to avoid the
>> > _path_ ambiguity? I've never cared for the _path_ name to begin with.
>> That's probably the first thing we need to do, standardize the naming.
>> Some path functions use 'uri' already btw, so I propose to keep using
>> that term.
>>
> Yes, we need to get the various path-names straightened out and the
> above sounds good to me. I guess we'd have something like
> _local_dirent_, _internal_dirent_, _repos_uri_, and
> _internal_repos_uri to match the different usages?
Reverted the windows path handling code in r23907.
Won't dirent and uri be sufficient? The uri and the paths inside the
repository have the same structure, no need to make a difference there.
I left the svn_path_is_root function in the code, so if I use above
conventions I'll rename it to svn_path_dirent_is_root before 1.5 will be
branched. I don't think any of the currently existing functions will
change in meaning, so I won't have to rev any of the functions.
There's probably some 20 svn_path functions that need to be deprecated
and replaced by svn_path_dirent and svn_path_uri functions. I can let
the deprecated svn_path* functions forward to svn_path_uri_*, which is
basically what they are right now. There may be no need to convert all
of them, I'll look into that after 1.5 is branched
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by "D.J. Heap" <dj...@gmail.com>.
On 3/14/07, Lieven Govaerts <sv...@mobsol.be> wrote:
> Justin Erenkrantz wrote:
[snip]
> > Is it worth to standardize on (say) _dirent_ or _uri_ to avoid the
> > _path_ ambiguity? I've never cared for the _path_ name to begin with.
> That's probably the first thing we need to do, standardize the naming.
> Some path functions use 'uri' already btw, so I propose to keep using
> that term.
>
Yes, we need to get the various path-names straightened out and the
above sounds good to me. I guess we'd have something like
_local_dirent_, _internal_dirent_, _repos_uri_, and
_internal_repos_uri to match the different usages?
DJ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Lieven Govaerts <sv...@mobsol.be>.
Justin Erenkrantz wrote:
> On 3/13/07, Lieven Govaerts <sv...@mobsol.be> wrote:
>> To be honest, I don't see many other options than to revert the whole
>> windows path thing and start from scratch on a feature branch. We'd have
>> to work out all different usage patterns of the path functions, create
>> new path functions for native, url, internal paths, and start using them
>> throughout the code. To test all that a new test case has to be created
>> to test all possible svn manipulations on 'c:hi' folders.
>>
>> What do you think?
>
> Sounds reasonable, but the resolution probably should be a blocker for
> 1.5, IMO.
I have a patch ready that removes all the new windows specific code in
path.c. There's some small cleanup left to do in the path tests, I added
those as part of the windows path patches but want to keep them now
atleast for uri's and unix paths. I suppose that's what you mean by
resolution?
> Is it worth to standardize on (say) _dirent_ or _uri_ to avoid the
> _path_ ambiguity? I've never cared for the _path_ name to begin with.
That's probably the first thing we need to do, standardize the naming.
Some path functions use 'uri' already btw, so I propose to keep using
that term.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 3/13/07, Lieven Govaerts <sv...@mobsol.be> wrote:
> To be honest, I don't see many other options than to revert the whole
> windows path thing and start from scratch on a feature branch. We'd have
> to work out all different usage patterns of the path functions, create
> new path functions for native, url, internal paths, and start using them
> throughout the code. To test all that a new test case has to be created
> to test all possible svn manipulations on 'c:hi' folders.
>
> What do you think?
Sounds reasonable, but the resolution probably should be a blocker for 1.5, IMO.
Is it worth to standardize on (say) _dirent_ or _uri_ to avoid the
_path_ ambiguity? I've never cared for the _path_ name to begin with.
-- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Lieven Govaerts <sv...@mobsol.be>.
Jonathan Gilbert wrote:
>
>> Now take this example:
>> c:\svn\wc\> svn diff G:test.txt
>>
>> If 'G:test.txt' would be considered a relative path, then Subversion
>> would think 'c:\svn\wc\G:test.txt' is the absolute path for test.txt,
>> which clearly is incorrect. So for practical reasons we consider
>> 'G:test.txt' as an absolute path, but you're right in saying
>> conceptually it's actually relative.
>>
>
> The C standard doesn't acknowledge this, obviously; getcwd() can only return a
> single value. Even within the Win32 API, GetCurrentDirectory() behaves like
> getcwd(), but there is an alternate function GetFullPathName() which can be
> used to find the current directory on other drives (one passes it a drive
> letter and a colon; the current directory on that drive is then assumed).
> The fact is, though, that Subversion (or the platform layer it is upon) do
> not consider the possibility of multiple filesystem roots, and thus the API
> functions involved don't have any way of taking these into consideration.
>
Interesting. As far as I'm concerned we do not need to know the cwd of
other drives. If the user refers to the cwd on drive G as 'G:', then the
output of svn should contain 'G:test.txt', not G:\svn\wc1\test.txt.
> To complicate things, Windows considers a UNC path specifying a share name
> to be a valid "drive letter" (though applications do not remember current
> directories relative to these beyond where the current directory *is* on a
> UNC path). That is, you can call
> SetCurrentDirectory("\\\\SERVER\\Share\\Path\\To\\Directory\\"), and
> "\\\\SERVER\\SHARE" is considered to be the "drive-letter" part. A
> subsequent call to SetCurrentDirectory("\\Another\\Directory\\") (note the
> lack of a drive letter) will be processed as relative to
> "\\\\SERVER\\SHARE". I don't know if this requires any special handling
> within Subversion, or if it "just works" because the values that filter
> down to the API happen to do the right thing. I'm fairly certain Windows is
> also the only OS with this complication as well, though :-)
>
Subversion never sets the current directory, but it should support unc
paths, represented internally as //server/share. In fact, up until
r23907, svn's path functions recognized Windows folders, UNC paths etc.
I added that feature and tests on trunk to get issues 1711 and 2556 fixed.
Problem is (and this is what this thread is all about) that the path
functions are used for directories - which should have Windows specific
behavior - and uri's without distinction between the two. So if someone
puts a file called 'c:hi' in the root of a repository using a linux
client, and then dumps or verifies the repository on Windows, svn
pre-r23907 will fail.
My changes handled things a bit differently than yours, if you like you
can check out r23907 to see how it used to work. This code will come
back later, as functions handling directories or uri's, but not both at
the same time.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Jonathan Gilbert <o2...@sneakemail.com>.
At 02:47 PM 3/24/2007 +0100, Lieven Govaerts wrote:
>>>> How is "c:hi" an absolute path on Windows? Isn't "c:hi" really the
>>>> equivalent of "c:.\hi"?
>>>
>>> Depending on the way you look at 'c:hi' it's both absolute
>>> and relative.
>>> Your example makes it look like a relative path. However, a
>>> relative path can be added to the current working directory
>>> to make an absolute path, but you can't do that with 'c:hi'.
[snip]
>Now take this example:
>c:\svn\wc\> svn diff G:test.txt
>
>If 'G:test.txt' would be considered a relative path, then Subversion
>would think 'c:\svn\wc\G:test.txt' is the absolute path for test.txt,
>which clearly is incorrect. So for practical reasons we consider
>'G:test.txt' as an absolute path, but you're right in saying
>conceptually it's actually relative.
The problem here is evidently that Windows has a "feature" that other
operating systems don't have: It has multiple filesystem roots, and each
root gets its own current directory for a given application. The
application also has a "current drive", conceptually, which identifies
which of the CWDs across all the drives is actually the current one. The C
standard doesn't acknowledge this, obviously; getcwd() can only return a
single value. Even within the Win32 API, GetCurrentDirectory() behaves like
getcwd(), but there is an alternate function GetFullPathName() which can be
used to find the current directory on other drives (one passes it a drive
letter and a colon; the current directory on that drive is then assumed).
The fact is, though, that Subversion (or the platform layer it is upon) do
not consider the possibility of multiple filesystem roots, and thus the API
functions involved don't have any way of taking these into consideration.
On Windows, the function to concatenate paths must be smart enough to
recognize drive letters at the start of a path and not use the left-hand
"parent directory" argument at all. Having detected that, if there is no
path separator character (Windows permitting both '/' and '\\'), if an
absolute path is desired by the caller, the path then needs to be expanded
against the drive letter that was provided (the aforementioned
GetFullPathName() API does this as well).
To complicate things, Windows considers a UNC path specifying a share name
to be a valid "drive letter" (though applications do not remember current
directories relative to these beyond where the current directory *is* on a
UNC path). That is, you can call
SetCurrentDirectory("\\\\SERVER\\Share\\Path\\To\\Directory\\"), and
"\\\\SERVER\\SHARE" is considered to be the "drive-letter" part. A
subsequent call to SetCurrentDirectory("\\Another\\Directory\\") (note the
lack of a drive letter) will be processed as relative to
"\\\\SERVER\\SHARE". I don't know if this requires any special handling
within Subversion, or if it "just works" because the values that filter
down to the API happen to do the right thing. I'm fairly certain Windows is
also the only OS with this complication as well, though :-)
Pseudocode for a Win32 combine_path function which takes drive letters into
account and always returns an absolute path:
bool starts_with_drive_letter(string path)
{
return (path[0].IsLetter && (path[1] == ':'));
}
bool starts_with_unc_share(string path)
{
// could definitely be improved:
return path.StartsWith("\\\\");
}
string extract_unc_share(string path)
{
int third_backslash = path.IndexOf('\\', 2); // skip the first two '\\'s
if (third_backslash < 0)
return path;
int fourth_backslash = path.IndexOf('\\', third_backslash + 1);
if (fourth_backslash < 0)
return path;
return path.Substring(0, fourth_backslash);
}
string combine_path(string base, string subpath)
{
if (starts_with_drive_letter(subpath)
|| starts_with_unc_share(subpath))
return GetFullPathName(subpath);
else
{
if (subpath.StartsWith('/' or '\\'))
{
if (starts_with_unc_share(base))
return combine_path(
extract_unc_share(base),
subpath.Substring(1));
else if (starts_with_drive_letter(base))
return base.Substring(0, 2) + subpath;
else
return combine_path(
GetFullPathName(base),
subpath);
}
else
{
if (base.EndsWith('/' or '\\'))
return base + subpath;
else
return base + '\\' + subpath;
}
}
}
I don't think I've missed any cases, but the code is obviously untested ;-)
Hopefully someone can find this helpful.
Jonathan Gilbert
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Lieven Govaerts <sv...@mobsol.be>.
Paul Burba wrote:
> Hi Lieven,
>
> svn_dirent_is_absolute is all that is required at least in the problem I
> mentioned above. Note that the workaround I have in place is only
> needed in once place, so the impact is not currently widespread.
>
I'll make a patch for svn_dirent_is_absolute and send it to the list, I will only need it later when adapting the path functions to dirent/uri.
>>> How is "c:hi" an absolute path on Windows? Isn't "c:hi" really the
>>> equivalent of "c:.\hi"?
>>>
>>>
>> Depending on the way you look at 'c:hi' it's both absolute
>> and relative.
>> Your example makes it look like a relative path. However, a
>> relative path can be added to the current working directory
>> to make an absolute path, but you can't do that with 'c:hi'.
>> So for practical reasons 'c:hi'
>> type of paths are considered absolute.
>>
>
> Heh, you lost me there, but it's late am and I'm a bit fried. Could you
> maybe give me an example where "c:hi" would be absolute? I'm just not
> seeing it :-\
>
>
It was 3:40AM when I wrote that mail, no wonder you don't see it ;)
What I'm saying is this: an absolute path = current working directory +
a relative path. So suppose I do this:
c:\svn\wc\> svn diff test.txt
In this example Subversion can get the full path of test.txt by
concatenating 'c:\svn\wc\' and 'test.txt'. I don't know if 'svn diff'
actually uses the absolute path, but this is basically what
svn_path_join does.
Now take this example:
c:\svn\wc\> svn diff G:test.txt
If 'G:test.txt' would be considered a relative path, then Subversion
would think 'c:\svn\wc\G:test.txt' is the absolute path for test.txt,
which clearly is incorrect. So for practical reasons we consider
'G:test.txt' as an absolute path, but you're right in saying
conceptually it's actually relative.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svnadmin dump/verify/load uses svn_path_join
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Lieven Govaerts [mailto:svnlgo@mobsol.be]
> Sent: Friday, March 23, 2007 10:40 PM
> To: Paul Burba
> Cc: D.J. Heap; Subversion Development
> Subject: Re: svnadmin dump/verify/load uses svn_path_join
>
> Paul Burba wrote:
> > Hi Lieven,
> >
> > While updating the merge tracking branch I ran into some
> fallout from
> > r23907 because we were using svn_path_is_absolute(). No
> big deal, we
> > can work around that easily enough, but reading this thread
> one thing
> > caught my eye:
> >
> Do you need svn_dirent_is_absolute or svn_uri_is_absolute? I
> rather add one of those again instead of you needing to rely
> on workarounds.
Hi Lieven,
svn_dirent_is_absolute is all that is required at least in the problem I
mentioned above. Note that the workaround I have in place is only
needed in once place, so the impact is not currently widespread.
> > How is "c:hi" an absolute path on Windows? Isn't "c:hi" really the
> > equivalent of "c:.\hi"?
> >
> Depending on the way you look at 'c:hi' it's both absolute
> and relative.
> Your example makes it look like a relative path. However, a
> relative path can be added to the current working directory
> to make an absolute path, but you can't do that with 'c:hi'.
> So for practical reasons 'c:hi'
> type of paths are considered absolute.
Heh, you lost me there, but it's late am and I'm a bit fried. Could you
maybe give me an example where "c:hi" would be absolute? I'm just not
seeing it :-\
Paul B.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Lieven Govaerts <sv...@mobsol.be>.
Paul Burba wrote:
> Hi Lieven,
>
> While updating the merge tracking branch I ran into some fallout from
> r23907 because we were using svn_path_is_absolute(). No big deal, we
> can work around that easily enough, but reading this thread one thing
> caught my eye:
>
Do you need svn_dirent_is_absolute or svn_uri_is_absolute? I rather add
one of those again instead of you needing to rely on workarounds.
> How is "c:hi" an absolute path on Windows? Isn't "c:hi" really the
> equivalent of "c:.\hi"?
>
Depending on the way you look at 'c:hi' it's both absolute and relative.
Your example makes it look like a relative path. However, a relative
path can be added to the current working directory to make an absolute
path, but you can't do that with 'c:hi'. So for practical reasons 'c:hi'
type of paths are considered absolute.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Paul Burba <pb...@collab.net>.
> -----Original Message-----
> From: Lieven Govaerts [mailto:svnlgo@mobsol.be]
> Sent: Tuesday, March 13, 2007 5:46 PM
> To: D.J. Heap
> Cc: Subversion Development
> Subject: Re: svnadmin dump/verify/load uses svn_path_join
<snip>
> So, the scope of the problem we're facing is that the word
> 'path' in the path functions can mean four things:
> 1.native directory/file names, (C:\temp) etc.
> 2. canonicalized native directory/file names (C:/temp) 3.
> urls (file:///c:/temp/repos/c:hi) 4. repos-internal
> directory/file names (/c:hi/A/B/C)
>
> When adding windows path support to the path functions I
> didn't take paths of type four into account, hence the
> situation you reported.
>
> Problem is, not all path functions can handle all those four
> cases at the same time.
> Example: svn_path_join("/", "c:hi") in the above four cases returns:
> 1+2: c:hi because c:hi is an absolute path on Windows
Hi Lieven,
While updating the merge tracking branch I ran into some fallout from
r23907 because we were using svn_path_is_absolute(). No big deal, we
can work around that easily enough, but reading this thread one thing
caught my eye:
How is "c:hi" an absolute path on Windows? Isn't "c:hi" really the
equivalent of "c:.\hi"?
Paul B.
> 3: n/a
> 4: /c:hi
>
> Now svn_path_join is used over 350 times in the Subversion
> code, in all of the 4 cases. The same goes for
> svn_path_dirname, svn_path_basename etc.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by "D.J. Heap" <dj...@gmail.com>.
On 3/13/07, Lieven Govaerts <sv...@mobsol.be> wrote:
[snip]
>
> Problem is, not all path functions can handle all those four cases at
> the same time.
> Example: svn_path_join("/", "c:hi") in the above four cases returns:
> 1+2: c:hi because c:hi is an absolute path on Windows
> 3: n/a
> 4: /c:hi
>
> Now svn_path_join is used over 350 times in the Subversion code, in all
> of the 4 cases. The same goes for svn_path_dirname, svn_path_basename etc.
Ugh, I didn't realize it was that bad.
>
> To be honest, I don't see many other options than to revert the whole
> windows path thing and start from scratch on a feature branch. We'd have
> to work out all different usage patterns of the path functions, create
> new path functions for native, url, internal paths, and start using them
> throughout the code. To test all that a new test case has to be created
> to test all possible svn manipulations on 'c:hi' folders.
>
> What do you think?
>
Yeah, that may be what is needed. As it stands, we've got a pretty
severe compatibility problem with repository paths at least.
Do you think it would be feasible to just fix the repository-path
uses? Or do you think that would leave a lot of similar lurking
problems and we're better off analyzing and fixing them all at once?
I've only encountered the repository-path problem and I've been
testing trunk code pretty heavily on a few repositories for a while --
but we have very few oddball names like that.
DJ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Lieven Govaerts <sv...@mobsol.be>.
D.J. Heap wrote:
>> Based on your report I'm worried a bit that there are more situations
>> like this,
>> were path functions for internal paths were modified to work for
>> Windows paths.
>> I'll see what I can find, tonight.
> Thanks for looking at it!
>
DJ,
I did a quick scan through the code and the conclusion is very simple:
on Windows, trunk svn can't handle paths like 'c:hi' stored in the
repository. Meaning, direct operations on those urls will fail. Checking
those out will also fail obviously, so it's not that this is a common
situation, but still.
Your specific issue was fixed easily by reverting svn_path_join and
svn_path_join_many to their original (unix only) implementation. But
then I tried 'svn "ls file:///c:/temp/repos/c:hi"'. Result:
..\..\..\subversion\libsvn_client\delete.c:184: (apr_err=160013)
svn: URL 'hi' does not exist. The problem here is in svn_path_split,
which uses svn_path_dirname and svn_path_basename. All these functions
(10 in total) support Windows paths right now.
So, the scope of the problem we're facing is that the word 'path' in the
path functions can mean four things:
1.native directory/file names, (C:\temp) etc.
2. canonicalized native directory/file names (C:/temp)
3. urls (file:///c:/temp/repos/c:hi)
4. repos-internal directory/file names (/c:hi/A/B/C)
When adding windows path support to the path functions I didn't take
paths of type four into account, hence the situation you reported.
Problem is, not all path functions can handle all those four cases at
the same time.
Example: svn_path_join("/", "c:hi") in the above four cases returns:
1+2: c:hi because c:hi is an absolute path on Windows
3: n/a
4: /c:hi
Now svn_path_join is used over 350 times in the Subversion code, in all
of the 4 cases. The same goes for svn_path_dirname, svn_path_basename etc.
To be honest, I don't see many other options than to revert the whole
windows path thing and start from scratch on a feature branch. We'd have
to work out all different usage patterns of the path functions, create
new path functions for native, url, internal paths, and start using them
throughout the code. To test all that a new test case has to be created
to test all possible svn manipulations on 'c:hi' folders.
What do you think?
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by "D.J. Heap" <dj...@gmail.com>.
On 3/13/07, Lieven Govaerts <sv...@mobsol.be> wrote:
[snip]
>
> Hm, are you talking about paths relative to the repository root? Or actual urls?
Absolute url's (I guess they are called paths as well) within the
repository. During a 'svnadmin verify', for example, in
libsvn_fs_fs\tree.c the open_path function calls svn_path_join on line
679:
path_so_far = svn_path_join(path_so_far, entry, pool);
and appears to be expecting an absolute url to be returned. In the
case of the sample repo, the return value is 'c:hi' rather than
'/c:hi' as it expects -- this later causes the assert on line 148 of
make_txn_root to fail and kills the process.
>
> > Should 'svnadmin dump/verify/load' not be using svn_path_join or
> > should svn_path_join work with urls?
>
> I checked out the comments of svn_path_join, and they say:
>
> * Note that the contents of @a base are not examined, so it is possible to
> * use this function for constructing URLs, or for relative URLs or
> * repository paths.
> *
> * This function is NOT appropriate for native (local) file
> * paths. Only for "internal" canonicalized paths, since it uses '/'
> * for the separator. Further, an absolute path (for @a component) is
> * based on a leading '/' character. Thus, an "absolute URI" for the
> * @a component won't be detected.
>
> The problem is that svn_path_join *is* used for local paths despite the warning
> in the comments, so that's why it has to work with Windows paths too. We miss a
> clear separation between functions for internal paths and functions for native
> paths at the moment.
Right -- this sort of thing has happened a few times in the past, I
believe. Since urls and paths are very similar on unixy OS', it's
easy to miss the distinction.
Do we need a svn_path_local_join (or something) and then fix callers
that use svn_path_join incorrectly? That can be hard to determine
sometimes, if I remember correctly.
>
> Based on your report I'm worried a bit that there are more situations like this,
> were path functions for internal paths were modified to work for Windows paths.
> I'll see what I can find, tonight.
Thanks for looking at it!
DJ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svnadmin dump/verify/load uses svn_path_join
Posted by Lieven Govaerts <sv...@mobsol.be>.
Quoting "D.J. Heap" <dj...@gmail.com>:
> I've run into a rather nasty issue with a repository using trunk code.
>
> I've attached a test repository that works fine in 1.4.x or earlier
> but croaks when loading with trunk code built on Windows.
>
> The core issue seems to be that the dump/verify/load code uses
> svn_path_join on url's and svn_path_join now seems to be assuming that
> it is working with filesystem paths, not urls.
That's a result of the windows path changes I made last year. I based
myself on the available unit tests at that time, and there were none for url's.
> This means that in Win32 builds, 'svnadmin dump/load/verify' asserts
> and aborts for repositories that have urls such as '/c:hi' (it can't
> be checked out on Windows, of course, but that is a client/filesystem
> issue). It is probably not as much of an issue on Unix-type OS'
> because their filesystem paths look a lot like urls.
Hm, are you talking about paths relative to the repository root? Or actual urls?
> Should 'svnadmin dump/verify/load' not be using svn_path_join or
> should svn_path_join work with urls?
I checked out the comments of svn_path_join, and they say:
* Note that the contents of @a base are not examined, so it is possible to
* use this function for constructing URLs, or for relative URLs or
* repository paths.
*
* This function is NOT appropriate for native (local) file
* paths. Only for "internal" canonicalized paths, since it uses '/'
* for the separator. Further, an absolute path (for @a component) is
* based on a leading '/' character. Thus, an "absolute URI" for the
* @a component won't be detected.
The problem is that svn_path_join *is* used for local paths despite the warning
in the comments, so that's why it has to work with Windows paths too. We miss a
clear separation between functions for internal paths and functions for native
paths at the moment.
Based on your report I'm worried a bit that there are more situations like this,
were path functions for internal paths were modified to work for Windows paths.
I'll see what I can find, tonight.
Lieven
----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org