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.
]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Aug 21 23:52:55 2006