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/06/23 19:39:43 UTC
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/
...
cmpilato@apache.org wrote on Wed, 23 Jun 2010 at 04:22 -0000:
> Author: cmpilato
> Date: Wed Jun 23 01:22:00 2010
> New Revision: 957094
>
> URL: http://svn.apache.org/viewvc?rev=957094&view=rev
> Log:
> Finish issue #3661: RA get-locks inefficiencies.
>
> * build.conf
> (svnserve): Add dependency on libsvn_ra.
>
> * subversion/svnserve/serve.c
> (get_locks): Look for optional depth in the get-locks request, and
> use it in what is now a call to svn_repos_fs_get_locks2().
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_get_locks): Add 'depth' parameter, and pass it to the server
> in case the server can make use of it. Also, filter out unwanted
> locks (returned by servers that *can't* make use of it).
>
> Modified: subversion/trunk/build.conf
> URL: http://svn.apache.org/viewvc/subversion/trunk/build.conf?rev=957094&r1=957093&r2=957094&view=diff
> ==============================================================================
> --- subversion/trunk/build.conf (original)
> +++ subversion/trunk/build.conf Wed Jun 23 01:22:00 2010
> @@ -147,7 +147,7 @@ type = exe
> path = subversion/svnserve
> install = bin
> manpages = subversion/svnserve/svnserve.8 subversion/svnserve/svnserve.conf.5
> -libs = libsvn_repos libsvn_fs libsvn_delta libsvn_subr libsvn_ra_svn
> +libs = libsvn_repos libsvn_fs libsvn_delta libsvn_subr libsvn_ra libsvn_ra_svn
> apriconv apr sasl
> msvc-libs = advapi32.lib ws2_32.lib
>
>
> Modified: subversion/trunk/subversion/libsvn_ra_svn/client.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/client.c?rev=957094&r1=957093&r2=957094&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_ra_svn/client.c (original)
> +++ subversion/trunk/subversion/libsvn_ra_svn/client.c Wed Jun 23 01:22:00 2010
> @@ -2210,14 +2210,23 @@ static svn_error_t *ra_svn_get_lock(svn_
> static svn_error_t *ra_svn_get_locks(svn_ra_session_t *session,
> apr_hash_t **locks,
> const char *path,
> + svn_depth_t depth,
> apr_pool_t *pool)
> {
> svn_ra_svn__session_baton_t *sess = session->priv;
> svn_ra_svn_conn_t* conn = sess->conn;
> apr_array_header_t *list;
> + const char *abs_path;
> int i;
>
> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-locks", "c", path));
> + /* Figure out the repository abspath from PATH. */
> + abs_path = svn_path_url_add_component2(sess->url, path, pool);
> + SVN_ERR(svn_ra_get_path_relative_to_root(session, &abs_path,
> + abs_path, pool));
I think this change means that, in build.conf, libsvn_ra should have
been added as a dependency to [libsvn_ra_svn]. (This patch added it only
to [svnserve].)
Unless objections, I'll make this change (while also committing the
ra_svn protocol bits noted on IRC and in the issue).
> + abs_path = apr_pstrcat(pool, "/", abs_path, NULL);
> +
> + SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-locks", "c?w", path,
> + svn_depth_to_word(depth)));
>
> /* Servers before 1.2 doesn't support locking. Check this here. */
> SVN_ERR(handle_unsupported_cmd(handle_auth_request(sess, pool),
> @@ -2237,7 +2246,27 @@ static svn_error_t *ra_svn_get_locks(svn
> return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> _("Lock element not a list"));
> SVN_ERR(parse_lock(elt->u.list, pool, &lock));
> - apr_hash_set(*locks, lock->path, APR_HASH_KEY_STRING, lock);
> +
> + /* Filter out unwanted paths. Since Subversion only allows
> + locks on files, we can treat depth=immediates the same as
> + depth=files for filtering purposes. Meaning, we'll keep
> + this lock if:
> +
> + a) its path is the very path we queried, or
> + b) we've asked for a fully recursive answer, or
> + c) we've asked for depth=files or depth=immediates, and this
> + lock is on an immediate child of our query path.
> + */
> + if ((strcmp(abs_path, lock->path) == 0) || (depth == svn_depth_infinity))
> + {
> + apr_hash_set(*locks, lock->path, APR_HASH_KEY_STRING, lock);
> + }
> + else if ((depth == svn_depth_files) || (depth == svn_depth_immediates))
> + {
> + const char *rel_uri = svn_uri_is_child(abs_path, lock->path, pool);
> + if (rel_uri && (svn_path_component_count(rel_uri) == 1))
> + apr_hash_set(*locks, lock->path, APR_HASH_KEY_STRING, lock);
> + }
> }
>
> return SVN_NO_ERROR;
>
> Modified: subversion/trunk/subversion/svnserve/serve.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svnserve/serve.c?rev=957094&r1=957093&r2=957094&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/svnserve/serve.c (original)
> +++ subversion/trunk/subversion/svnserve/serve.c Wed Jun 23 01:22:00 2010
> @@ -2572,21 +2572,23 @@ static svn_error_t *get_locks(svn_ra_svn
> server_baton_t *b = baton;
> const char *path;
> const char *full_path;
> + const char *depth_word;
> + svn_depth_t depth;
> apr_hash_t *locks;
> apr_hash_index_t *hi;
>
> - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c", &path));
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "c?w", &path, &depth_word));
>
> - full_path = svn_uri_join(b->fs_path->data, svn_uri_canonicalize(path,
> - pool),
> - pool);
> + depth = depth_word ? svn_depth_from_word(depth_word) : svn_depth_infinity;
> + full_path = svn_uri_join(b->fs_path->data,
> + svn_uri_canonicalize(path, pool), pool);
>
> SVN_ERR(trivial_auth_request(conn, pool, b));
>
> SVN_ERR(log_command(b, conn, pool, "get-locks %s",
> svn_path_uri_encode(full_path, pool)));
> - SVN_CMD_ERR(svn_repos_fs_get_locks(&locks, b->repos, full_path,
> - authz_check_access_cb_func(b), b, pool));
> + SVN_CMD_ERR(svn_repos_fs_get_locks2(&locks, b->repos, full_path, depth,
> + authz_check_access_cb_func(b), b, pool));
>
> SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w((!", "success"));
> for (hi = apr_hash_first(pool, locks); hi; hi = apr_hash_next(hi))
>
>
>
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/
...
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Fri, 25 Jun 2010 at 23:14 -0000:
> C. Michael Pilato wrote:
> > Arfrever Frehtes Taifersar Arahesis wrote:
> >> I suggest to create libsvn_ra_util library (similar to libsvn_fs_util).
> >> The code of svn_ra_get_path_relative_to_root() would be moved to
> >> svn_ra__get_path_relative_to_root(), which would be defined in libsvn_ra_util.
> >> libsvn_ra and libsvn_ra_svn would be linked against libsvn_ra_util.
> >> libsvn_ra_svn would use svn_ra__get_path_relative_to_root().
> >> svn_ra_get_path_relative_to_root() would be defined in libsvn_ra and would
> >> wrap svn_ra__get_path_relative_to_root().
> >
> > But svn_ra_get_path_relative_to_root() and
> > svn_ra_get_path_relative_to_session() call in the RA providers. So wouldn't
> > that make them dependent on libsvn_ra (another dependency cycle)?
>
> Is there any way to make this work without just implementing these two
> functions in each of the RA provider libraries, and then only calling their
> library-internal implementations from inside a given provider?
>
We could duplicate the prototype of svn_ra__vtable_t.get_session_url and
svn_ra__vtable_t.get_repos_root, perhaps?
Anyway, I see that's now fixed in trunk by r958113.
Thanks!
Daniel
>
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/ ...
Posted by "C. Michael Pilato" <cm...@collab.net>.
C. Michael Pilato wrote:
> Arfrever Frehtes Taifersar Arahesis wrote:
>> I suggest to create libsvn_ra_util library (similar to libsvn_fs_util).
>> The code of svn_ra_get_path_relative_to_root() would be moved to
>> svn_ra__get_path_relative_to_root(), which would be defined in libsvn_ra_util.
>> libsvn_ra and libsvn_ra_svn would be linked against libsvn_ra_util.
>> libsvn_ra_svn would use svn_ra__get_path_relative_to_root().
>> svn_ra_get_path_relative_to_root() would be defined in libsvn_ra and would
>> wrap svn_ra__get_path_relative_to_root().
>
> But svn_ra_get_path_relative_to_root() and
> svn_ra_get_path_relative_to_session() call in the RA providers. So wouldn't
> that make them dependent on libsvn_ra (another dependency cycle)?
Is there any way to make this work without just implementing these two
functions in each of the RA provider libraries, and then only calling their
library-internal implementations from inside a given provider?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/ ...
Posted by "C. Michael Pilato" <cm...@collab.net>.
Arfrever Frehtes Taifersar Arahesis wrote:
> I suggest to create libsvn_ra_util library (similar to libsvn_fs_util).
> The code of svn_ra_get_path_relative_to_root() would be moved to
> svn_ra__get_path_relative_to_root(), which would be defined in libsvn_ra_util.
> libsvn_ra and libsvn_ra_svn would be linked against libsvn_ra_util.
> libsvn_ra_svn would use svn_ra__get_path_relative_to_root().
> svn_ra_get_path_relative_to_root() would be defined in libsvn_ra and would
> wrap svn_ra__get_path_relative_to_root().
But svn_ra_get_path_relative_to_root() and
svn_ra_get_path_relative_to_session() call in the RA providers. So wouldn't
that make them dependent on libsvn_ra (another dependency cycle)?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/ subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/ ...
Posted by Arfrever Frehtes Taifersar Arahesis <ar...@gmail.com>.
2010-06-24 18:35:21 Daniel Shahaf napisał(a):
> C. Michael Pilato wrote on Wed, 23 Jun 2010 at 22:43 -0000:
> > Daniel Shahaf wrote:
> > >> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-locks", "c", path));
> > >> + /* Figure out the repository abspath from PATH. */
> > >> + abs_path = svn_path_url_add_component2(sess->url, path, pool);
> > >> + SVN_ERR(svn_ra_get_path_relative_to_root(session, &abs_path,
> > >> + abs_path, pool));
> > >
> > > I think this change means that, in build.conf, libsvn_ra should have
> > > been added as a dependency to [libsvn_ra_svn]. (This patch added it only
> > > to [svnserve].)
> > >
> > > Unless objections, I'll make this change (while also committing the
> > > ra_svn protocol bits noted on IRC and in the issue).
> >
> > Fine with me. (The change I made was sufficient to fix the problem I was
> > seeing in my build.)
> >
>
> Looking further, the patch added svn_ra_get_path_relative_to_root() to
> all network-based RA layers. However, when I try to add libsvn_ra in
> build.conf as suggested above, I just get errors from configure/make
> about circular dependencies :-(
>
> I'm not sure what's going on here. But if it breaks in the future,
> hopefully this thread is going to be useful...
This revision breaks building with --enable-disallowing-of-undefined-references
option passed to `configure`.
cd subversion/libsvn_ra_svn && /usr/bin/libtool --tag=CC --silent --mode=link x86_64-pc-linux-gnu-gcc -march=core2 -pipe -O2 -pthread -Werror=implicit-function-declaration -Wl,-O1,--as-needed,--gc-sections,--hash-style=gnu,--sort-common -L/usr/lib64/qt4 -rpath /usr/lib64 -Wl,--no-undefined -o libsvn_ra_svn-1.la client.lo cram.lo cyrus_auth.lo editorp.lo internal_auth.lo marshal.lo streams.lo version.lo ../../subversion/libsvn_delta/libsvn_delta-1.la ../../subversion/libsvn_subr/libsvn_subr-1.la -laprutil-1 -lapr-1 -lsasl2
.libs/client.o: In function `ra_svn_get_locks':
client.c:(.text+0x7ee): undefined reference to `svn_ra_get_path_relative_to_root'
collect2: ld returned 1 exit status
make: *** [subversion/libsvn_ra_svn/libsvn_ra_svn-1.la] Error 1
I suggest to create libsvn_ra_util library (similar to libsvn_fs_util).
The code of svn_ra_get_path_relative_to_root() would be moved to
svn_ra__get_path_relative_to_root(), which would be defined in libsvn_ra_util.
libsvn_ra and libsvn_ra_svn would be linked against libsvn_ra_util.
libsvn_ra_svn would use svn_ra__get_path_relative_to_root().
svn_ra_get_path_relative_to_root() would be defined in libsvn_ra and would
wrap svn_ra__get_path_relative_to_root().
--
Arfrever Frehtes Taifersar Arahesis
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/
...
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Wed, 23 Jun 2010 at 22:43 -0000:
> Daniel Shahaf wrote:
> >> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-locks", "c", path));
> >> + /* Figure out the repository abspath from PATH. */
> >> + abs_path = svn_path_url_add_component2(sess->url, path, pool);
> >> + SVN_ERR(svn_ra_get_path_relative_to_root(session, &abs_path,
> >> + abs_path, pool));
> >
> > I think this change means that, in build.conf, libsvn_ra should have
> > been added as a dependency to [libsvn_ra_svn]. (This patch added it only
> > to [svnserve].)
> >
> > Unless objections, I'll make this change (while also committing the
> > ra_svn protocol bits noted on IRC and in the issue).
>
> Fine with me. (The change I made was sufficient to fix the problem I was
> seeing in my build.)
>
Looking further, the patch added svn_ra_get_path_relative_to_root() to
all network-based RA layers. However, when I try to add libsvn_ra in
build.conf as suggested above, I just get errors from configure/make
about circular dependencies :-(
I'm not sure what's going on here. But if it breaks in the future,
hopefully this thread is going to be useful...
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/
...
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
C. Michael Pilato wrote on Wed, 23 Jun 2010 at 22:43 -0000:
> Daniel Shahaf wrote:
> >> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-locks", "c", path));
> >> + /* Figure out the repository abspath from PATH. */
> >> + abs_path = svn_path_url_add_component2(sess->url, path, pool);
> >> + SVN_ERR(svn_ra_get_path_relative_to_root(session, &abs_path,
> >> + abs_path, pool));
> >
> > I think this change means that, in build.conf, libsvn_ra should have
> > been added as a dependency to [libsvn_ra_svn]. (This patch added it only
> > to [svnserve].)
> >
> > Unless objections, I'll make this change (while also committing the
> > ra_svn protocol bits noted on IRC and in the issue).
>
> Fine with me. (The change I made was sufficient to fix the problem I was
> seeing in my build.)
>
Looking further, the patch added svn_ra_get_path_relative_to_root() to
all network-based RA layers. However, when I try to add libsvn_ra in
build.conf as suggested above, I just get errors from configure/make
about circular dependencies :-(
I'm not sure what's going on here. But if it breaks in the future,
hopefully this thread is going to be useful...
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/ ...
Posted by "C. Michael Pilato" <cm...@collab.net>.
Daniel Shahaf wrote:
>> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-locks", "c", path));
>> + /* Figure out the repository abspath from PATH. */
>> + abs_path = svn_path_url_add_component2(sess->url, path, pool);
>> + SVN_ERR(svn_ra_get_path_relative_to_root(session, &abs_path,
>> + abs_path, pool));
>
> I think this change means that, in build.conf, libsvn_ra should have
> been added as a dependency to [libsvn_ra_svn]. (This patch added it only
> to [svnserve].)
>
> Unless objections, I'll make this change (while also committing the
> ra_svn protocol bits noted on IRC and in the issue).
Fine with me. (The change I made was sufficient to fix the problem I was
seeing in my build.)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r957094 - in /subversion/trunk: ./ subversion/include/
subversion/libsvn_client/ subversion/libsvn_fs*/ subversion/libsvn_ra*/ ...
Posted by "C. Michael Pilato" <cm...@collab.net>.
Daniel Shahaf wrote:
>> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "get-locks", "c", path));
>> + /* Figure out the repository abspath from PATH. */
>> + abs_path = svn_path_url_add_component2(sess->url, path, pool);
>> + SVN_ERR(svn_ra_get_path_relative_to_root(session, &abs_path,
>> + abs_path, pool));
>
> I think this change means that, in build.conf, libsvn_ra should have
> been added as a dependency to [libsvn_ra_svn]. (This patch added it only
> to [svnserve].)
>
> Unless objections, I'll make this change (while also committing the
> ra_svn protocol bits noted on IRC and in the issue).
Fine with me. (The change I made was sufficient to fix the problem I was
seeing in my build.)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand