You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Lieven Govaerts <lg...@mobsol.be> on 2006/08/14 14:34:14 UTC
[PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Hi,
attached patch is a fix for issue #2556:
http://subversion.tigris.org/issues/show_bug.cgi?id=2556.
Windows allows one to create virtual drives to folders. If you map a virtual
drive to a working copy, so the working copy is on the root of that folder,
Subversion will fail updating and committing that working copy. In fact,
this issue is not dependent on using virtual drives, a working copy on the
root of a physical drive or partition will show the same issue.
This patch provides two fixes:
1. Introduce 'X:/' as a syntax for a root folder, on Windows. I've
encapsulated the check whether a path is a root folder in a new function
svn_path_is_root. There's a number of direct comparisons between path and
'/' throughout the code, so I've replaced them with a call to
svn_path_is_root. While my tests point out that it now works correctly, I've
might have missed some.
2. Fix an issue where Subversion thinks a working copy on a root path (both
'/' and 'X:/') is switched. I'm pretty sure the old code couldn't work on
unix either, probably no one has ever tested a working copy on '/'?
regards,
Lieven.
[[[
Fix for issue #2556: on Windows, X:/ is a root folder, so whenever we check
for the root folder, check for X:/ too.
* subversion/include/svn_path.h
(svn_path_is_root): New function declaration.
* subversion/libsvn_subr/path.c
(svn_path_is_root): New function. Tests for either '/' on all platforms
or 'X:/' on Windows.
(is_canonical): 'X:/' syntax on Windows is canonical.
(svn_path_join): Support the new type of root path on Windows, mostly by
replacing direct comparisons of path and '/' with a call to
svn_path_is_root.
(svn_path_dirname): idem
(svn_path_basename): idem
(svn_path_canonicalize): don't strip the trailing slash if the path is
of the 'X:/' syntax.
* subversion/libsvn_wc/status.c
(assemble_status): add support for working copies at the root of a
(virtual)
drive. This is not Windows specific, this code didn't work for working
copies on '/' either.
* subversion/tests/cmdline/update_tests.py
(update_wc_on_windows_drive): New test for issue 2556.
(test_list): add the new test to the list.
]]]
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
My intention was to follow up these patches, which contain only a basic
test scenario, with some extra tests and fixes (where needed). I have
two approaches in mind:
1. checking the code the same way like you did.
2. locally modifying the python test framework to map each created
working copy to a windows virtual drive. Then I'll test all svn actions
on the root of a drive.
FYI: There's also issue #1711, which theses patches partially solve, and
issue #1869, which has been solved in the apr tree. I just touched this
issue because my colleagues want to use virtual drives on Windows, but
I'll try to fix the other issue as well.
Philip Martin wrote:
> Grepping for \[0\].*\'/\' shows that there might be a few places where
> we assume that path[0]=='/' is special; I don't know whether your
> patch invalidates those assumptions. If path is a Subversion FS path
> then it's OK but if path is a WC path then it could be a problem. For
> example:
>
> - libsvn_client/commit.c:1312 seems to be checking for "/", should it
> be checking for "x:/" as well?
Yes it should. This can be tested by committing multiple directories on
the root of a drive.
>
> - libsvn_wc/props.c:2110 seems to be checking for "/...", should it be
> checking for "x:/..." as well?
If I read the code correctly, this is the case of a propset on the root
of a drive.
>
> - libsvn_client/diff.c:401 and 408 seem to be checking for "/...",
> should they be checking for "x:/..." as well?
As far as I see, this can be tested by a diff on two files in the root
folder of a drive.
> I don't know if these are bugs, and if they are I suspect they are
> unrelated to your patch, but someone who groks this Windows stuff
> probably ought to take a look :)
>
I prefer to commit these two patches first, and then follow up with any
other cases we can find in the code. I'll keep the locations you found
on my TODO list for issue 2556.
regards,
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Philip Martin <ph...@codematters.co.uk>.
"Lieven Govaerts" <lg...@mobsol.be> writes:
> Prepare fix for issue #2556: abstract root folder checks in
> svn_path_is_root.
> Add support for 'X:/' as a root folder on Windows.
Grepping for \[0\].*\'/\' shows that there might be a few places where
we assume that path[0]=='/' is special; I don't know whether your
patch invalidates those assumptions. If path is a Subversion FS path
then it's OK but if path is a WC path then it could be a problem. For
example:
- libsvn_client/commit.c:1312 seems to be checking for "/", should it
be checking for "x:/" as well?
- libsvn_wc/props.c:2110 seems to be checking for "/...", should it be
checking for "x:/..." as well?
- libsvn_client/diff.c:401 and 408 seem to be checking for "/...",
should they be checking for "x:/..." as well?
I don't know if these are bugs, and if they are I suspect they are
unrelated to your patch, but someone who groks this Windows stuff
probably ought to take a look :)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Branko Čibej <br...@xbc.nu>.
Lieven Govaerts wrote:
> Branko Čibej wrote:
>> Lieven Govaerts wrote:
>>>>> int svn_path_is_empty(const char *path);
>>>>> which returns either 1 or 0. You know why we use the 'int' there?
>>>>>
>>>> No idea, probably history. I think it's an error we didn't see
>>>> before releasing 1.0...
>>>>
>>> I'll send a patch for this to the list tomorrow.
>>>
>>
>> That won't help, you can't change the return type before svn-2.0.
>>
>
> I was under the impression that svn_path.h is an internal header, but
> you're right, it isn't.
>
> Makes me wonder, the declaration of svn_boolean_t is:
> typedef int svn_boolean_t;
>
> How would compatibility be impacted if we changed return type int to
> svn_boolean_t?
Hm, I was certain svn_boolean_t was a "char", heh. Well, I suppose it
wouldn't matter.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
Branko Čibej wrote:
> Lieven Govaerts wrote:
>>>> int svn_path_is_empty(const char *path);
>>>> which returns either 1 or 0. You know why we use the 'int' there?
>>>>
>>> No idea, probably history. I think it's an error we didn't
>>> see before releasing 1.0...
>>>
>> I'll send a patch for this to the list tomorrow.
>>
>
> That won't help, you can't change the return type before svn-2.0.
>
I was under the impression that svn_path.h is an internal header, but
you're right, it isn't.
Makes me wonder, the declaration of svn_boolean_t is:
typedef int svn_boolean_t;
How would compatibility be impacted if we changed return type int to
svn_boolean_t?
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Branko Čibej <br...@xbc.nu>.
Lieven Govaerts wrote:
>>> int svn_path_is_empty(const char *path);
>>> which returns either 1 or 0. You know why we use the 'int' there?
>>>
>> No idea, probably history. I think it's an error we didn't
>> see before releasing 1.0...
>>
>
> I'll send a patch for this to the list tomorrow.
>
That won't help, you can't change the return type before svn-2.0.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
> -----Original Message-----
> From: Erik Huelsmann [mailto:ehuels@gmail.com]
> Sent: zaterdag 19 augustus 2006 0:01
..
> Well, if you check them for the expected value, it will give
> you an immediate exit from the comparison, so there's no real
> need to check them all for \0: if it's a null length string
> (""\0), the first char won't be in a-zA-Z, so that's where
> the comparison exits. Right where you want it.
>
I've updated the patch:
- a check that the first character of the path is in [a..z][A..Z] in
is_canonical and svn_path_is_root.
- return type of svn_path_is_root changed from int to svn_boolean_t.
Both patches are added (the 'wc is switched' patch isn't changed).
> > int svn_path_is_empty(const char *path);
> > which returns either 1 or 0. You know why we use the 'int' there?
>
> No idea, probably history. I think it's an error we didn't
> see before releasing 1.0...
I'll send a patch for this to the list tomorrow.
Thanks for the review,
Lieven.
Commit messages:
[[[
Prepare fix for issue #2556: abstract root folder checks in
svn_path_is_root.
Add support for 'X:/' as a root folder on Windows.
* subversion/include/svn_path.h
(svn_path_is_root): New function declaration.
* subversion/libsvn_subr/path.c
(svn_path_is_root): New function. Tests for either '/' on all platforms
or 'X:/' on Windows.
(is_canonical): 'X:/' syntax on Windows is canonical.
(svn_path_join, svn_path_dirname, svn_path_basename): Support the new type
of root path on Windows, mostly by replacing direct comparisons of path
and '/' with a call to svn_path_is_root.
(svn_path_canonicalize): don't strip the trailing slash if the path is
of the 'X:/' syntax.
* subversion/tests/cmdline/update_tests.py
(update_wc_on_windows_drive): New test for issue 2556.
(test_list): add the new test to the list.
]]]
[[[
Fix an issue where the status of a working copy on the root of a drive is
'S' (switched), as part of issue #2556.
This is not Windows specific, this code didn't work for working copies on
'/' either.
* subversion/libsvn_wc/status.c
(assemble_status): add support for working copies at the root of a
(virtual)
drive.
]]]
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
> > > Regarding this patch: It's basically fine, but you skip testing for
> > > the first character of "X:\\"<nul>, but that means you may
> > be checking
> > > past the end of the string. (this is in svn_path_is_root
> > and further
> > > down in the patch [the last hunk]; in is_canonical, you do it
> > > correctly.)
>
> In is_canonical we already have the length of the path, while in
> svn_path_is_root we'd have to calculate it. Not that that's a problem, but
> there's this comment in svn_path_is_empty in path.c:
> /* assert (is_canonical (path, strlen (path))); ### Expensive strlen */
Well, you don't actually need to strlen the string: if you explicitly
check each of the 4 chars (0..3) for values, the comparison will stop
at the first non-match. That way, you will never compare more
characters than there are in the string.
> I've taken this as a general rule to avoid calculating string lenghts too
> often. Considering the time svn spends creating and deleting temporary files
> during a commit I doubt anyone will notice an extra strlen call though.
> I can check all chars for '\0' first too, no real need to calculate strlen.
Well, if you check them for the expected value, it will give you an
immediate exit from the comparison, so there's no real need to check
them all for \0: if it's a null length string (""\0), the first char
won't be in a-zA-Z, so that's where the comparison exits. Right where
you want it.
> > > Isn't it so that the X should be in the A-Za-z set of characters?
> > > Should we explicitly test for that?
>
> I'll add that.
>
> > Ok, but I did find a problem: svn_path_is_root returns an int
> > in your patch, but it should return svn_boolean_t, since
> > that's what it is (a boolean).
>
> Well, I thought the same thing, but I based svn_path_is_root on another
> function in path.c:
> int svn_path_is_empty(const char *path);
>
> which returns either 1 or 0. You know why we use the 'int' there?
No idea, probably history. I think it's an error we didn't see before
releasing 1.0...
> > I have no working copy here, so I'll ask you this question instead:
> > Does this change make it so that the parent of the root is
> > also the root? Or was that the defined behaviour already?
> > (Your next patch is based on that behaviour, that's why I ask...)
>
> Yep, that's the current (both pre and post my patches) behaviour of
> svn_path_dirname:
[snip]
Ok. Well, I think that in that case, your change is probably best.
Should any calls to assemble_status be added, then your change will
catch their edge-cases too.
> > What I mean is: currently, if a parent of an entry isn't
> > versioned, we already have code which determines that the
> > current entry can't be switched. My question is: can't we
> > make some change somewhere so that we can benefit from the
> > fact that this behaviour is correctly coded throughout the
> > system? (Instead of introducing yet another edge
> > case...)
>
> Since you don't have a working copy at hand, I'll copy the relevant portion
> of svn_wc_status2 here:
>
[ snip ]
No, I prefer your solution now. Thanks!
Bye,
Erik.
BTW: I forgot all about my lxr of the Subversion code:
http://hix.nu/subversion/lxr/source; I used it to have a look at the
status.c lines you indicated. It seems I do have a working copy at
hand!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
> -----Original Message-----
> From: Erik Huelsmann [mailto:ehuels@gmail.com]
> Sent: donderdag 17 augustus 2006 20:39
> > > > > 1. Introduce 'X:/' as a syntax for a root folder, on Windows.
> > >
> > > This change is contained in issue-2556-wc-on-root.patch.txt.
> >
> > Regarding this patch: It's basically fine, but you skip testing for
> > the first character of "X:\\"<nul>, but that means you may
> be checking
> > past the end of the string. (this is in svn_path_is_root
> and further
> > down in the patch [the last hunk]; in is_canonical, you do it
> > correctly.)
In is_canonical we already have the length of the path, while in
svn_path_is_root we'd have to calculate it. Not that that's a problem, but
there's this comment in svn_path_is_empty in path.c:
/* assert (is_canonical (path, strlen (path))); ### Expensive strlen */
I've taken this as a general rule to avoid calculating string lenghts too
often. Considering the time svn spends creating and deleting temporary files
during a commit I doubt anyone will notice an extra strlen call though.
I can check all chars for '\0' first too, no real need to calculate strlen.
> > Isn't it so that the X should be in the A-Za-z set of characters?
> > Should we explicitly test for that?
I'll add that.
> Ok, but I did find a problem: svn_path_is_root returns an int
> in your patch, but it should return svn_boolean_t, since
> that's what it is (a boolean).
Well, I thought the same thing, but I based svn_path_is_root on another
function in path.c:
int svn_path_is_empty(const char *path);
which returns either 1 or 0. You know why we use the 'int' there?
> I have no working copy here, so I'll ask you this question instead:
> Does this change make it so that the parent of the root is
> also the root? Or was that the defined behaviour already?
> (Your next patch is based on that behaviour, that's why I ask...)
Yep, that's the current (both pre and post my patches) behaviour of
svn_path_dirname:
/** Get the dirname of the specified canonicalized @a path, defined as
* the path with its basename removed.
*
* Get the dirname of the specified @a path, defined as the path with its
* basename removed. If @a path is root ("/"), it is returned unchanged.
*
* The returned dirname will be allocated in @a pool.
*/
char *svn_path_dirname(const char *path, apr_pool_t *pool);
> > > > > 2. Fix an issue where Subversion thinks a working copy on a root
path
> > > (both '/' and 'X:/') is switched.
> > >
> > > This change is contained in issue-2556-wc-switched.patch.txt.
> >
> I've looked at it now. It's a nice and localized change. But
> what would need to happen so that the parent of the root was
> something that returns 'unversioned' (parent_entry == NULL),
> so that this change isn't even required?
>
> What I mean is: currently, if a parent of an entry isn't
> versioned, we already have code which determines that the
> current entry can't be switched. My question is: can't we
> make some change somewhere so that we can benefit from the
> fact that this behaviour is correctly coded throughout the
> system? (Instead of introducing yet another edge
> case...)
Since you don't have a working copy at hand, I'll copy the relevant portion
of svn_wc_status2 here:
line 2068:
if (entry && ! svn_path_is_empty(path))
{
const char *parent_path = svn_path_dirname(path, pool);
svn_wc_adm_access_t *parent_access;
SVN_ERR(svn_wc__adm_retrieve_internal(&parent_access, adm_access,
parent_path, pool));
if (parent_access)
SVN_ERR(svn_wc_entry(&parent_entry, parent_path, parent_access,
FALSE, pool));
}
SVN_ERR(assemble_status(status, path, adm_access, entry, parent_entry,
svn_node_unknown, FALSE, /* bogus */
TRUE, FALSE, NULL, NULL, pool));
If we check that parent_path equals path, we can skip retrieving adm_access,
but that creates a similar edge case in my opinion. Would you prefer that
option?
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
On 8/17/06, Erik Huelsmann <eh...@gmail.com> wrote:
> > > In that case, could you provide 2 patches? It's best to
> > > separate conceptually different changes into separate commits.
> >
> > Ok, attached you'll find two patches.
>
> Thanks.
>
> > > > 1. Introduce 'X:/' as a syntax for a root folder, on Windows.
> >
> > This change is contained in issue-2556-wc-on-root.patch.txt.
>
> Regarding this patch: It's basically fine, but you skip testing for
> the first character of "X:\\"<nul>, but that means you may be checking
> past the end of the string. (this is in svn_path_is_root and further
> down in the patch [the last hunk]; in is_canonical, you do it
> correctly.)
> Isn't it so that the X should be in the A-Za-z set of characters?
> Should we explicitly test for that?
Ok, but I did find a problem: svn_path_is_root returns an int in your
patch, but it should return svn_boolean_t, since that's what it is (a
boolean).
I have no working copy here, so I'll ask you this question instead:
Does this change make it so that the parent of the root is also the
root? Or was that the defined behaviour already? (Your next patch is
based on that behaviour, that's why I ask...)
> > > > 2. Fix an issue where Subversion thinks a working copy on a root path
> > (both '/' and 'X:/') is switched.
> >
> > This change is contained in issue-2556-wc-switched.patch.txt.
>
> I haven't had time to look into this patch yet; I'll come back to you later.
I've looked at it now. It's a nice and localized change. But what
would need to happen so that the parent of the root was something that
returns 'unversioned' (parent_entry == NULL), so that this change
isn't even required?
What I mean is: currently, if a parent of an entry isn't versioned, we
already have code which determines that the current entry can't be
switched. My question is: can't we make some change somewhere so that
we can benefit from the fact that this behaviour is correctly coded
throughout the system? (Instead of introducing yet another edge
case...)
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
> > In that case, could you provide 2 patches? It's best to
> > separate conceptually different changes into separate commits.
>
> Ok, attached you'll find two patches.
Thanks.
> > > 1. Introduce 'X:/' as a syntax for a root folder, on Windows.
>
> This change is contained in issue-2556-wc-on-root.patch.txt.
Regarding this patch: It's basically fine, but you skip testing for
the first character of "X:\\"<nul>, but that means you may be checking
past the end of the string. (this is in svn_path_is_root and further
down in the patch [the last hunk]; in is_canonical, you do it
correctly.)
Isn't it so that the X should be in the A-Za-z set of characters?
Should we explicitly test for that?
> > > 2. Fix an issue where Subversion thinks a working copy on a root path
> (both '/' and 'X:/') is switched.
>
> This change is contained in issue-2556-wc-switched.patch.txt.
I haven't had time to look into this patch yet; I'll come back to you later.
> I've used your comments to make two new commit messages.
>
> For issue-2556-wc-on-root.patch.txt:
> [[[
> Prepare fix for issue #2556: abstract root folder check in svn_path_is_root.
>
> Add support for 'X:/' as a root folder on Windows.
>
> * subversion/include/svn_path.h
> (svn_path_is_root): New function declaration.
>
> * subversion/libsvn_subr/path.c
> (svn_path_is_root): New function. Tests for either '/' on all platforms
> or 'X:/' on Windows.
> (is_canonical): 'X:/' syntax on Windows is canonical.
> (svn_path_join, svn_path_dirname, svn_path_basename): Support the new type
>
> of root path on Windows, mostly by replacing direct comparisons of path
> and '/' with a call to svn_path_is_root.
> (svn_path_canonicalize): don't strip the trailing slash if the path is
> of the 'X:/' syntax.
>
> * subversion/tests/cmdline/update_tests.py
> (update_wc_on_windows_drive): New test for issue 2556.
> (test_list): add the new test to the list.
> ]]]
These messages are fine now.
> For issue-2556-wc-switched.patch.txt:
> [[[
> Fix an issue where the status of a working copy on the root of a drive is
> 'S' (switched), as part of issue #2556.
> This is not Windows specific, this code didn't work for working copies on
> '/' either.
>
> * subversion/libsvn_wc/status.c
> (assemble_status): add support for working copies at the root of a
> (virtual)
> drive.
> ]]]
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
Thanks for the review Erik.
> -----Original Message-----
> From: Erik Huelsmann [mailto:ehuels@gmail.com]
> Sent: maandag 14 augustus 2006 22:04
>
> > This patch provides two fixes:
>
> In that case, could you provide 2 patches? It's best to
> separate conceptually different changes into separate commits.
Ok, attached you'll find two patches.
> > 1. Introduce 'X:/' as a syntax for a root folder, on Windows.
This change is contained in issue-2556-wc-on-root.patch.txt.
> > 2. Fix an issue where Subversion thinks a working copy on a root path
(both '/' and 'X:/') is switched.
This change is contained in issue-2556-wc-switched.patch.txt.
> Being on holidays (and thus writing this over dail-up), I've
> just skipped over your commit message and not read the actual patch.
> Comment(s) below.
I've used your comments to make two new commit messages.
For issue-2556-wc-on-root.patch.txt:
[[[
Prepare fix for issue #2556: abstract root folder check in svn_path_is_root.
Add support for 'X:/' as a root folder on Windows.
* subversion/include/svn_path.h
(svn_path_is_root): New function declaration.
* subversion/libsvn_subr/path.c
(svn_path_is_root): New function. Tests for either '/' on all platforms
or 'X:/' on Windows.
(is_canonical): 'X:/' syntax on Windows is canonical.
(svn_path_join, svn_path_dirname, svn_path_basename): Support the new type
of root path on Windows, mostly by replacing direct comparisons of path
and '/' with a call to svn_path_is_root.
(svn_path_canonicalize): don't strip the trailing slash if the path is
of the 'X:/' syntax.
* subversion/tests/cmdline/update_tests.py
(update_wc_on_windows_drive): New test for issue 2556.
(test_list): add the new test to the list.
]]]
For issue-2556-wc-switched.patch.txt:
[[[
Fix an issue where the status of a working copy on the root of a drive is
'S' (switched), as part of issue #2556.
This is not Windows specific, this code didn't work for working copies on
'/' either.
* subversion/libsvn_wc/status.c
(assemble_status): add support for working copies at the root of a
(virtual)
drive.
]]]
regards,
Lieven.
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
On 8/14/06, Lieven Govaerts <lg...@mobsol.be> wrote:
> Windows allows one to create virtual drives to folders. If you map a virtual
> drive to a working copy, so the working copy is on the root of that folder,
> Subversion will fail updating and committing that working copy. In fact,
> this issue is not dependent on using virtual drives, a working copy on the
> root of a physical drive or partition will show the same issue.
> This patch provides two fixes:
In that case, could you provide 2 patches? It's best to separate
conceptually different changes into separate commits.
> 1. Introduce 'X:/' as a syntax for a root folder, on Windows. I've
> encapsulated the check whether a path is a root folder in a new function
> svn_path_is_root. There's a number of direct comparisons between path and
> '/' throughout the code, so I've replaced them with a call to
> svn_path_is_root. While my tests point out that it now works correctly, I've
> might have missed some.
Nice.
> 2. Fix an issue where Subversion thinks a working copy on a root path (both
> '/' and 'X:/') is switched. I'm pretty sure the old code couldn't work on
> unix either, probably no one has ever tested a working copy on '/'?
No, probably not :-)
Being on holidays (and thus writing this over dail-up), I've just
skipped over your commit message and not read the actual patch.
Comment(s) below.
> [[[
> Fix for issue #2556: on Windows, X:/ is a root folder, so whenever we check
> for the root folder, check for X:/ too.
For the first change this could say 'Prepare for fixing issue ...:
abstract root folder checks' or something like it.
> * subversion/include/svn_path.h
> (svn_path_is_root): New function declaration.
>
> * subversion/libsvn_subr/path.c
> (svn_path_is_root): New function. Tests for either '/' on all platforms
> or 'X:/' on Windows.
> (is_canonical): 'X:/' syntax on Windows is canonical.
> (svn_path_join): Support the new type of root path on Windows, mostly by
> replacing direct comparisons of path and '/' with a call to
> svn_path_is_root.
> (svn_path_dirname): idem
> (svn_path_basename): idem
if 'idem' means 'the same as above', then that part would better read:
(svn_path_join, svn_path_dirname,
svn_path_basename): Support the new type of root path on Windows, mostly by
replacing direct comparisons of path and '/' with a call to
svn_path_is_root.
> (svn_path_canonicalize): don't strip the trailing slash if the path is
> of the 'X:/' syntax.
>
> * subversion/libsvn_wc/status.c
> (assemble_status): add support for working copies at the root of a
> (virtual)
> drive. This is not Windows specific, this code didn't work for working
> copies on '/' either.
The remark about non-windows specificness could be placed on its own
line at the top of the message below the summary, that way, the reader
knows what to expect beforehand (at least, that's what I would do)
> * subversion/tests/cmdline/update_tests.py
> (update_wc_on_windows_drive): New test for issue 2556.
> (test_list): add the new test to the list.
> ]]]
HTH,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
On 8/20/06, Lieven Govaerts <lg...@mobsol.be> wrote:
> > -----Original Message-----
> > From: Erik Huelsmann [mailto:ehuels@gmail.com]
> > Sent: zondag 20 augustus 2006 17:53
>
> > >
> > > Why don't you just use apr_filepath_root here and everywhere else
> > > where this matters?
> >
> >
> > In that case: why introduce a function in the svn_ namespace,
> > if there's a function in APR which does exactly what we want?
> > But, isn't it a problem that our paths are UTF-8 while APR
> > expects localized paths?
> >
> > Lieven: apr_filepath_root will give the root of a given
> > inpath, maybe it'll work, because then it will also work for
> > working copies in the root of a share or UNC path...
>
> I'm working on it now, will have a patch tonight.
>
> The apr_filepath_root is more then we want, because it also splits the path
> in two parts, so we need to allocate new strings, pass a pool to the
> function etc. But, it supports more Windows path conventions than the one I
> implemented, so let's use it.
>
> I'm gonna stick to the svn_path_is_root function because of the overhead
> needed to use apr_filepath_root.
Yup. I realised that after reading the full doc string, but also after
sending my previous message. Seems fine.
Don't hurry with your patch: I'm not going anywhere (yet) :-)
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
On 8/20/06, Branko Čibej <br...@xbc.nu> wrote:
> Lieven Govaerts wrote:
> > Erik Huelsmann wrote:
> >>
> >> I wouldn't bother with the subpool creation: the extra memory required
> >> is negligeable when compared to other allocated amounts. We generally
> >> don't create subpools for usecases like this.
> >>
> >> (To give you a first review.)
> >
> > I introduced the subpool because I saw suddenly a huge memory usage,
> > which I suspected was due the strings not being cleared from the pool.
> > The problem turned out to be something else though.
> >
> > However, there's another reason why I create a subpool. The function
> > is_canonical (private function in path.c) is calling svn_path_is_root,
> > but that function doesn't have a pool available and is called by other
> > functions which don't have pool parameters.
>
> Eek. That's potentially a big problem, yes; you'd have to use the global
> pool in such calls.
Or, if all of them are private, add a pool parameter...
bye,
Erik.
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Branko Čibej <br...@xbc.nu>.
Lieven Govaerts wrote:
> Erik Huelsmann wrote:
>>
>> I wouldn't bother with the subpool creation: the extra memory required
>> is negligeable when compared to other allocated amounts. We generally
>> don't create subpools for usecases like this.
>>
>> (To give you a first review.)
>
> I introduced the subpool because I saw suddenly a huge memory usage,
> which I suspected was due the strings not being cleared from the pool.
> The problem turned out to be something else though.
>
> However, there's another reason why I create a subpool. The function
> is_canonical (private function in path.c) is calling svn_path_is_root,
> but that function doesn't have a pool available and is called by other
> functions which don't have pool parameters.
Eek. That's potentially a big problem, yes; you'd have to use the global
pool in such calls.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
>> -----Original Message-----
>> From: Erik Huelsmann [mailto:ehuels@gmail.com]
>> Sent: dinsdag 22 augustus 2006 23:39
>
..
>
>> I'm +1 on committing this. Note that that's only based on code review:
>> I have no means of testing it here, but judging from the review, it's good.
>
With Erik's approval on IRC I committed these two patches in r21276 (wc
switched) and r21277 (svn_path_is_root).
As a result, one of the unit tests started to fail. I started a new mail
thread to discuss possible solutions for this problem:
http://svn.haxx.se/dev/archive-2006-08/0838.shtml
Thanks for taking the time for the review of these patches!
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
> -----Original Message-----
> From: Erik Huelsmann [mailto:ehuels@gmail.com]
> Sent: dinsdag 22 augustus 2006 23:39
> Apart from this little nit:
>
> + if ((status == APR_SUCCESS ||
> + status == APR_EINCOMPLETE) &&
> + rel_path[0] == '\0')
> + {
> + result = TRUE;
> + goto cleanup;
> + }
> +
> + cleanup:
> + if (!pool)
> + apr_pool_destroy(strpool);
> + return result;
> +}
>
>
> Which is shorter written as
>
> + if ((status == APR_SUCCESS ||
> + status == APR_EINCOMPLETE) &&
> + rel_path[0] == '\0')
> + result = TRUE;
> +
> + cleanup:
> + if (!pool)
> + apr_pool_destroy(strpool);
> + return result;
> +}
>
>I'm +1 on committing this. Note that that's only based on code review:
>I have no means of testing it here, but judging from the review, it's good.
Hm, I don't like to use fall-through, it's bound to be forgotten when
someone adds an extra line of code after the 'if' statement later. I've
checked the subversion code before writing this part and this approach seems
to be fairly common.
I'll wait with the commit for now, see if there other remarks.
Thanks for taking the time you spent on the review Erik (and enjoy the rest
of your holiday).
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
> > Reading it Garrett's way: we may want to rev the APIs just because of the
> pool requirement. But, when the pool being
> > passed in is NULL (as the old api compat routines would), we'd need to
> create our own pool with svn_pool_create(NULL);
> > something along these lines:
>
> Included a slightly simpler form of your example.
Apart from this little nit:
+ if ((status == APR_SUCCESS ||
+ status == APR_EINCOMPLETE) &&
+ rel_path[0] == '\0')
+ {
+ result = TRUE;
+ goto cleanup;
+ }
+
+ cleanup:
+ if (!pool)
+ apr_pool_destroy(strpool);
+ return result;
+}
Which is shorter written as
+ if ((status == APR_SUCCESS ||
+ status == APR_EINCOMPLETE) &&
+ rel_path[0] == '\0')
+ result = TRUE;
+
+ cleanup:
+ if (!pool)
+ apr_pool_destroy(strpool);
+ return result;
+}
I'm +1 on committing this. Note that that's only based on code review:
I have no means of testing it here, but judging from the review, it's
good.
> I didn't rev any of the functions in path.c nor deleted is_canonical. This
> can be done, it'll improve the memory usage slightly.
That, and in the common case (where the revved api would be used), you
wouldn't need to resort to allocating a global pool, so you wouldn't
need to resort to obtaining a global mutex, which should be faster
too. The fallback scenario would be to obtain the global mutex (and
thus be slow and less memory-efficient).
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
Attached a new version of the issue-2556-wc-on-root patch. The patch is
updated with your remarks (see comments). I also removed the python
regression test (already committed).
Your remarks:
> > + will cause problems elsewhere, anyway. */ if
> > + (svn_path_cstring_from_utf8(&rel_path, rel_path, strpool) !=
> > APR_SUCCESS)
> >
> > Here you may leek a pool.
Fixed.
> This function returns an svn_error_t*, which really shouldn't
> be compared to APR_SUCCESS, because they're two totally
> different beasts.
> You'll have to introduce an svn_error_t* variable to hold the
> return value, and call svn_error_clear. For example,
>
> svn_error_t *err = svn_path_cstring_from_utf8(&rel_path,
> rel_path, strpool); if (err)
> {
> svn_error_clear(err);
> return FALSE;
> }
Fixed.
> Reading it Garrett's way: we may want to rev the APIs just because of the
pool requirement. But, when the pool being
> passed in is NULL (as the old api compat routines would), we'd need to
create our own pool with svn_pool_create(NULL);
> something along these lines:
Included a slightly simpler form of your example.
I didn't rev any of the functions in path.c nor deleted is_canonical. This
can be done, it'll improve the memory usage slightly. Note that this is only
the first patch though, others will follow in which other functions like
svn_path_add_component, svn_path_component_count, svn_path_is_ancestor...
(none of these have a pool parameter) will start using
svn_path_file_is_root.
> There's one more nit: In the patch you say you added X:/ as canonical,
which is true,
> but by using apr_filepath_root, you also added \\?\X:/, \\.\..,
\\server\share and maybe
> some others. I've got no idea how to put it other than 'All forms
considered canonical
> root specs on the platform at hand, amongst which...' or something like
it.
While the new function svn_path_is_root now supports these forms, that
doesn't mean svn now supports them. I don't want to give the impression that
by applying this patch people can actual use those forms, in fact, I'd be
surprised of those work. I used your proposal in the comment of the
svn_path_is_root function.
regards,
Lieven.
issue-2556-wc-on-root.patch.txt:
[[[
Prepare fix for issue #2556: abstract root folder checks in
svn_path_is_root.
Add support for 'X:/' as a root folder on Windows.
* subversion/include/svn_path.h
(svn_path_is_root): New function declaration.
(svn_path_is_empty): Added comment.
* subversion/libsvn_client/commit.c
(svn_client_commit4): make sure to abort when the iterating through the
parent folders passese the root folder.
* subversion/libsvn_subr/path.c
(svn_path_is_root): New function. Tests for all forms considered canonical
root specs on the platform at hand ('/', 'X:/' etc.)
(is_canonical): 'X:/' syntax on Windows is canonical.
(svn_path_join, svn_path_dirname, svn_path_basename,
svn_path_join_many, previous_segment,
get_path_ancestor_length): Support the new type of root path on Windows,
mostly by replacing direct comparisons of path and '/' with a call to
svn_path_is_root.
(svn_path_canonicalize): don't strip the trailing slash if the path is
of the 'X:/' syntax.
* subversion/tests/libsvn_subr/path-test.c
(test_is_root): New test for validation of svn_path_is_root.
(test_path_split, test_join,
test_basename, test_remove_component): add extra cases for 'X:/'.
(test_funcs): run the new test test_is_root.
]]]
issue-2556-wc-switched.patch.txt:
[[[
Fix an issue where the status of a working copy on the root of a drive is
'S' (switched), as part of issue #2556.
This is not Windows specific, this code didn't work for working copies on
'/' either.
* subversion/libsvn_wc/status.c
(assemble_status): add support for working copies at the root of a
(virtual)
drive.
]]]
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Branko Čibej <br...@xbc.nu>.
Erik Huelsmann wrote:
> However, calling svn_pool_create(NULL) isn't thread safe AFAIK,
> because it allocates from the main pool (is this right, Branko,
> anybody?). Or is that the only bit for which the main pool *is*
> protected?
This is not allocation from the global pool, it's subpool creation,
which is a different beast. If subpool creation weren't thread-safe, you
couldn't create _any_ pools safely.
>
> Anyway: two other comments inline below:
>
> @@ -418,6 +424,35 @@
> }
>
>
> +svn_boolean_t
> +svn_path_is_root(const char *path, apr_size_t len)
> +{
> + char *root_path = NULL;
> + apr_status_t status;
> + apr_pool_t *strpool = svn_pool_create(NULL);
> + char *rel_path = apr_pstrmemdup(strpool, path, len);
> +
> + /* svn_path_cstring_from_utf8 will create a copy of path.
> +
> + It should be safe to convert this error to a false return value.
> An error
> + in this case would indicate that the path isn't encoded in
> UTF-8, which
> + will cause problems elsewhere, anyway. */
> + if (svn_path_cstring_from_utf8(&rel_path, rel_path, strpool) !=
> APR_SUCCESS)
>
> Here you may leek a pool.
This function returns an svn_error_t*, which really shouldn't be
compared to APR_SUCCESS, because they're two totally different beasts.
You'll have to introduce an svn_error_t* variable to hold the return
value, and call svn_error_clear. For example,
svn_error_t *err = svn_path_cstring_from_utf8(&rel_path, rel_path, strpool);
if (err)
{
svn_error_clear(err);
return FALSE;
}
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
> Ok. I didn't investigate more of it myself, so I rely on you to make
> sure that we call svn_path_is_root from other functions which don't
> have a pool (excluding is_canonical, since Philip said we can safely
> remove that function from consideration).
Sorry for reading your patch post so badly. You already addressed that.
There's one more nit: In the patch you say you added X:/ as canonical,
which is true, but by using apr_filepath_root, you also added \\?\X:/,
\\.\.., \\server\share and maybe some others. I've got no idea how to
put it other than 'All forms considered canonical root specs on the
platform at hand, amongst which...' or something like it.
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
On 8/21/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 8/21/06, Erik Huelsmann <eh...@gmail.com> wrote:
>
> > Ok. I didn't investigate more of it myself, so I rely on you to make
> > sure that we call svn_path_is_root from other functions which don't
> > have a pool (excluding is_canonical, since Philip said we can safely
> > remove that function from consideration).
> >
> > However, calling svn_pool_create(NULL) isn't thread safe AFAIK,
> > because it allocates from the main pool (is this right, Branko,
> > anybody?). Or is that the only bit for which the main pool *is*
> > protected?
> >
>
> Incorrect, creating a new global pool (via svn_pool_create(NULL))
> should be thread safe (allocating things directly from the global pool
> yourself is not necessarily thread safe though, although you have to
> jump through hoops to get a handle on it anyway), it's just not
> something you really want to do very often because it's kinda slow.
> Creating a new pool involves allocating about 8k of data, which is
> kind of silly if you're just going to use it for one tiny thing then
> destroy it.
Reading it Garrett's way: we may want to rev the APIs just because of
the pool requirement. But, when the pool being passed in is NULL (as
the old api compat routines would), we'd need to create our own pool
with svn_pool_create(NULL); something along these lines:
svn_boolean_t
svn_path_is_root(const char *path, apr_size_t len, apr_pool_t *pool)
{
char *root_path = NULL;
apr_status_t status;
apr_pool_t *strpool = (pool) ? NULL : svn_pool_create(NULL);
char *rel_path = apr_pstrmemdup(strpool, path, len);
svn_error_t *err;
if (! pool)
pool = strpool;
/* svn_path_cstring_from_utf8 will create a copy of path.
It should be safe to convert this error to a false return value. An error
in this case would indicate that the path isn't encoded in UTF-8, which
will cause problems elsewhere, anyway. */
if (err = svn_path_cstring_from_utf8(&rel_path, rel_path, strpool))
{
svn_error_clear(err);
if (strpool)
apr_pool_destroy(strpool);
return FALSE;
}
status = apr_filepath_root(&root_path, &rel_path, 0, strpool);
if ((status == APR_SUCCESS ||
status == APR_EINCOMPLETE) &&
rel_path[0] == '\0')
{
if (strpool)
apr_pool_destroy(strpool);
return TRUE;
}
if (strpool)
apr_pool_destroy(strpool);
return FALSE;
}
HTH,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/21/06, Erik Huelsmann <eh...@gmail.com> wrote:
> Ok. I didn't investigate more of it myself, so I rely on you to make
> sure that we call svn_path_is_root from other functions which don't
> have a pool (excluding is_canonical, since Philip said we can safely
> remove that function from consideration).
>
> However, calling svn_pool_create(NULL) isn't thread safe AFAIK,
> because it allocates from the main pool (is this right, Branko,
> anybody?). Or is that the only bit for which the main pool *is*
> protected?
>
Incorrect, creating a new global pool (via svn_pool_create(NULL))
should be thread safe (allocating things directly from the global pool
yourself is not necessarily thread safe though, although you have to
jump through hoops to get a handle on it anyway), it's just not
something you really want to do very often because it's kinda slow.
Creating a new pool involves allocating about 8k of data, which is
kind of silly if you're just going to use it for one tiny thing then
destroy it.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
On 8/21/06, Lieven Govaerts <lg...@mobsol.be> wrote:
> Attached is a new version of these patches. It's based on the remarks all of
> you made the previous days.
>
> Changes compared to the previous version include:
> - use apr's apr_filepath_root for a portable way to determine if a path is a
> root path.
> - an expanded python test so more problem cases could be solved
> - unit tests (test_path) to validate the changes in the svn_path functions.
>
> The 'working copy is switched' is unchanged compared to previous version.
>
> As I said before, there are probably other scenario's of using wc's on root
> folders we can find that won't work with current patch. I'll try to find
> these scenario's, add them to the python tests and fix the issues in the
> coming days/weeks.
Ok. I didn't investigate more of it myself, so I rely on you to make
sure that we call svn_path_is_root from other functions which don't
have a pool (excluding is_canonical, since Philip said we can safely
remove that function from consideration).
However, calling svn_pool_create(NULL) isn't thread safe AFAIK,
because it allocates from the main pool (is this right, Branko,
anybody?). Or is that the only bit for which the main pool *is*
protected?
Anyway: two other comments inline below:
@@ -418,6 +424,35 @@
}
+svn_boolean_t
+svn_path_is_root(const char *path, apr_size_t len)
+{
+ char *root_path = NULL;
+ apr_status_t status;
+ apr_pool_t *strpool = svn_pool_create(NULL);
+ char *rel_path = apr_pstrmemdup(strpool, path, len);
+
+ /* svn_path_cstring_from_utf8 will create a copy of path.
+
+ It should be safe to convert this error to a false return value. An error
+ in this case would indicate that the path isn't encoded in UTF-8, which
+ will cause problems elsewhere, anyway. */
+ if (svn_path_cstring_from_utf8(&rel_path, rel_path, strpool) != APR_SUCCESS)
Here you may leek a pool.
+ return FALSE;
+
+ status = apr_filepath_root(&root_path, &rel_path, 0, strpool);
+
+ apr_pool_destroy(strpool);
+
+ if ((status == APR_SUCCESS ||
+ status == APR_EINCOMPLETE) &&
+ rel_path[0] == '\0')
Here you access memory which you may have destroyed in
apr_pool_destroy: rel_path may have been allocated in the strpool,
right?
+ return TRUE;
+
+ return FALSE;
+}
+
+
int
svn_path_compare_paths(const char *path1,
const char *path2)
Thanks for your perseverance.
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
Lieven Govaerts wrote:
> Attached is a new version of these patches. It's based on the remarks all of
> you made the previous days.
>
I've committed a slightly expanded version of the regression test from
this patch as XFail in r21156.
When the fix is ready to commit I'll remove the XFail part of the test.
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
Attached is a new version of these patches. It's based on the remarks all of
you made the previous days.
Changes compared to the previous version include:
- use apr's apr_filepath_root for a portable way to determine if a path is a
root path.
- an expanded python test so more problem cases could be solved
- unit tests (test_path) to validate the changes in the svn_path functions.
The 'working copy is switched' is unchanged compared to previous version.
As I said before, there are probably other scenario's of using wc's on root
folders we can find that won't work with current patch. I'll try to find
these scenario's, add them to the python tests and fix the issues in the
coming days/weeks.
> -----Original Message-----
> From: Philip Martin [mailto:philip@codematters.co.uk]
> Sent: maandag 21 augustus 2006 0:00
..
> Lieven Govaerts <lg...@mobsol.be> writes:
>
> > However, there's another reason why I create a subpool. The function
> > is_canonical (private function in path.c) is calling svn_path_is_root,
> > but that function doesn't have a pool available and is called by other
> > functions which don't have pool parameters.
>
> Perhaps it's time to get rid of is_canonical? I added it
> when I was attempting to catch the places where the svn
> client was not passing canonical paths, it was cheap and
> efficient so I just scattered it through the path code. If
> it's not removed then it's use could be restricted to those
> places that do have pools.
I have no opinion on this suggestion, except that it doesn't solve the
problem with the temporary pool, as there are other public functions in
path.c that don't have a pool available.
Lieven.
issue-2556-wc-on-root.patch.txt:
[[[
Prepare fix for issue #2556: abstract root folder checks in
svn_path_is_root.
Add support for 'X:/' as a root folder on Windows.
* subversion/include/svn_path.h
(svn_path_is_root): New function declaration.
(svn_path_is_empty): Added comment.
* subversion/libsvn_client/commit.c
(svn_client_commit4): make sure to abort when the iterating through the
parent folders passese the root folder.
* subversion/libsvn_subr/path.c
(svn_path_is_root): New function. Tests for either '/' on all platforms
or 'X:/' on Windows.
(is_canonical): 'X:/' syntax on Windows is canonical.
(svn_path_join, svn_path_dirname, svn_path_basename,
svn_path_join_many, previous_segment,
get_path_ancestor_length): Support the new type of root path on Windows,
mostly by replacing direct comparisons of path and '/' with a call to
svn_path_is_root.
(svn_path_canonicalize): don't strip the trailing slash if the path is
of the 'X:/' syntax.
* subversion/tests/libsvn_subr/path-test.c
(test_is_root): New test for validation of svn_path_is_root.
(test_path_split, test_join,
test_basename, test_remove_component): add extra cases for 'X:/'.
(test_funcs): run the new test test_is_root.
* subversion/tests/cmdline/update_tests.py
(update_wc_on_windows_drive): New test for issue 2556.
(test_list): add the new test to the list.
]]]
issue-2556-wc-switched.patch.txt:
[[[
Fix an issue where the status of a working copy on the root of a drive is
'S' (switched), as part of issue #2556.
This is not Windows specific, this code didn't work for working copies on
'/' either.
* subversion/libsvn_wc/status.c
(assemble_status): add support for working copies at the root of a
(virtual)
drive.
]]]
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Philip Martin <ph...@codematters.co.uk>.
Lieven Govaerts <lg...@mobsol.be> writes:
> However, there's another reason why I create a subpool. The function
> is_canonical (private function in path.c) is calling svn_path_is_root,
> but that function doesn't have a pool available and is called by other
> functions which don't have pool parameters.
Perhaps it's time to get rid of is_canonical? I added it when I was
attempting to catch the places where the svn client was not passing
canonical paths, it was cheap and efficient so I just scattered it
through the path code. If it's not removed then it's use could be
restricted to those places that do have pools.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
Erik Huelsmann wrote:
>
> I wouldn't bother with the subpool creation: the extra memory required
> is negligeable when compared to other allocated amounts. We generally
> don't create subpools for usecases like this.
>
> (To give you a first review.)
I introduced the subpool because I saw suddenly a huge memory usage,
which I suspected was due the strings not being cleared from the pool.
The problem turned out to be something else though.
However, there's another reason why I create a subpool. The function
is_canonical (private function in path.c) is calling svn_path_is_root,
but that function doesn't have a pool available and is called by other
functions which don't have pool parameters.
I'll include your and Branko's remarks in my next patch.
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Branko Čibej <br...@xbc.nu>.
Lieven Govaerts wrote:
>
>
>
>> -----Original Message-----
>> From: Branko Čibej [mailto:brane@xbc.nu]
>> Sent: zondag 20 augustus 2006 18:38
>>
> ..
>
>
>> * Don't make calling apr_filepath_root Windows-specific; that
>> defeats the whole point of calling that function in the
>> first place.
>>
>
> It was meant as a 'don't punish the guys that are not responsible for the
> problem' thing, but I've removed it now.
>
All the world isn't Unix and Windows. :)
I imagine there are still a few systems out there that use a third
scheme. APR works on OS/2 and Netware, for example; I don't know if
anyone's ported Subversion to those platforms, but if they do, it'll
certainly help to use apr_filepath_root there.
>> * Use svn_path_cstring_from_utf8, which will convert the string from
>>
> internal encoding to whatever APR expects (and is usually a no-op
>
>> on Windows, by the way).
>>
>
> I have a problem here, which I hope you can help me with.
>
> The function svn_path_cstring_from_utf8 returns svn_error_t *, but I don't
> know how to handle that potential error. I've tried to change the return
> type of svn_path_is_root, but that would push the problem to its callers.
> Atleast two of those callers svn_path_basename and svn_path_join are public
> functions, so I can't change those declarations.
>
> I could convert the string somewhere higher in the stack, but that doesn't
> seem right either.
>
> Suggestions?
>
I think you can safely convert an error to a false return value. An
error in this case would indicate that the path isn't encoded in UTF-8,
which will cause problems elsewhere, anyway.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@xbc.nu]
> Sent: zondag 20 augustus 2006 18:38
..
> * Don't make calling apr_filepath_root Windows-specific; that
> defeats the whole point of calling that function in the
> first place.
It was meant as a 'don't punish the guys that are not responsible for the
problem' thing, but I've removed it now.
> * Use svn_path_cstring_from_utf8, which will convert the string from
internal encoding to whatever APR expects (and is usually a no-op
> on Windows, by the way).
I have a problem here, which I hope you can help me with.
The function svn_path_cstring_from_utf8 returns svn_error_t *, but I don't
know how to handle that potential error. I've tried to change the return
type of svn_path_is_root, but that would push the problem to its callers.
Atleast two of those callers svn_path_basename and svn_path_join are public
functions, so I can't change those declarations.
I could convert the string somewhere higher in the stack, but that doesn't
seem right either.
Suggestions?
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Branko Čibej <br...@xbc.nu>.
Erik Huelsmann wrote:
>> > I don't like the idea of rolling our own function for
>> > something that APR already does. The overhead is trivial when
>> > compared to the extra functionality *and portability* of
>> > apr_filepath_root. I think you need to prove that the
>> > overhead is unacceptable performance-wise.
>> >
>> I think you misunderstood me, the svn_path_is_root function is a wrapper
>> around apr_file_path_root. This is the current version (probably the one
>> that's going to be included in the patch):
>>
>> svn_boolean_t
>> svn_path_is_root(const char *path, apr_pool_t *pool)
>> {
>> #if defined(WIN32)
>> char *root_path = NULL;
>> apr_pool_t *subpool = svn_pool_create(pool);
>> char *rel_path = apr_pstrmemdup(subpool, path, strlen(path));
>>
>> apr_status_t status = apr_filepath_root(&root_path, &rel_path, 0,
>> subpool);
>>
>> apr_pool_destroy(subpool);
>>
>> if ((status == APR_SUCCESS ||
>> status == APR_EINCOMPLETE) &&
>> rel_path[0] == 0)
>> return TRUE;
>>
>> return FALSE;
>> #endif /* WIN32 */
>>
>> return path[0] == '/' && path[1] == 0;
>> }
>
> I wouldn't bother with the subpool creation: the extra memory required
> is negligeable when compared to other allocated amounts. We generally
> don't create subpools for usecases like this.
Right. I have a few other comments:
* Don't make calling apr_filepath_root Windows-specific; that
defeats the whole point of calling that function in the first place.
* Use svn_path_cstring_from_utf8, which will convert the string from
internal encoding to whatever APR expects (and is usually a no-op
on Windows, by the way).
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
> > I don't like the idea of rolling our own function for
> > something that APR already does. The overhead is trivial when
> > compared to the extra functionality *and portability* of
> > apr_filepath_root. I think you need to prove that the
> > overhead is unacceptable performance-wise.
> >
> I think you misunderstood me, the svn_path_is_root function is a wrapper
> around apr_file_path_root. This is the current version (probably the one
> that's going to be included in the patch):
>
> svn_boolean_t
> svn_path_is_root(const char *path, apr_pool_t *pool)
> {
> #if defined(WIN32)
> char *root_path = NULL;
> apr_pool_t *subpool = svn_pool_create(pool);
> char *rel_path = apr_pstrmemdup(subpool, path, strlen(path));
>
> apr_status_t status = apr_filepath_root(&root_path, &rel_path, 0,
> subpool);
>
> apr_pool_destroy(subpool);
>
> if ((status == APR_SUCCESS ||
> status == APR_EINCOMPLETE) &&
> rel_path[0] == 0)
> return TRUE;
>
> return FALSE;
> #endif /* WIN32 */
>
> return path[0] == '/' && path[1] == 0;
> }
I wouldn't bother with the subpool creation: the extra memory required
is negligeable when compared to other allocated amounts. We generally
don't create subpools for usecases like this.
(To give you a first review.)
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@xbc.nu]
> Sent: zondag 20 augustus 2006 18:15
> To: Lieven Govaerts
> > I'm gonna stick to the svn_path_is_root function because of the
> > overhead needed to use apr_filepath_root.
> >
>
> I don't like the idea of rolling our own function for
> something that APR already does. The overhead is trivial when
> compared to the extra functionality *and portability* of
> apr_filepath_root. I think you need to prove that the
> overhead is unacceptable performance-wise.
>
I think you misunderstood me, the svn_path_is_root function is a wrapper
around apr_file_path_root. This is the current version (probably the one
that's going to be included in the patch):
svn_boolean_t
svn_path_is_root(const char *path, apr_pool_t *pool)
{
#if defined(WIN32)
char *root_path = NULL;
apr_pool_t *subpool = svn_pool_create(pool);
char *rel_path = apr_pstrmemdup(subpool, path, strlen(path));
apr_status_t status = apr_filepath_root(&root_path, &rel_path, 0,
subpool);
apr_pool_destroy(subpool);
if ((status == APR_SUCCESS ||
status == APR_EINCOMPLETE) &&
rel_path[0] == 0)
return TRUE;
return FALSE;
#endif /* WIN32 */
return path[0] == '/' && path[1] == 0;
}
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Branko Čibej <br...@xbc.nu>.
Lieven Govaerts wrote:
> > -----Original Message-----
>
>> From: Erik Huelsmann [mailto:ehuels@gmail.com]
>> Sent: zondag 20 augustus 2006 17:53
>>
>
>
>>> Why don't you just use apr_filepath_root here and everywhere else
>>> where this matters?
>>>
>> In that case: why introduce a function in the svn_ namespace,
>> if there's a function in APR which does exactly what we want?
>> But, isn't it a problem that our paths are UTF-8 while APR
>> expects localized paths?
>>
>> Lieven: apr_filepath_root will give the root of a given
>> inpath, maybe it'll work, because then it will also work for
>> working copies in the root of a share or UNC path...
>>
>
> I'm working on it now, will have a patch tonight.
>
> The apr_filepath_root is more then we want, because it also splits the path
> in two parts, so we need to allocate new strings, pass a pool to the
> function etc. But, it supports more Windows path conventions than the one I
> implemented, so let's use it.
>
> I'm gonna stick to the svn_path_is_root function because of the overhead
> needed to use apr_filepath_root.
>
I don't like the idea of rolling our own function for something that APR
already does. The overhead is trivial when compared to the extra
functionality *and portability* of apr_filepath_root. I think you need
to prove that the overhead is unacceptable performance-wise.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Lieven Govaerts <lg...@mobsol.be>.
> -----Original Message-----
> From: Erik Huelsmann [mailto:ehuels@gmail.com]
> Sent: zondag 20 augustus 2006 17:53
> >
> > Why don't you just use apr_filepath_root here and everywhere else
> > where this matters?
>
>
> In that case: why introduce a function in the svn_ namespace,
> if there's a function in APR which does exactly what we want?
> But, isn't it a problem that our paths are UTF-8 while APR
> expects localized paths?
>
> Lieven: apr_filepath_root will give the root of a given
> inpath, maybe it'll work, because then it will also work for
> working copies in the root of a share or UNC path...
I'm working on it now, will have a patch tonight.
The apr_filepath_root is more then we want, because it also splits the path
in two parts, so we need to allocate new strings, pass a pool to the
function etc. But, it supports more Windows path conventions than the one I
implemented, so let's use it.
I'm gonna stick to the svn_path_is_root function because of the overhead
needed to use apr_filepath_root.
Lieven.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive
Posted by Erik Huelsmann <eh...@gmail.com>.
On 8/19/06, Branko Čibej <br...@xbc.nu> wrote:
> Lieven Govaerts wrote:
> > +int
> > +svn_path_is_root(const char *path)
> > +{
> > +#if defined(WIN32)
> > + /* On Windows, X:/ is a root path! */
> > + if (path[1] == ':' && path[2] == '/' && path[3] == 0)
> > + return 1;
> > +#endif /* WIN32 */
> >
> > + return path[0] == '/' && path[1] == 0;
> > +}
> > +
> >
>
> Why don't you just use apr_filepath_root here and everywhere else where
> this matters?
In that case: why introduce a function in the svn_ namespace, if
there's a function in APR which does exactly what we want? But, isn't
it a problem that our paths are UTF-8 while APR expects localized
paths?
Lieven: apr_filepath_root will give the root of a given inpath, maybe
it'll work, because then it will also work for working copies in the
root of a share or UNC path...
Well, I was about to +1 for commit, but maybe someone has a bit of
time to test this first? Lieven, do you?
Thanks in advance!
bye,
Erik.
Re: [PATCH] fix for issue #2556: support working copies on the root
of a (virtual) drive
Posted by Branko Čibej <br...@xbc.nu>.
Lieven Govaerts wrote:
> +int
> +svn_path_is_root(const char *path)
> +{
> +#if defined(WIN32)
> + /* On Windows, X:/ is a root path! */
> + if (path[1] == ':' && path[2] == '/' && path[3] == 0)
> + return 1;
> +#endif /* WIN32 */
>
> + return path[0] == '/' && path[1] == 0;
> +}
> +
>
Why don't you just use apr_filepath_root here and everywhere else where
this matters?
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org