You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/12/08 22:24:28 UTC
Re: [PATCH] Please review : repositories recursive finding with
SVNParentPath
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 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
>