You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Burley <bu...@geek.net> on 2010/12/08 23:03:06 UTC
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Greetings,
So I have forward-ported Stephane's patch to add recursion to SVNParentPath
for 1.6.15 (see attached patch). I have also started, but had to stop, work
on making it function with trunk (see also, attached patch). The 1.6.15
patch works, the trunk patch does not (some functions take different
arguments now, it seems -- and that may be part of it since I excised those
bits entirely). Anyways, I am hoping at minimum that this will allow the
patch to be used by someone else who needs this functionality. Ideally
someone with some better knowledge of the SVN coding standards and internals
would finish this for us and include it into the base distribution when they
have time. If I have or am given such time in the future, I will do that --
but the time isn't now unfortunately.
As for Glasser's prior concern that this would change the functionality and
potentially cause some security issues -- my response would be to add it to
a new devel line and note it as a functional change. I don't think there's
much use or safety in storing the repository in a deeper dir of something
that is apache accessible anyways -- so this would effectively make things
function more like I think should be expected anyways. I am going to also
attach these patches to the bug below.
Cheers,
David Burley
Systems Programmer/Analyst, Geeknet, Inc.
david@geek.net
References to others with same request:
http://subversion.tigris.org/issues/show_bug.cgi?id=3588
http://serverfault.com/questions/68818/is-svnparentpath-recursive
Original Email:
http://svn.haxx.se/dev/archive-2007-12/0280.shtml
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Posted by Gavin Beau Baumanis <ga...@thespidernet.com>.
HI David,
As the Patch Manager, I notice that your latest patch proposal has still has not received any comments.
At this stage I would normally attach the patch to a new / existing issue and pass the details back into this thread.
Since you have previously used the issue tracker, would you like to re-test your patch against the trunk / 1.6.x branch and add it to the issue tracker, please?
I am happy to the do the issue tracker if you like, but you'll need to do the retesting anyway - since it has been a little while now that the submission has been sitting around.
If there is anything that I can do to assist you, please let me know.
Gavin.
On 09/12/2010, at 11:23 PM, David Burley wrote:
> They got mangled on the way to my mail client -- I tested them again locally before attaching them this time -- and they are now unmangled.
>
> Thank you,
>
> David
>
> On Thu, Dec 9, 2010 at 2:38 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> David Burley wrote on Wed, Dec 08, 2010 at 19:37:51 -0500:
> > Patches attached again but can also be found here:
> >
> > http://subversion.tigris.org/issues/show_bug.cgi?id=3588
> >
> > On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> >
> > > David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> > > > So I have forward-ported Stephane's patch to add recursion to
> > > SVNParentPath
> > > > for 1.6.15 (see attached patch).
> > >
> > > The patch didn't get to the list. Please re-send it with *.txt extension.
> > >
> > > Thanks.
> > >
> > > Daniel
> > >
>
> The 1.7.x patch doesn't apply to HEAD of trunk.
>
> > Index: subversion/mod_dav_svn/repos.c
> > ===================================================================
> > --- subversion/mod_dav_svn/repos.c (revision 1043620)
> > +++ subversion/mod_dav_svn/repos.c (working copy)
> > @@ -1235,34 +1235,63 @@
> > {
> > /* SVNParentPath was used instead: assume the first component of
> > 'relative' is the name of a repository. */
> > - const char *magic_component, *magic_end;
> > + extern svn_boolean_t check_repos_path(const char *path, apr_pool_t *pool);
> > + const char *magic_component = NULL, *magic_end;
> > + char *repos_path;
> >
> > - /* A repository name is required here.
> > - Remember that 'relative' always starts with a "/". */
> > - if (relative[1] == '\0')
> > - {
> > - /* ### are SVN_ERR_APMOD codes within the right numeric space? */
> > - return dav_svn__new_error(r->pool, HTTP_FORBIDDEN,
> > - SVN_ERR_APMOD_MALFORMED_URI,
> > - "The URI does not contain the name "
> > - "of a repository.");
> > - }
> > + do
> > + {
> > + /* A repository name is required here.
> > + Remember that 'relative' always starts with a "/". */
> > + if (relative[1] == '\0')
> > + {
> > + /* ### are SVN_ERR_APMOD codes within the right numeric space? */
> > + return dav_new_error(r->pool, HTTP_FORBIDDEN,
> > + SVN_ERR_APMOD_MALFORMED_URI,
> > + "The URI does not contain the name "
> > + "of a repository.");
> > + }
> > +
> > + magic_end = ap_strchr_c(relative + 1, '/');
> > + if (!magic_end)
> > + {
> > + /* ### Request was for parent directory with no trailing
> > + slash; we probably ought to just redirect to same with
> > + trailing slash appended. */
> > + if (!magic_component)
> > + {
> > + magic_component = relative + 1;
> > + }
> > + else
> > + {
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + relative, NULL);
> > + }
> > + relative = "/";
> > + }
> > + else
> > + {
> > + if (!magic_component)
> > + {
> > + magic_component = apr_pstrndup(r->pool, relative + 1,
> > + magic_end - relative - 1);
> > + }
> > + else
> > + {
> > + char *tmp_magic_component;
> >
> > - magic_end = ap_strchr_c(relative + 1, '/');
> > - if (!magic_end)
> > - {
> > - /* ### Request was for parent directory with no trailing
> > - slash; we probably ought to just redirect to same with
> > - trailing slash appended. */
> > - magic_component = relative + 1;
> > - relative = "/";
> > - }
> > - else
> > - {
> > - magic_component = apr_pstrndup(r->pool, relative + 1,
> > - magic_end - relative - 1);
> > - relative = magic_end;
> > - }
> > + tmp_magic_component = apr_pstrndup(r->pool, relative,
> > + magic_end - relative);
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + tmp_magic_component, NULL);
> > + }
> > + relative = magic_end;
> > + }
> > +
> > + repos_path = svn_path_join(fs_parent_path, magic_component,
> > + r->pool);
> > + }
> > + while (check_repos_path(repos_path, r->pool) != TRUE);
> >
> > /* return answer */
> > *repos_basename = magic_component;
> > @@ -1881,7 +1910,7 @@
> > dav_resource_combined *comb;
> > dav_svn_repos *repos;
> > const char *cleaned_uri;
> > - const char *repo_basename;
> > + const char *repo_basename = NULL;
> > const char *relative;
> > const char *repos_path;
> > const char *repos_key;
> > @@ -1897,9 +1926,15 @@
> > xslt_uri = dav_svn__get_xslt_uri(r);
> > fs_parent_path = dav_svn__get_fs_parent_path(r);
> >
> > + /* This does all the work of interpreting/splitting the request uri. */
> > + err = dav_svn_split_uri(r, r->uri, root_path,
> > + &cleaned_uri, &had_slash,
> > + &repo_basename, &relative, &repos_path);
> > +
> > +
> > /* Special case: detect and build the SVNParentPath as a unique type
> > of private resource, iff the SVNListParentPath directive is 'on'. */
> > - if (fs_parent_path && dav_svn__get_list_parentpath_flag(r))
> > + if (fs_parent_path && dav_svn__get_list_parentpath_flag(r) && !repo_basename)
> > {
> > char *uri = apr_pstrdup(r->pool, r->uri);
> > char *parentpath = apr_pstrdup(r->pool, root_path);
> > @@ -1912,13 +1947,10 @@
> > if (parentpath[parentpath_len-1] == '/')
> > parentpath[parentpath_len-1] = '\0';
> >
> > - if (strcmp(parentpath, uri) == 0)
> > - {
> > - err = get_parentpath_resource(r, resource);
> > - if (err)
> > - return err;
> > - return NULL;
> > - }
> > + err = get_parentpath_resource(r, resource);
> > + if (err)
> > + return err;
> > + return NULL;
> > }
> >
> > /* This does all the work of interpreting/splitting the request uri. */
> > @@ -3161,8 +3193,13 @@
> > {
> > apr_hash_index_t *hi;
> > apr_hash_t *dirents;
> > + int root_path_len = strlen(resource->info->repos->root_path);
> > const char *fs_parent_path =
> > - dav_svn__get_fs_parent_path(resource->info->r);
> > + apr_pstrcat(resource->info->r->pool,
> > + dav_svn__get_fs_parent_path(resource->info->r),
> > + resource->info->r->uri
> > + + ((root_path_len > 1) ? root_path_len : 0),
> > + NULL);
> >
> > serr = svn_io_get_dirents3(&dirents, fs_parent_path, TRUE,
> > resource->pool, resource->pool);
> > Index: subversion/libsvn_repos/repos.c
> > ===================================================================
> > --- subversion/libsvn_repos/repos.c (revision 1043620)
> > +++ subversion/libsvn_repos/repos.c (working copy)
> > @@ -1428,7 +1428,7 @@
> > * on errors (which would be permission errors, probably) so that
> > * we the user will see them after we try to open the repository
> > * for real. */
> > -static svn_boolean_t
> > +svn_boolean_t
> > check_repos_path(const char *path,
> > apr_pool_t *pool)
>
> As earlier reviewers told you, we do not define functions in one library
> and use them in another unless they have a svn_ svn_foo__ name prefix.
>
> > {
> >
>
>
> <svnparentpath-recursion-1.6.15.patch.txt><svnparentpath-recursion-1.7.x.patch.txt>
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Posted by Gavin Beau Baumanis <ga...@thespidernet.com>.
Ping. This patch is yet to receive any new comments.
It seems that David has already attached the patches to issue #3588
Gavin "Beau" Baumanis
On 09/12/2010, at 11:23 PM, David Burley wrote:
> They got mangled on the way to my mail client -- I tested them again locally before attaching them this time -- and they are now unmangled.
>
> Thank you,
>
> David
>
> On Thu, Dec 9, 2010 at 2:38 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> David Burley wrote on Wed, Dec 08, 2010 at 19:37:51 -0500:
> > Patches attached again but can also be found here:
> >
> > http://subversion.tigris.org/issues/show_bug.cgi?id=3588
> >
> > On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> >
> > > David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> > > > So I have forward-ported Stephane's patch to add recursion to
> > > SVNParentPath
> > > > for 1.6.15 (see attached patch).
> > >
> > > The patch didn't get to the list. Please re-send it with *.txt extension.
> > >
> > > Thanks.
> > >
> > > Daniel
> > >
>
> The 1.7.x patch doesn't apply to HEAD of trunk.
>
> > Index: subversion/mod_dav_svn/repos.c
> > ===================================================================
> > --- subversion/mod_dav_svn/repos.c (revision 1043620)
> > +++ subversion/mod_dav_svn/repos.c (working copy)
> > @@ -1235,34 +1235,63 @@
> > {
> > /* SVNParentPath was used instead: assume the first component of
> > 'relative' is the name of a repository. */
> > - const char *magic_component, *magic_end;
> > + extern svn_boolean_t check_repos_path(const char *path, apr_pool_t *pool);
> > + const char *magic_component = NULL, *magic_end;
> > + char *repos_path;
> >
> > - /* A repository name is required here.
> > - Remember that 'relative' always starts with a "/". */
> > - if (relative[1] == '\0')
> > - {
> > - /* ### are SVN_ERR_APMOD codes within the right numeric space? */
> > - return dav_svn__new_error(r->pool, HTTP_FORBIDDEN,
> > - SVN_ERR_APMOD_MALFORMED_URI,
> > - "The URI does not contain the name "
> > - "of a repository.");
> > - }
> > + do
> > + {
> > + /* A repository name is required here.
> > + Remember that 'relative' always starts with a "/". */
> > + if (relative[1] == '\0')
> > + {
> > + /* ### are SVN_ERR_APMOD codes within the right numeric space? */
> > + return dav_new_error(r->pool, HTTP_FORBIDDEN,
> > + SVN_ERR_APMOD_MALFORMED_URI,
> > + "The URI does not contain the name "
> > + "of a repository.");
> > + }
> > +
> > + magic_end = ap_strchr_c(relative + 1, '/');
> > + if (!magic_end)
> > + {
> > + /* ### Request was for parent directory with no trailing
> > + slash; we probably ought to just redirect to same with
> > + trailing slash appended. */
> > + if (!magic_component)
> > + {
> > + magic_component = relative + 1;
> > + }
> > + else
> > + {
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + relative, NULL);
> > + }
> > + relative = "/";
> > + }
> > + else
> > + {
> > + if (!magic_component)
> > + {
> > + magic_component = apr_pstrndup(r->pool, relative + 1,
> > + magic_end - relative - 1);
> > + }
> > + else
> > + {
> > + char *tmp_magic_component;
> >
> > - magic_end = ap_strchr_c(relative + 1, '/');
> > - if (!magic_end)
> > - {
> > - /* ### Request was for parent directory with no trailing
> > - slash; we probably ought to just redirect to same with
> > - trailing slash appended. */
> > - magic_component = relative + 1;
> > - relative = "/";
> > - }
> > - else
> > - {
> > - magic_component = apr_pstrndup(r->pool, relative + 1,
> > - magic_end - relative - 1);
> > - relative = magic_end;
> > - }
> > + tmp_magic_component = apr_pstrndup(r->pool, relative,
> > + magic_end - relative);
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + tmp_magic_component, NULL);
> > + }
> > + relative = magic_end;
> > + }
> > +
> > + repos_path = svn_path_join(fs_parent_path, magic_component,
> > + r->pool);
> > + }
> > + while (check_repos_path(repos_path, r->pool) != TRUE);
> >
> > /* return answer */
> > *repos_basename = magic_component;
> > @@ -1881,7 +1910,7 @@
> > dav_resource_combined *comb;
> > dav_svn_repos *repos;
> > const char *cleaned_uri;
> > - const char *repo_basename;
> > + const char *repo_basename = NULL;
> > const char *relative;
> > const char *repos_path;
> > const char *repos_key;
> > @@ -1897,9 +1926,15 @@
> > xslt_uri = dav_svn__get_xslt_uri(r);
> > fs_parent_path = dav_svn__get_fs_parent_path(r);
> >
> > + /* This does all the work of interpreting/splitting the request uri. */
> > + err = dav_svn_split_uri(r, r->uri, root_path,
> > + &cleaned_uri, &had_slash,
> > + &repo_basename, &relative, &repos_path);
> > +
> > +
> > /* Special case: detect and build the SVNParentPath as a unique type
> > of private resource, iff the SVNListParentPath directive is 'on'. */
> > - if (fs_parent_path && dav_svn__get_list_parentpath_flag(r))
> > + if (fs_parent_path && dav_svn__get_list_parentpath_flag(r) && !repo_basename)
> > {
> > char *uri = apr_pstrdup(r->pool, r->uri);
> > char *parentpath = apr_pstrdup(r->pool, root_path);
> > @@ -1912,13 +1947,10 @@
> > if (parentpath[parentpath_len-1] == '/')
> > parentpath[parentpath_len-1] = '\0';
> >
> > - if (strcmp(parentpath, uri) == 0)
> > - {
> > - err = get_parentpath_resource(r, resource);
> > - if (err)
> > - return err;
> > - return NULL;
> > - }
> > + err = get_parentpath_resource(r, resource);
> > + if (err)
> > + return err;
> > + return NULL;
> > }
> >
> > /* This does all the work of interpreting/splitting the request uri. */
> > @@ -3161,8 +3193,13 @@
> > {
> > apr_hash_index_t *hi;
> > apr_hash_t *dirents;
> > + int root_path_len = strlen(resource->info->repos->root_path);
> > const char *fs_parent_path =
> > - dav_svn__get_fs_parent_path(resource->info->r);
> > + apr_pstrcat(resource->info->r->pool,
> > + dav_svn__get_fs_parent_path(resource->info->r),
> > + resource->info->r->uri
> > + + ((root_path_len > 1) ? root_path_len : 0),
> > + NULL);
> >
> > serr = svn_io_get_dirents3(&dirents, fs_parent_path, TRUE,
> > resource->pool, resource->pool);
> > Index: subversion/libsvn_repos/repos.c
> > ===================================================================
> > --- subversion/libsvn_repos/repos.c (revision 1043620)
> > +++ subversion/libsvn_repos/repos.c (working copy)
> > @@ -1428,7 +1428,7 @@
> > * on errors (which would be permission errors, probably) so that
> > * we the user will see them after we try to open the repository
> > * for real. */
> > -static svn_boolean_t
> > +svn_boolean_t
> > check_repos_path(const char *path,
> > apr_pool_t *pool)
>
> As earlier reviewers told you, we do not define functions in one library
> and use them in another unless they have a svn_ svn_foo__ name prefix.
>
> > {
> >
>
>
> <svnparentpath-recursion-1.6.15.patch.txt><svnparentpath-recursion-1.7.x.patch.txt>
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Posted by Gavin Beau Baumanis <ga...@thespidernet.com>.
Ping.
This Submission has received no new comments.
On 09/12/2010, at 11:23 PM, David Burley wrote:
> They got mangled on the way to my mail client -- I tested them again locally before attaching them this time -- and they are now unmangled.
>
> Thank you,
>
> David
>
> On Thu, Dec 9, 2010 at 2:38 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> David Burley wrote on Wed, Dec 08, 2010 at 19:37:51 -0500:
> > Patches attached again but can also be found here:
> >
> > http://subversion.tigris.org/issues/show_bug.cgi?id=3588
> >
> > On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> >
> > > David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> > > > So I have forward-ported Stephane's patch to add recursion to
> > > SVNParentPath
> > > > for 1.6.15 (see attached patch).
> > >
> > > The patch didn't get to the list. Please re-send it with *.txt extension.
> > >
> > > Thanks.
> > >
> > > Daniel
> > >
>
> The 1.7.x patch doesn't apply to HEAD of trunk.
>
> > Index: subversion/mod_dav_svn/repos.c
> > ===================================================================
> > --- subversion/mod_dav_svn/repos.c (revision 1043620)
> > +++ subversion/mod_dav_svn/repos.c (working copy)
> > @@ -1235,34 +1235,63 @@
> > {
> > /* SVNParentPath was used instead: assume the first component of
> > 'relative' is the name of a repository. */
> > - const char *magic_component, *magic_end;
> > + extern svn_boolean_t check_repos_path(const char *path, apr_pool_t *pool);
> > + const char *magic_component = NULL, *magic_end;
> > + char *repos_path;
> >
> > - /* A repository name is required here.
> > - Remember that 'relative' always starts with a "/". */
> > - if (relative[1] == '\0')
> > - {
> > - /* ### are SVN_ERR_APMOD codes within the right numeric space? */
> > - return dav_svn__new_error(r->pool, HTTP_FORBIDDEN,
> > - SVN_ERR_APMOD_MALFORMED_URI,
> > - "The URI does not contain the name "
> > - "of a repository.");
> > - }
> > + do
> > + {
> > + /* A repository name is required here.
> > + Remember that 'relative' always starts with a "/". */
> > + if (relative[1] == '\0')
> > + {
> > + /* ### are SVN_ERR_APMOD codes within the right numeric space? */
> > + return dav_new_error(r->pool, HTTP_FORBIDDEN,
> > + SVN_ERR_APMOD_MALFORMED_URI,
> > + "The URI does not contain the name "
> > + "of a repository.");
> > + }
> > +
> > + magic_end = ap_strchr_c(relative + 1, '/');
> > + if (!magic_end)
> > + {
> > + /* ### Request was for parent directory with no trailing
> > + slash; we probably ought to just redirect to same with
> > + trailing slash appended. */
> > + if (!magic_component)
> > + {
> > + magic_component = relative + 1;
> > + }
> > + else
> > + {
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + relative, NULL);
> > + }
> > + relative = "/";
> > + }
> > + else
> > + {
> > + if (!magic_component)
> > + {
> > + magic_component = apr_pstrndup(r->pool, relative + 1,
> > + magic_end - relative - 1);
> > + }
> > + else
> > + {
> > + char *tmp_magic_component;
> >
> > - magic_end = ap_strchr_c(relative + 1, '/');
> > - if (!magic_end)
> > - {
> > - /* ### Request was for parent directory with no trailing
> > - slash; we probably ought to just redirect to same with
> > - trailing slash appended. */
> > - magic_component = relative + 1;
> > - relative = "/";
> > - }
> > - else
> > - {
> > - magic_component = apr_pstrndup(r->pool, relative + 1,
> > - magic_end - relative - 1);
> > - relative = magic_end;
> > - }
> > + tmp_magic_component = apr_pstrndup(r->pool, relative,
> > + magic_end - relative);
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + tmp_magic_component, NULL);
> > + }
> > + relative = magic_end;
> > + }
> > +
> > + repos_path = svn_path_join(fs_parent_path, magic_component,
> > + r->pool);
> > + }
> > + while (check_repos_path(repos_path, r->pool) != TRUE);
> >
> > /* return answer */
> > *repos_basename = magic_component;
> > @@ -1881,7 +1910,7 @@
> > dav_resource_combined *comb;
> > dav_svn_repos *repos;
> > const char *cleaned_uri;
> > - const char *repo_basename;
> > + const char *repo_basename = NULL;
> > const char *relative;
> > const char *repos_path;
> > const char *repos_key;
> > @@ -1897,9 +1926,15 @@
> > xslt_uri = dav_svn__get_xslt_uri(r);
> > fs_parent_path = dav_svn__get_fs_parent_path(r);
> >
> > + /* This does all the work of interpreting/splitting the request uri. */
> > + err = dav_svn_split_uri(r, r->uri, root_path,
> > + &cleaned_uri, &had_slash,
> > + &repo_basename, &relative, &repos_path);
> > +
> > +
> > /* Special case: detect and build the SVNParentPath as a unique type
> > of private resource, iff the SVNListParentPath directive is 'on'. */
> > - if (fs_parent_path && dav_svn__get_list_parentpath_flag(r))
> > + if (fs_parent_path && dav_svn__get_list_parentpath_flag(r) && !repo_basename)
> > {
> > char *uri = apr_pstrdup(r->pool, r->uri);
> > char *parentpath = apr_pstrdup(r->pool, root_path);
> > @@ -1912,13 +1947,10 @@
> > if (parentpath[parentpath_len-1] == '/')
> > parentpath[parentpath_len-1] = '\0';
> >
> > - if (strcmp(parentpath, uri) == 0)
> > - {
> > - err = get_parentpath_resource(r, resource);
> > - if (err)
> > - return err;
> > - return NULL;
> > - }
> > + err = get_parentpath_resource(r, resource);
> > + if (err)
> > + return err;
> > + return NULL;
> > }
> >
> > /* This does all the work of interpreting/splitting the request uri. */
> > @@ -3161,8 +3193,13 @@
> > {
> > apr_hash_index_t *hi;
> > apr_hash_t *dirents;
> > + int root_path_len = strlen(resource->info->repos->root_path);
> > const char *fs_parent_path =
> > - dav_svn__get_fs_parent_path(resource->info->r);
> > + apr_pstrcat(resource->info->r->pool,
> > + dav_svn__get_fs_parent_path(resource->info->r),
> > + resource->info->r->uri
> > + + ((root_path_len > 1) ? root_path_len : 0),
> > + NULL);
> >
> > serr = svn_io_get_dirents3(&dirents, fs_parent_path, TRUE,
> > resource->pool, resource->pool);
> > Index: subversion/libsvn_repos/repos.c
> > ===================================================================
> > --- subversion/libsvn_repos/repos.c (revision 1043620)
> > +++ subversion/libsvn_repos/repos.c (working copy)
> > @@ -1428,7 +1428,7 @@
> > * on errors (which would be permission errors, probably) so that
> > * we the user will see them after we try to open the repository
> > * for real. */
> > -static svn_boolean_t
> > +svn_boolean_t
> > check_repos_path(const char *path,
> > apr_pool_t *pool)
>
> As earlier reviewers told you, we do not define functions in one library
> and use them in another unless they have a svn_ svn_foo__ name prefix.
>
> > {
> >
>
>
> <svnparentpath-recursion-1.6.15.patch.txt><svnparentpath-recursion-1.7.x.patch.txt>
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Posted by David Burley <bu...@geek.net>.
They got mangled on the way to my mail client -- I tested them again locally
before attaching them this time -- and they are now unmangled.
Thank you,
David
On Thu, Dec 9, 2010 at 2:38 AM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> David Burley wrote on Wed, Dec 08, 2010 at 19:37:51 -0500:
> > Patches attached again but can also be found here:
> >
> > http://subversion.tigris.org/issues/show_bug.cgi?id=3588
> >
> > On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf <d.s@daniel.shahaf.name
> >wrote:
> >
> > > David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> > > > So I have forward-ported Stephane's patch to add recursion to
> > > SVNParentPath
> > > > for 1.6.15 (see attached patch).
> > >
> > > The patch didn't get to the list. Please re-send it with *.txt
> extension.
> > >
> > > Thanks.
> > >
> > > Daniel
> > >
>
> The 1.7.x patch doesn't apply to HEAD of trunk.
>
> > Index: subversion/mod_dav_svn/repos.c
> > ===================================================================
> > --- subversion/mod_dav_svn/repos.c (revision 1043620)
> > +++ subversion/mod_dav_svn/repos.c (working copy)
> > @@ -1235,34 +1235,63 @@
> > {
> > /* SVNParentPath was used instead: assume the first component of
> > 'relative' is the name of a repository. */
> > - const char *magic_component, *magic_end;
> > + extern svn_boolean_t check_repos_path(const char *path, apr_pool_t
> *pool);
> > + const char *magic_component = NULL, *magic_end;
> > + char *repos_path;
> >
> > - /* A repository name is required here.
> > - Remember that 'relative' always starts with a "/". */
> > - if (relative[1] == '\0')
> > - {
> > - /* ### are SVN_ERR_APMOD codes within the right numeric space?
> */
> > - return dav_svn__new_error(r->pool, HTTP_FORBIDDEN,
> > - SVN_ERR_APMOD_MALFORMED_URI,
> > - "The URI does not contain the name "
> > - "of a repository.");
> > - }
> > + do
> > + {
> > + /* A repository name is required here.
> > + Remember that 'relative' always starts with a "/". */
> > + if (relative[1] == '\0')
> > + {
> > + /* ### are SVN_ERR_APMOD codes within the right numeric
> space? */
> > + return dav_new_error(r->pool, HTTP_FORBIDDEN,
> > + SVN_ERR_APMOD_MALFORMED_URI,
> > + "The URI does not contain the name "
> > + "of a repository.");
> > + }
> > +
> > + magic_end = ap_strchr_c(relative + 1, '/');
> > + if (!magic_end)
> > + {
> > + /* ### Request was for parent directory with no trailing
> > + slash; we probably ought to just redirect to same with
> > + trailing slash appended. */
> > + if (!magic_component)
> > + {
> > + magic_component = relative + 1;
> > + }
> > + else
> > + {
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + relative, NULL);
> > + }
> > + relative = "/";
> > + }
> > + else
> > + {
> > + if (!magic_component)
> > + {
> > + magic_component = apr_pstrndup(r->pool, relative + 1,
> > + magic_end - relative -
> 1);
> > + }
> > + else
> > + {
> > + char *tmp_magic_component;
> >
> > - magic_end = ap_strchr_c(relative + 1, '/');
> > - if (!magic_end)
> > - {
> > - /* ### Request was for parent directory with no trailing
> > - slash; we probably ought to just redirect to same with
> > - trailing slash appended. */
> > - magic_component = relative + 1;
> > - relative = "/";
> > - }
> > - else
> > - {
> > - magic_component = apr_pstrndup(r->pool, relative + 1,
> > - magic_end - relative - 1);
> > - relative = magic_end;
> > - }
> > + tmp_magic_component = apr_pstrndup(r->pool, relative,
> > + magic_end -
> relative);
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + tmp_magic_component,
> NULL);
> > + }
> > + relative = magic_end;
> > + }
> > +
> > + repos_path = svn_path_join(fs_parent_path, magic_component,
> > + r->pool);
> > + }
> > + while (check_repos_path(repos_path, r->pool) != TRUE);
> >
> > /* return answer */
> > *repos_basename = magic_component;
> > @@ -1881,7 +1910,7 @@
> > dav_resource_combined *comb;
> > dav_svn_repos *repos;
> > const char *cleaned_uri;
> > - const char *repo_basename;
> > + const char *repo_basename = NULL;
> > const char *relative;
> > const char *repos_path;
> > const char *repos_key;
> > @@ -1897,9 +1926,15 @@
> > xslt_uri = dav_svn__get_xslt_uri(r);
> > fs_parent_path = dav_svn__get_fs_parent_path(r);
> >
> > + /* This does all the work of interpreting/splitting the request uri.
> */
> > + err = dav_svn_split_uri(r, r->uri, root_path,
> > + &cleaned_uri, &had_slash,
> > + &repo_basename, &relative, &repos_path);
> > +
> > +
> > /* Special case: detect and build the SVNParentPath as a unique type
> > of private resource, iff the SVNListParentPath directive is 'on'. */
> > - if (fs_parent_path && dav_svn__get_list_parentpath_flag(r))
> > + if (fs_parent_path && dav_svn__get_list_parentpath_flag(r) &&
> !repo_basename)
> > {
> > char *uri = apr_pstrdup(r->pool, r->uri);
> > char *parentpath = apr_pstrdup(r->pool, root_path);
> > @@ -1912,13 +1947,10 @@
> > if (parentpath[parentpath_len-1] == '/')
> > parentpath[parentpath_len-1] = '\0';
> >
> > - if (strcmp(parentpath, uri) == 0)
> > - {
> > - err = get_parentpath_resource(r, resource);
> > - if (err)
> > - return err;
> > - return NULL;
> > - }
> > + err = get_parentpath_resource(r, resource);
> > + if (err)
> > + return err;
> > + return NULL;
> > }
> >
> > /* This does all the work of interpreting/splitting the request uri. */
> > @@ -3161,8 +3193,13 @@
> > {
> > apr_hash_index_t *hi;
> > apr_hash_t *dirents;
> > + int root_path_len = strlen(resource->info->repos->root_path);
> > const char *fs_parent_path =
> > - dav_svn__get_fs_parent_path(resource->info->r);
> > + apr_pstrcat(resource->info->r->pool,
> > + dav_svn__get_fs_parent_path(resource->info->r),
> > + resource->info->r->uri
> > + + ((root_path_len > 1) ? root_path_len : 0),
> > + NULL);
> >
> > serr = svn_io_get_dirents3(&dirents, fs_parent_path, TRUE,
> > resource->pool, resource->pool);
> > Index: subversion/libsvn_repos/repos.c
> > ===================================================================
> > --- subversion/libsvn_repos/repos.c (revision 1043620)
> > +++ subversion/libsvn_repos/repos.c (working copy)
> > @@ -1428,7 +1428,7 @@
> > * on errors (which would be permission errors, probably) so that
> > * we the user will see them after we try to open the repository
> > * for real. */
> > -static svn_boolean_t
> > +svn_boolean_t
> > check_repos_path(const char *path,
> > apr_pool_t *pool)
>
> As earlier reviewers told you, we do not define functions in one library
> and use them in another unless they have a svn_ svn_foo__ name prefix.
>
> > {
> >
>
>
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Posted by David Burley <bu...@geek.net>.
They got mangled on the way to my mail client -- I tested them again locally
before attaching them this time -- and they are now unmangled.
Thank you,
David
On Thu, Dec 9, 2010 at 2:38 AM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> David Burley wrote on Wed, Dec 08, 2010 at 19:37:51 -0500:
> > Patches attached again but can also be found here:
> >
> > http://subversion.tigris.org/issues/show_bug.cgi?id=3588
> >
> > On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf <d.s@daniel.shahaf.name
> >wrote:
> >
> > > David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> > > > So I have forward-ported Stephane's patch to add recursion to
> > > SVNParentPath
> > > > for 1.6.15 (see attached patch).
> > >
> > > The patch didn't get to the list. Please re-send it with *.txt
> extension.
> > >
> > > Thanks.
> > >
> > > Daniel
> > >
>
> The 1.7.x patch doesn't apply to HEAD of trunk.
>
> > Index: subversion/mod_dav_svn/repos.c
> > ===================================================================
> > --- subversion/mod_dav_svn/repos.c (revision 1043620)
> > +++ subversion/mod_dav_svn/repos.c (working copy)
> > @@ -1235,34 +1235,63 @@
> > {
> > /* SVNParentPath was used instead: assume the first component of
> > 'relative' is the name of a repository. */
> > - const char *magic_component, *magic_end;
> > + extern svn_boolean_t check_repos_path(const char *path, apr_pool_t
> *pool);
> > + const char *magic_component = NULL, *magic_end;
> > + char *repos_path;
> >
> > - /* A repository name is required here.
> > - Remember that 'relative' always starts with a "/". */
> > - if (relative[1] == '\0')
> > - {
> > - /* ### are SVN_ERR_APMOD codes within the right numeric space?
> */
> > - return dav_svn__new_error(r->pool, HTTP_FORBIDDEN,
> > - SVN_ERR_APMOD_MALFORMED_URI,
> > - "The URI does not contain the name "
> > - "of a repository.");
> > - }
> > + do
> > + {
> > + /* A repository name is required here.
> > + Remember that 'relative' always starts with a "/". */
> > + if (relative[1] == '\0')
> > + {
> > + /* ### are SVN_ERR_APMOD codes within the right numeric
> space? */
> > + return dav_new_error(r->pool, HTTP_FORBIDDEN,
> > + SVN_ERR_APMOD_MALFORMED_URI,
> > + "The URI does not contain the name "
> > + "of a repository.");
> > + }
> > +
> > + magic_end = ap_strchr_c(relative + 1, '/');
> > + if (!magic_end)
> > + {
> > + /* ### Request was for parent directory with no trailing
> > + slash; we probably ought to just redirect to same with
> > + trailing slash appended. */
> > + if (!magic_component)
> > + {
> > + magic_component = relative + 1;
> > + }
> > + else
> > + {
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + relative, NULL);
> > + }
> > + relative = "/";
> > + }
> > + else
> > + {
> > + if (!magic_component)
> > + {
> > + magic_component = apr_pstrndup(r->pool, relative + 1,
> > + magic_end - relative -
> 1);
> > + }
> > + else
> > + {
> > + char *tmp_magic_component;
> >
> > - magic_end = ap_strchr_c(relative + 1, '/');
> > - if (!magic_end)
> > - {
> > - /* ### Request was for parent directory with no trailing
> > - slash; we probably ought to just redirect to same with
> > - trailing slash appended. */
> > - magic_component = relative + 1;
> > - relative = "/";
> > - }
> > - else
> > - {
> > - magic_component = apr_pstrndup(r->pool, relative + 1,
> > - magic_end - relative - 1);
> > - relative = magic_end;
> > - }
> > + tmp_magic_component = apr_pstrndup(r->pool, relative,
> > + magic_end -
> relative);
> > + magic_component = apr_pstrcat(r->pool, magic_component,
> > + tmp_magic_component,
> NULL);
> > + }
> > + relative = magic_end;
> > + }
> > +
> > + repos_path = svn_path_join(fs_parent_path, magic_component,
> > + r->pool);
> > + }
> > + while (check_repos_path(repos_path, r->pool) != TRUE);
> >
> > /* return answer */
> > *repos_basename = magic_component;
> > @@ -1881,7 +1910,7 @@
> > dav_resource_combined *comb;
> > dav_svn_repos *repos;
> > const char *cleaned_uri;
> > - const char *repo_basename;
> > + const char *repo_basename = NULL;
> > const char *relative;
> > const char *repos_path;
> > const char *repos_key;
> > @@ -1897,9 +1926,15 @@
> > xslt_uri = dav_svn__get_xslt_uri(r);
> > fs_parent_path = dav_svn__get_fs_parent_path(r);
> >
> > + /* This does all the work of interpreting/splitting the request uri.
> */
> > + err = dav_svn_split_uri(r, r->uri, root_path,
> > + &cleaned_uri, &had_slash,
> > + &repo_basename, &relative, &repos_path);
> > +
> > +
> > /* Special case: detect and build the SVNParentPath as a unique type
> > of private resource, iff the SVNListParentPath directive is 'on'. */
> > - if (fs_parent_path && dav_svn__get_list_parentpath_flag(r))
> > + if (fs_parent_path && dav_svn__get_list_parentpath_flag(r) &&
> !repo_basename)
> > {
> > char *uri = apr_pstrdup(r->pool, r->uri);
> > char *parentpath = apr_pstrdup(r->pool, root_path);
> > @@ -1912,13 +1947,10 @@
> > if (parentpath[parentpath_len-1] == '/')
> > parentpath[parentpath_len-1] = '\0';
> >
> > - if (strcmp(parentpath, uri) == 0)
> > - {
> > - err = get_parentpath_resource(r, resource);
> > - if (err)
> > - return err;
> > - return NULL;
> > - }
> > + err = get_parentpath_resource(r, resource);
> > + if (err)
> > + return err;
> > + return NULL;
> > }
> >
> > /* This does all the work of interpreting/splitting the request uri. */
> > @@ -3161,8 +3193,13 @@
> > {
> > apr_hash_index_t *hi;
> > apr_hash_t *dirents;
> > + int root_path_len = strlen(resource->info->repos->root_path);
> > const char *fs_parent_path =
> > - dav_svn__get_fs_parent_path(resource->info->r);
> > + apr_pstrcat(resource->info->r->pool,
> > + dav_svn__get_fs_parent_path(resource->info->r),
> > + resource->info->r->uri
> > + + ((root_path_len > 1) ? root_path_len : 0),
> > + NULL);
> >
> > serr = svn_io_get_dirents3(&dirents, fs_parent_path, TRUE,
> > resource->pool, resource->pool);
> > Index: subversion/libsvn_repos/repos.c
> > ===================================================================
> > --- subversion/libsvn_repos/repos.c (revision 1043620)
> > +++ subversion/libsvn_repos/repos.c (working copy)
> > @@ -1428,7 +1428,7 @@
> > * on errors (which would be permission errors, probably) so that
> > * we the user will see them after we try to open the repository
> > * for real. */
> > -static svn_boolean_t
> > +svn_boolean_t
> > check_repos_path(const char *path,
> > apr_pool_t *pool)
>
> As earlier reviewers told you, we do not define functions in one library
> and use them in another unless they have a svn_ svn_foo__ name prefix.
>
> > {
> >
>
>
Re: [PATCH] Please review : repositories recursive finding with
SVNParentPath
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
David Burley wrote on Wed, Dec 08, 2010 at 19:37:51 -0500:
> Patches attached again but can also be found here:
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=3588
>
> On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
>
> > David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> > > So I have forward-ported Stephane's patch to add recursion to
> > SVNParentPath
> > > for 1.6.15 (see attached patch).
> >
> > The patch didn't get to the list. Please re-send it with *.txt extension.
> >
> > Thanks.
> >
> > Daniel
> >
The 1.7.x patch doesn't apply to HEAD of trunk.
> Index: subversion/mod_dav_svn/repos.c
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 1043620)
> +++ subversion/mod_dav_svn/repos.c (working copy)
> @@ -1235,34 +1235,63 @@
> {
> /* SVNParentPath was used instead: assume the first component of
> 'relative' is the name of a repository. */
> - const char *magic_component, *magic_end;
> + extern svn_boolean_t check_repos_path(const char *path, apr_pool_t *pool);
> + const char *magic_component = NULL, *magic_end;
> + char *repos_path;
>
> - /* A repository name is required here.
> - Remember that 'relative' always starts with a "/". */
> - if (relative[1] == '\0')
> - {
> - /* ### are SVN_ERR_APMOD codes within the right numeric space? */
> - return dav_svn__new_error(r->pool, HTTP_FORBIDDEN,
> - SVN_ERR_APMOD_MALFORMED_URI,
> - "The URI does not contain the name "
> - "of a repository.");
> - }
> + do
> + {
> + /* A repository name is required here.
> + Remember that 'relative' always starts with a "/". */
> + if (relative[1] == '\0')
> + {
> + /* ### are SVN_ERR_APMOD codes within the right numeric space? */
> + return dav_new_error(r->pool, HTTP_FORBIDDEN,
> + SVN_ERR_APMOD_MALFORMED_URI,
> + "The URI does not contain the name "
> + "of a repository.");
> + }
> +
> + magic_end = ap_strchr_c(relative + 1, '/');
> + if (!magic_end)
> + {
> + /* ### Request was for parent directory with no trailing
> + slash; we probably ought to just redirect to same with
> + trailing slash appended. */
> + if (!magic_component)
> + {
> + magic_component = relative + 1;
> + }
> + else
> + {
> + magic_component = apr_pstrcat(r->pool, magic_component,
> + relative, NULL);
> + }
> + relative = "/";
> + }
> + else
> + {
> + if (!magic_component)
> + {
> + magic_component = apr_pstrndup(r->pool, relative + 1,
> + magic_end - relative - 1);
> + }
> + else
> + {
> + char *tmp_magic_component;
>
> - magic_end = ap_strchr_c(relative + 1, '/');
> - if (!magic_end)
> - {
> - /* ### Request was for parent directory with no trailing
> - slash; we probably ought to just redirect to same with
> - trailing slash appended. */
> - magic_component = relative + 1;
> - relative = "/";
> - }
> - else
> - {
> - magic_component = apr_pstrndup(r->pool, relative + 1,
> - magic_end - relative - 1);
> - relative = magic_end;
> - }
> + tmp_magic_component = apr_pstrndup(r->pool, relative,
> + magic_end - relative);
> + magic_component = apr_pstrcat(r->pool, magic_component,
> + tmp_magic_component, NULL);
> + }
> + relative = magic_end;
> + }
> +
> + repos_path = svn_path_join(fs_parent_path, magic_component,
> + r->pool);
> + }
> + while (check_repos_path(repos_path, r->pool) != TRUE);
>
> /* return answer */
> *repos_basename = magic_component;
> @@ -1881,7 +1910,7 @@
> dav_resource_combined *comb;
> dav_svn_repos *repos;
> const char *cleaned_uri;
> - const char *repo_basename;
> + const char *repo_basename = NULL;
> const char *relative;
> const char *repos_path;
> const char *repos_key;
> @@ -1897,9 +1926,15 @@
> xslt_uri = dav_svn__get_xslt_uri(r);
> fs_parent_path = dav_svn__get_fs_parent_path(r);
>
> + /* This does all the work of interpreting/splitting the request uri. */
> + err = dav_svn_split_uri(r, r->uri, root_path,
> + &cleaned_uri, &had_slash,
> + &repo_basename, &relative, &repos_path);
> +
> +
> /* Special case: detect and build the SVNParentPath as a unique type
> of private resource, iff the SVNListParentPath directive is 'on'. */
> - if (fs_parent_path && dav_svn__get_list_parentpath_flag(r))
> + if (fs_parent_path && dav_svn__get_list_parentpath_flag(r) && !repo_basename)
> {
> char *uri = apr_pstrdup(r->pool, r->uri);
> char *parentpath = apr_pstrdup(r->pool, root_path);
> @@ -1912,13 +1947,10 @@
> if (parentpath[parentpath_len-1] == '/')
> parentpath[parentpath_len-1] = '\0';
>
> - if (strcmp(parentpath, uri) == 0)
> - {
> - err = get_parentpath_resource(r, resource);
> - if (err)
> - return err;
> - return NULL;
> - }
> + err = get_parentpath_resource(r, resource);
> + if (err)
> + return err;
> + return NULL;
> }
>
> /* This does all the work of interpreting/splitting the request uri. */
> @@ -3161,8 +3193,13 @@
> {
> apr_hash_index_t *hi;
> apr_hash_t *dirents;
> + int root_path_len = strlen(resource->info->repos->root_path);
> const char *fs_parent_path =
> - dav_svn__get_fs_parent_path(resource->info->r);
> + apr_pstrcat(resource->info->r->pool,
> + dav_svn__get_fs_parent_path(resource->info->r),
> + resource->info->r->uri
> + + ((root_path_len > 1) ? root_path_len : 0),
> + NULL);
>
> serr = svn_io_get_dirents3(&dirents, fs_parent_path, TRUE,
> resource->pool, resource->pool);
> Index: subversion/libsvn_repos/repos.c
> ===================================================================
> --- subversion/libsvn_repos/repos.c (revision 1043620)
> +++ subversion/libsvn_repos/repos.c (working copy)
> @@ -1428,7 +1428,7 @@
> * on errors (which would be permission errors, probably) so that
> * we the user will see them after we try to open the repository
> * for real. */
> -static svn_boolean_t
> +svn_boolean_t
> check_repos_path(const char *path,
> apr_pool_t *pool)
As earlier reviewers told you, we do not define functions in one library
and use them in another unless they have a svn_ svn_foo__ name prefix.
> {
>
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Posted by David Burley <bu...@geek.net>.
Patches attached again but can also be found here:
http://subversion.tigris.org/issues/show_bug.cgi?id=3588
On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> > So I have forward-ported Stephane's patch to add recursion to
> SVNParentPath
> > for 1.6.15 (see attached patch).
>
> The patch didn't get to the list. Please re-send it with *.txt extension.
>
> Thanks.
>
> Daniel
>
Re: [PATCH] Please review : repositories recursive finding with SVNParentPath
Posted by David Burley <bu...@geek.net>.
Patches attached again but can also be found here:
http://subversion.tigris.org/issues/show_bug.cgi?id=3588
On Wed, Dec 8, 2010 at 5:24 PM, Daniel Shahaf <d....@daniel.shahaf.name>wrote:
> David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> > So I have forward-ported Stephane's patch to add recursion to
> SVNParentPath
> > for 1.6.15 (see attached patch).
>
> The patch didn't get to the list. Please re-send it with *.txt extension.
>
> Thanks.
>
> Daniel
>
Re: [PATCH] Please review : repositories recursive finding with
SVNParentPath
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
David Burley wrote on Wed, Dec 08, 2010 at 17:03:06 -0500:
> So I have forward-ported Stephane's patch to add recursion to SVNParentPath
> for 1.6.15 (see attached patch).
The patch didn't get to the list. Please re-send it with *.txt extension.
Thanks.
Daniel