You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2006/06/10 20:08:31 UTC
Re: svn commit: r20033 - in branches/merge-tracking/subversion: include libsvn_repos svnserve
dlr@tigris.org writes:
> Author: dlr
> Date: Fri Jun 9 15:59:21 2006
> New Revision: 20033
>
> Modified:
> branches/merge-tracking/subversion/include/svn_repos.h
> branches/merge-tracking/subversion/libsvn_repos/fs-wrap.c
> branches/merge-tracking/subversion/svnserve/serve.c
>
> Log:
> On the merge-tracking branch: Punch a merge info retrieval operation
> through svnserve, via the repos, down to the FS.
>
> * subversion/include/svn_repos.h
> (svn_repos_fs_get_merge_info): Add function decl to fetch merge
> information for a set of paths.
>
> * subversion/libsvn_repos/fs-wrap.c
> (svn_repos_fs_get_merge_info): Implement function. While unreadable
> paths for which merge info are requested are filtered out,
> unreadable paths are NOT filtered out of the returned merge info.
>
> * subversion/svnserve/serve.c
> (get_merge_info): Add a new function to get the merge info for a
> list of paths at a specific revision.
>
> (main_commands): Add get_merge_info to the list as "get-merge-info".
>
>
It is much easier to review changes to svnserve if you update the protocol
document in parallell with updating the code.
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r20033 - in branches/merge-tracking/subversion:
include libsvn_repos svnserve
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2006-06-12 at 15:11 -0700, Daniel Rall wrote:
> That makes sense. The 'change-rev-prop' and 'log' commands don't
> conform to this pattern, as their trailing optional elements aren't
> enclosed in a tuple. Do they predate this pattern?
Ah, sorry, I see where the confusion comes from. Both of those commands
use a different pattern as the result of protocol changes. The
change-rev-prop value argument was at one time mandatory, while the log
limit argument is only "optional" in the sense that pre-1.3 clients
don't know to send it, not that we would ever choose not to send it.
get-dir's last argument is similar, as is diff's last argument.
> Lastly, some optional elements are enclosed in optional tuples (that
> is, their enclosing tuple can end before the optional nested tuple
> ever begins). Is this a pattern we want to continue?
Basically, in the protocol document, "? field" means "new field", while
"[ field ]" means "optional field". In the case of status, there was a
new optional field: not only would an old client not send the field at
all, but a new client might choose not to send it because there isn't
always a revision to specify.
From the server's perspective, "the client was too old to send it" isn't
usually different from "the client chose not to specify it". But
extending status with "?(?r)" means that another new field might be
added in the future, making the end of the format specifier look like
"?(?r)?b" or something.
For a new command, the protocol document should use only brackets, not
question marks.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r20033 - in branches/merge-tracking/subversion: include libsvn_repos svnserve
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 12 Jun 2006, Greg Hudson wrote:
> On Mon, 2006-06-12 at 12:13 -0700, Daniel Rall wrote:
...
> > Also, "rev" should be an optional parameter (the server defaults to
> > "youngest" -- the protocol doc might want to indicate that, too).
> > Does "rev" need to be placed in a tuple, or can it simply be preceded
> > by a question mark?
>
> With all due respect, there's nothing unique about this request; you
> can just look at how other commands handle similar constructs.
I did, but as this is my first foray into any serious svnserve/ra_svn
modifications, I appreciate the confirmation.
> ... On the second point, use the same optional-tuple syntax used by
> every other command, which puts the argument inside a tuple. That
> way the command can be extended to include more optional arguments.
That makes sense. The 'change-rev-prop' and 'log' commands don't
conform to this pattern, as their trailing optional elements aren't
enclosed in a tuple. Do they predate this pattern?
The 'get-dir' command has a trailing optional "list" type elment which
is not enclosed in a tuple. Is the tuple implicit for the "list" type
case?
Lastly, some optional elements are enclosed in optional tuples (that
is, their enclosing tuple can end before the optional nested tuple
ever begins). Is this a pattern we want to continue?
--
Daniel Rall
Re: svn commit: r20033 - in branches/merge-tracking/subversion:
include libsvn_repos svnserve
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2006-06-12 at 12:13 -0700, Daniel Rall wrote:
> The first nested tuple should be a list. I think I need to add an
> ellipses to indicate that.
>
> Also, "rev" should be an optional parameter (the server defaults to
> "youngest" -- the protocol doc might want to indicate that, too).
> Does "rev" need to be placed in a tuple, or can it simply be preceded
> by a question mark?
With all due respect, there's nothing unique about this request; you can
just look at how other commands handle similar constructs.
You're correct on the first point. On the second point, use the same
optional-tuple syntax used by every other command, which puts the
argument inside a tuple. That way the command can be extended to
include more optional arguments.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r20033 - in branches/merge-tracking/subversion: include libsvn_repos svnserve
Posted by Daniel Rall <dl...@collab.net>.
Peter Lundblad requested that the changes to the protocol document be
submitted together with the code. This time, because I wanted to get
those changes in front of DannyB *before* I finished writing the code
(and am on a branch), I committed them earlier (in r20025). For
reference, here's that addition to the protocol document:
get-merge-info
params: ( ( path:string ) rev:number )
response: ( merge-info:string )
New in svn 1.5. If no paths are specified, an empty response is returned.
The first nested tuple should be a list. I think I need to add an
ellipses to indicate that.
Also, "rev" should be an optional parameter (the server defaults to
"youngest" -- the protocol doc might want to indicate that, too).
Does "rev" need to be placed in a tuple, or can it simply be preceded
by a question mark?
--- protocol (revision 20049)
+++ protocol (working copy)
@@ -279,7 +279,7 @@
New in svn 1.2. If path is non-existent, an empty response is returned.
get-merge-info
- params: ( ( path:string ) rev:number )
+ params: ( ( path:string ... ) ? rev:number )
response: ( merge-info:string )
New in svn 1.5. If no paths are specified, an empty response is returned.
Thoughts?
On Fri, 09 Jun 2006, dlr@tigris.org wrote:
> Author: dlr
> Date: Fri Jun 9 15:59:21 2006
> New Revision: 20033
>
> Modified:
> branches/merge-tracking/subversion/include/svn_repos.h
> branches/merge-tracking/subversion/libsvn_repos/fs-wrap.c
> branches/merge-tracking/subversion/svnserve/serve.c
>
> Log:
> On the merge-tracking branch: Punch a merge info retrieval operation
> through svnserve, via the repos, down to the FS.
>
> * subversion/include/svn_repos.h
> (svn_repos_fs_get_merge_info): Add function decl to fetch merge
> information for a set of paths.
>
> * subversion/libsvn_repos/fs-wrap.c
> (svn_repos_fs_get_merge_info): Implement function. While unreadable
> paths for which merge info are requested are filtered out,
> unreadable paths are NOT filtered out of the returned merge info.
>
> * subversion/svnserve/serve.c
> (get_merge_info): Add a new function to get the merge info for a
> list of paths at a specific revision.
>
> (main_commands): Add get_merge_info to the list as "get-merge-info".
>
>
> Modified: branches/merge-tracking/subversion/include/svn_repos.h
> URL: http://svn.collab.net/viewvc/svn/branches/merge-tracking/subversion/include/svn_repos.h?pathrev=20033&r1=20032&r2=20033
> ==============================================================================
> --- branches/merge-tracking/subversion/include/svn_repos.h (original)
> +++ branches/merge-tracking/subversion/include/svn_repos.h Fri Jun 9 15:59:21 2006
> @@ -1032,6 +1032,36 @@
>
> /* ---------------------------------------------------------------*/
>
> +/* Retrieving merge info. */
> +
> +/**
> + * Fetch the merge info for @a paths at @rev, and save it to @a
> + * mergeinfo (a mapping of char * paths to apr_array_header_t *'s of
> + * svn_merge_range_t * elements).
> + *
> + * If @a rev is @c SVN_INVALID_REVNUM, it defaults to youngest.
> + *
> + * If optional @a authz_read_func is non-NULL, then use this function
> + * (along with optional @a authz_read_baton) to check the readability
> + * of each path which merge info was requested for (from @a paths).
> + * Silently omit unreadable paths from the request for merge info.
> + *
> + * Use @a pool for temporary allocations.
> + *
> + * @since New in 1.5.
> + */
> +svn_error_t *
> +svn_repos_fs_get_merge_info(apr_hash_t **mergeinfo,
> + svn_repos_t *repos,
> + const apr_array_header_t *paths,
> + svn_revnum_t rev,
> + svn_repos_authz_func_t authz_read_func,
> + void *authz_read_baton,
> + apr_pool_t *pool);
> +
> +
> +/* ---------------------------------------------------------------*/
> +
> /* Retreiving multiple revisions of a file. */
>
> /**
>
> Modified: branches/merge-tracking/subversion/libsvn_repos/fs-wrap.c
> URL: http://svn.collab.net/viewvc/svn/branches/merge-tracking/subversion/libsvn_repos/fs-wrap.c?pathrev=20033&r1=20032&r2=20033
> ==============================================================================
> --- branches/merge-tracking/subversion/libsvn_repos/fs-wrap.c (original)
> +++ branches/merge-tracking/subversion/libsvn_repos/fs-wrap.c Fri Jun 9 15:59:21 2006
> @@ -570,6 +570,50 @@
> }
>
>
> +svn_error_t *
> +svn_repos_fs_get_merge_info(apr_hash_t **mergeinfo,
> + svn_repos_t *repos,
> + const apr_array_header_t *paths,
> + svn_revnum_t rev,
> + svn_repos_authz_func_t authz_read_func,
> + void *authz_read_baton,
> + apr_pool_t *pool)
> +{
> + apr_pool_t *subpool;
> + apr_array_header_t *readable_paths;
> + svn_fs_root_t *root;
> + int i;
> +
> + if (!SVN_IS_VALID_REVNUM(rev))
> + SVN_ERR(svn_fs_youngest_rev(&rev, repos->fs, pool));
> + SVN_ERR(svn_fs_revision_root(&root, repos->fs, rev, pool));
> + readable_paths = apr_array_make(pool, paths->nelts, sizeof(*readable_paths));
> + subpool = svn_pool_create(pool);
> +
> + /* Filter out unreadable paths before divining merge tracking info. */
> + for (i = 0; i < paths->nelts; i++)
> + {
> + svn_boolean_t readable;
> + const char *path = APR_ARRAY_IDX(paths, i, char *);
> + svn_pool_clear(subpool);
> + SVN_ERR(authz_read_func(&readable, root, path, authz_read_baton,
> + subpool));
> + if (readable)
> + APR_ARRAY_PUSH(readable_paths, const char *) = path;
> + }
> +
> + /* We consciously do not perform authz checks on the paths returned
> + in *MERGEINFO, avoiding massive authz overhead which would allow
> + us to protect the name of where a change was merged from, but not
> + the change itself. */
> + if (readable_paths->nelts > 0)
> + SVN_ERR(svn_fs_get_merge_info(root, readable_paths, rev, mergeinfo, pool));
> + else
> + *mergeinfo = NULL;
> +
> + apr_pool_destroy(subpool);
> + return SVN_NO_ERROR;
> +}
>
>
>
>
> Modified: branches/merge-tracking/subversion/svnserve/serve.c
> URL: http://svn.collab.net/viewvc/svn/branches/merge-tracking/subversion/svnserve/serve.c?pathrev=20033&r1=20032&r2=20033
> ==============================================================================
> --- branches/merge-tracking/subversion/svnserve/serve.c (original)
> +++ branches/merge-tracking/subversion/svnserve/serve.c Fri Jun 9 15:59:21 2006
> @@ -38,6 +38,7 @@
> #include "svn_md5.h"
> #include "svn_config.h"
> #include "svn_props.h"
> +#include "svn_mergeinfo.h"
> #include "svn_user.h"
>
> #include "server.h"
> @@ -1334,6 +1335,35 @@
> text_deltas, recurse, ignore_ancestry);
> }
>
> +/* ASSUMPTION: When performing a 'merge' with two URLs, the client
> + will call this command more than once. */
> +static svn_error_t *get_merge_info(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> + apr_array_header_t *params, void *baton)
> +{
> + server_baton_t *b = baton;
> + svn_revnum_t rev = SVN_INVALID_REVNUM;
> + apr_array_header_t *paths;
> + apr_hash_t *mergeinfo;
> + int i;
> + svn_stringbuf_t *value;
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "l?r", &paths, &rev));
> + for (i = 0; i < paths->nelts; i++)
> + APR_ARRAY_IDX(paths, i, const char *) =
> + svn_path_canonicalize(APR_ARRAY_IDX(paths, i, const char *), pool);
> + SVN_ERR(trivial_auth_request(conn, pool, b));
> + SVN_CMD_ERR(svn_repos_fs_get_merge_info(&mergeinfo, b->repos, paths, rev,
> + authz_check_access_cb_func(b), b,
> + pool));
> + if (mergeinfo != NULL)
> + SVN_ERR(svn_mergeinfo_to_string(&value, mergeinfo, pool));
> + else
> + value = NULL;
> + SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, "c",
> + value ? value->data : ""));
> + return SVN_NO_ERROR;
> +}
> +
> /* Send a log entry to the client. */
> static svn_error_t *log_receiver(void *baton, apr_hash_t *changed_paths,
> svn_revnum_t rev, const char *author,
> @@ -2007,6 +2037,7 @@
> { "switch", switch_cmd },
> { "status", status },
> { "diff", diff },
> + { "get-merge-info", get_merge_info },
> { "log", log_cmd },
> { "check-path", check_path },
> { "stat", stat },