Troy,
I've taken your patch (which I like better already) and started to modify it a
bit more.
Apart from some simple changes and reformatting, there is one significant
thing: the layering of command-line syntax knowledge. I've had a long hard
think about this, and come to the conclusion that it's the historical accident
of the "svn_opt" argument-processing functions being in a low-level library
that's getting in the way of doing this neatly.
I'll tell you what's bothering me, and what I think the possible solutions are.
In your patch, the two functions co-operate to parse the targets from the
command-line arguments:
svn_opt_args_to_target_array3()
which is (rightly or wrongly) in a low-level library, and
svn_cl__args_to_target_array()
which is in the command-line client code. The responsibility for detecting the
"^/" syntax and for character-encoding conversions and canonicalisation is not
clearly separated between these two places.
The code is structured like this:
+------------------------------------------+
| client |
+------------------------------------------+
| |
| +-----------------+
| | libsvn_wc |
| +-----------------+
| |
+---------------------------------------------+
| libsvn_subr |
| +-----------------+ |
| | svn_opt | |
| +-----------------+ |
| | |
| +-----------------+ |
| | svn_path | |
| +-----------------+ |
+---------------------------------------------+
Originally, when argument processing was simple, this worked OK. Now, we need
part of the argument processing code to use libsvn_wc in order to convert a
repository-relative URL to a full URL.
There are two possible approaches here.
1. Let svn_opt_args_to_target_arrayN() recognise repository-relative syntax,
but not try to determine the repository root, and pass the results back to the
client as repository-relative URLs. Let the client find the root and join the
relative part on to the root later. Let svn_opt_args_to_target_arrayN() do all
of the character encoding conversions and canonicalisation that it currently
does, on all targets including the repository-relative ones.
or
2. Let svn_opt_args_to_target_arrayN() recognise repository-relative syntax,
determine the repository root, join the relative part on to the root, and pass
back complete URLs to the client.
Either of these options would be OK.
Option 1 is pretty much what your patch does now, except that:
(a) The character encoding and canonicalisation of relative URLs is missing.
We must do this processing on the relative URLs, where it is currently written
to work on full URLs. The alternative, to defer this processing till later, is
messy because it means returning a mixture of processed and unprocessed
arguments. It would also require public access to the processing code which is
presently in line in svn_opt_args_to_target_array3().
(b) Distinguishing a rel-URL from a local path. When returning the array of
processed targets, svn_opt_args_to_target_array3() presently returns them all
in internal, canonical, properly URI-encoded form, and the way to distinguish
between a local path and a URL is unambiguously defined (using
svn_path_is_url() to recognises a "scheme://" prefix). When we come to add
relative URLs into the array, we have chosen in this patch to continue using
the "^/" prefix to distinguish it from a full URL or a local path. However,
this leaves room for ambiguity with a local path that was specified on the
command-line as "./^/local/path" and canonicalised to "^/local/path".
The fact that a "^/" prefix was the way to recognise an incoming relative URL
on the command line doesn't mean it has to be the way it is communicated
internally later on. We could use a different way to distinguish the type of
target, such as attaching an enumeration { TARGET_TYPE_URL,
TARGET_TYPE_REL_URL, TARGET_TYPE_PATH }.
Option 2 involves moving the argument processing out of libsvn_subr and up to a
higher layer. Architecture for option 2:
+------------------------------------------+
| client |
+------------------------------------------+
| |
+---------------------+ |
| svn_opt_args_... | |
+---------------------+ |
| | |
| +-----------------+
| | libsvn_wc |
| +-----------------+
| |
+---------------------------------------------+
| libsvn_subr |
| |
| +-----------------+ |
| | svn_path | |
| +-----------------+ |
+---------------------------------------------+
In fact, if you look at the comments in svn_opt_args_to_target_array3(), you'll
see that some time ago we already reached the point of needing to do this, but
instead we have hacked around it by duplicating knowledge of the special WC
directory names ".svn" and "_svn" here.
I feel this "option 2" is the better route, but right now I'm open to either
way, and I acknowledge that moving code to a different architectural layer is a
bigger task that you might be less comfortable with. Ideally I or someone else
would prepare this new layer first, and then you would be able to use it.
Troy Curtis Jr wrote:
> [[[
> Implement repository root relative url support for the svn command-line
> client. This allows the user to use to type '^/' in front of any target to
> mean the repository root url. The repository root url is determined by
> checking the other arguments' root urls (if they exist) and using that common
> url. If none is found the root url of the current directory is used. If no
> common repository root url can be found, an error is generated.
>
> * subversion/libsvn_subr/opt.c
> (svn_opt_args_to_target_array3): Special case a bare repository root
> relative url, '^/', by simply using it without any further processing.
> * subversion/libsvn_client/externals.c
> (resolve_relative_external_url): Include a brief note mentioning the fact
> there is a partial reimplementation in libsvn_subr/path.c.
>
> * subversion/svn/cl.h
> (svn_cl__is_relative_url): New function prototype.
> (svn_cl__resolve_relative_url): New function prototype.
Let's call these functions ..._repos_relative_url(). It might be that we
anticipate one day making them handle more general relative syntax variations
(like the "externals" functions) and therefore wanting the more general names,
but I decided that for now it's better to avoid any confusion that might arise
from people assuming that "relative URL" means what it would normally means
outside the context of svn command-line client argument syntax. I made this change.
> (svn_cl__args_to_target_array_print_reserved): Deleted old function
> prototype.
> (svn_cl__args_to_target_array): Add new function prototype.
>
> * subversion/svn/util.c
> (svn_cl__is_relative_url): New function to test whether a given string is a
> relative url.
> (svn_cl__resolve_relative_url): New function to resolve a relative url
> given the repository root url.
> (svn_cl__args_to_target_array_print_reserved): Replace this function with a
> more generically named svn_cl__args_to_target_array().
> (svn_cl__args_to_target_array): Resolve repository root relative urls in the
> arguments and known_targets.
>
> - Show quoted text -
> * subversion/svn/propdel-cmd.c,
> subversion/svn/merge-cmd.c,
> subversion/svn/checkout-cmd.c,
> subversion/svn/move-cmd.c,
> subversion/svn/mkdir-cmd.c,
> subversion/svn/cat-cmd.c,
> subversion/svn/revert-cmd.c,
> subversion/svn/diff-cmd.c,
> subversion/svn/copy-cmd.c,
> subversion/svn/mergeinfo-cmd.c,
> subversion/svn/list-cmd.c,
> subversion/svn/blame-cmd.c,
> subversion/svn/propget-cmd.c,
> subversion/svn/changelist-cmd.c,
> subversion/svn/log-cmd.c,
> subversion/svn/resolved-cmd.c,
> subversion/svn/cleanup-cmd.c,
> subversion/svn/commit-cmd.c,
> subversion/svn/add-cmd.c,
> subversion/svn/propset-cmd.c,
> subversion/svn/switch-cmd.c,
> subversion/svn/delete-cmd.c,
> subversion/svn/import-cmd.c,
> subversion/svn/proplist-cmd.c,
> subversion/svn/export-cmd.c,
> subversion/svn/status-cmd.c,
> subversion/svn/propedit-cmd.c,
> subversion/svn/lock-cmd.c,
> subversion/svn/info-cmd.c,
> subversion/svn/unlock-cmd.c
> (svn_cl__add, svn_cl__blame, svn_cl__cat, svn_cl__changelist,
> svn_cl__checkout, svn_cl__cleanup, svn_cl__commit, svn_cl__copy,
> svn_cl__delete, svn_cl__diff, svn_cl__export, svn_cl__import, svn_cl__info,
> svn_cl__list, svn_cl__lock, svn_cl__log, svn_cl__merge, svn_cl__mergeinfo,
> svn_cl__mkdir, svn_cl__move, svn_cl__propdel, svn_cl__propedit,
> svn_cl__propget, svn_cl__proplist, svn_cl__propset, svn_cl__resolved,
> svn_cl__revert, svn_cl__status, svn_cl__switch, svn_cl__unlock):
> Replaced the usage of svn_cl__args_to_target_array_print_reserved() with
> the more generically named svn_cl__args_to_target_array() which combines
> the original function's functionality with relative url resolution.
> (svn_cl__diff): Add access to the client context handle.
>
> * subversion/svn/update-cmd.c
> (svn_cl__update): Replace svn_opt_args_to_target_array3() with
> svn_cl__args_to_target_array().
>
> * subversion/tests/cmdline/basic_tests.py
> (basic_relative_url_using_current_dir): New test.
> (basic_relative_url_using_other_targets): New test.
> (basic_relative_url_multi_repo): New test.
> (test_list): Run new tests.
> ]]]
>
> Troy
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/libsvn_subr/opt.c
> ===================================================================
> --- subversion/libsvn_subr/opt.c (revision 29570)
> +++ subversion/libsvn_subr/opt.c (working copy)
> @@ -962,9 +962,18 @@
> utf8_target,
> peg_start - utf8_target);
>
> - /* URLs and wc-paths get treated differently. */
> - if (svn_path_is_url(utf8_target))
> + /* URLs, relative repository root URLs, and wc-paths each get treated
> + * differently. */
> + if (0 == strcmp("^/", utf8_target))
> {
> + /* Any processing of the relative repository root url will remove
> + * the needed trailing slash. There are no other components to
> + * escape or process so just return it as is. */
> + /* Convert to URI. */
> + target = apr_pstrdup(pool, utf8_target);
> + }
> + else if (svn_path_is_url(utf8_target))
> + {
That's not quite right. If the argument is in relative-URL syntax but is not
just "^/", it will fail both of these code paths and go through the "native
filename" conversions.
> /* No need to canonicalize a URL's case or path separators. */
>
> /* Convert to URI. */
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 29570)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -2159,7 +2159,93 @@
>
>
> #----------------------------------------------------------------------
> +# Relative urls
> +#
> +# Use blame to test three specific cases for relative url support.
> +def basic_relative_url_using_current_dir(sbox):
> + "basic relative url target using current dir"
>
> + # We'll use blame to test relative url parsing
> + sbox.build()
> +
> + # First, make a new revision of iota.
> + iota = os.path.join(sbox.wc_dir, 'iota')
> + svntest.main.file_append(iota, "New contents for iota\n")
> + svntest.main.run_svn(None, 'ci',
> + '-m', '', iota)
> +
> + expected_output = [
> + " 1 jrandom This is the file 'iota'.\n",
> + " 2 jrandom New contents for iota\n",
> + ]
> +
> + orig_dir = os.getcwd()
> + os.chdir(sbox.wc_dir)
> +
> + output, error = svntest.actions.run_and_verify_svn(None,
> + expected_output, [],
> + 'blame', '^/iota')
> +
> + os.chdir(orig_dir)
> +
> +def basic_relative_url_using_other_targets(sbox):
> + "basic relative url target using other targets"
> +
> + sbox.build()
> +
> + # First, make a new revision of iota.
> + iota = os.path.join(sbox.wc_dir, 'iota')
> + svntest.main.file_append(iota, "New contents for iota\n")
> + svntest.main.run_svn(None, 'ci',
> + '-m', '', iota)
> +
> + # Now, make a new revision of A/mu .
> + mu = os.path.join(sbox.wc_dir, 'A', 'mu')
> + mu_url = sbox.repo_url + '/A/mu'
> +
> + svntest.main.file_append(mu, "New contents for mu\n")
> + svntest.main.run_svn(None, 'ci',
> + '-m', '', mu)
> +
> +
> + expected_output = [
> + " 1 jrandom This is the file 'iota'.\n",
> + " 2 jrandom New contents for iota\n",
> + " 1 jrandom This is the file 'mu'.\n",
> + " 3 jrandom New contents for mu\n",
> + ]
> +
> + output, error = svntest.actions.run_and_verify_svn(None,
> + expected_output, [], 'blame',
> + '^/iota', mu_url)
> +
> +def basic_relative_url_multi_repo(sbox):
> + "basic relative url target with multiple repos"
> +
> + sbox.build()
> + repo_url1 = sbox.repo_url
> + repo_dir1 = sbox.repo_dir
> + wc_dir1 = sbox.wc_dir
> +
> + repo_dir2, repo_url2 = sbox.add_repo_path("other")
> + svntest.main.copy_repos(repo_dir1, repo_dir2, 1, 1)
> + wc_dir2 = sbox.add_wc_path("other")
> + svntest.actions.run_and_verify_svn("Unexpected error during co",
> + svntest.verify.AnyOutput, [], "co",
> + repo_url2,
> + wc_dir2)
> +
> + # Don't bother with making new revisions, the command should not work.
> + iota_url_repo1 = repo_url1 + '/iota'
> + iota_url_repo2 = repo_url2 + '/iota'
> +
> + output, error = svntest.actions.run_and_verify_svn(None, [],
> + svntest.verify.AnyOutput, 'blame',
> + '^/A/mu', iota_url_repo1, iota_url_repo2)
> +
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 29570)
> +++ subversion/svn/cl.h (working copy)
> @@ -555,7 +555,48 @@
> "file" or, in any other case, the empty string. */
> const char *svn_cl__node_kind_str(svn_node_kind_t kind);
>
> +/*
> + * Return true iff URL is a relative URL. Specifically that it starts
> + * with the characters "^/".
> + */
> +svn_boolean_t svn_cl__is_relative_url(const char *url);
>
> +/*
> + * Convert the possibly relative url in RELATIVE_URL to an absolute
> + * url using REPOS_ROOT_URL and stick it in ABSOLUTE_URL. If the
> + * RELATIVE_URL starts with the characters '^/', they are replaced by
> + * the repository root url, otherwise the RELATIVE_URL is returned.
> + */
> +svn_error_t *
> +svn_cl__resolve_relative_url(const char **absolute_url,
> + const char *relative_url,
> + const char *repos_root_url,
> + apr_pool_t *pool);
> +
> +/* Command-line client argument and target list parser:
> + * - Runs OS and KNOWN_TARGETS through the svn_opt_args_to_target_array3()
> + * function to get a concatentated target list from the command-line
> + * arguments (via OS) and KNOWN_TARGETS.
> + * - Prints a warning if a reserved filename is specified in the target list.
> + * - Resolves relative urls present in the target list using the following
> + * logic:
> + * - Find the root url for all the non-relative urls and paths (if they have
> + * one) using the client context, CTX.
> + * - Print an error and return if the root urls do not match
> + * - If there are no root urls found (all targets are relative urls or
> + * non-existant), use the root url of the current directory
> + * - If the current directory is not a working copy, print an error and
> + * return.
> + * - Replace the '^/' characters in the relative urls with the common root
> + * url of the other targets
> + */
> +svn_error_t *
> +svn_cl__args_to_target_array(apr_array_header_t **targets_ret,
> + apr_getopt_t *os,
> + apr_array_header_t *known_targets,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
I've edited the doc-strings a bit.
> Index: subversion/svn/util.c
> ===================================================================
> --- subversion/svn/util.c (revision 29570)
> +++ subversion/svn/util.c (working copy)
> @@ -35,6 +35,7 @@
> #include <apr_tables.h>
> #include <apr_general.h>
> #include <apr_lib.h>
> +#include <apr_uri.h>
>
> #include "svn_pools.h"
> #include "svn_error.h"
> @@ -1009,15 +1010,94 @@
> }
> }
>
> +svn_boolean_t
> +svn_cl__is_relative_url(const char *url)
> +{
> + return(strncmp("^/", url, 2) ? FALSE : TRUE);
> +}
>
> svn_error_t *
> -svn_cl__args_to_target_array_print_reserved(apr_array_header_t **targets,
> - apr_getopt_t *os,
> - apr_array_header_t *known_targets,
> - apr_pool_t *pool)
> +svn_cl__resolve_relative_url(const char **absolute_url,
> + const char *relative_url,
> + const char *repos_root_url,
> + apr_pool_t *pool)
> {
> - svn_error_t *error = svn_opt_args_to_target_array3(targets, os,
> - known_targets, pool);
> + apr_status_t status;
> + apr_array_header_t *base_components;
> + apr_array_header_t *relative_components;
> + apr_uri_t parsed_uri;
> +
> + if (svn_path_is_url(relative_url))
> + {
> + *absolute_url = apr_pstrdup(pool, relative_url);
> +
> + return SVN_NO_ERROR;
> + }
I don't see any need for this function to accept non-repos-relative URLs. I've
removed this clause.
> +
> + /*
> + * This is a partial re-implementation of the relative external support
> + * found in:
> + * libsvn_client/externals.c: resolve_relative_external_url()
> + *
> + * Updates to this code MAY be applicable to the other code
> + * (and vice versa).
> + */
> +
> + if (0 == strncmp("^/", relative_url, 2))
> + {
> + /* Is there something beyond '^/'? If not just use what we have
> + * (the repository root URL)
> + */
> + if (strlen(relative_url) == 2)
> + {
> + *absolute_url = apr_pstrdup(pool, repos_root_url);
> + }
> + else
> + {
> + status = apr_uri_parse(pool, repos_root_url, &parsed_uri);
> + if (status)
> + return svn_error_createf(SVN_ERR_BAD_URL, 0,
> + _("Illegal repository root URL '%s'"),
> + repos_root_url);
> +
> + base_components = svn_path_decompose(parsed_uri.path,
> + pool);
> +
> + relative_components = svn_path_decompose(relative_url + 2,
> + pool);
> +
> + apr_array_cat(base_components, relative_components);
> +
> + parsed_uri.path = (char *)svn_path_compose(base_components,
> + pool);
> + parsed_uri.query = NULL;
> + parsed_uri.fragment = NULL;
> +
> + *absolute_url = apr_uri_unparse(pool, &parsed_uri, 0);
> + }
> + }
These 30-odd lines accomplish concatenating relative_url (less its "^/" prefix)
on to repos_root_url. That can be done in one line with the function
"svn_path_join". I've made that change.
> + else
> + return svn_error_createf(SVN_ERR_BAD_URL, 0,
> + _("Improper relative URL '%s'"),
> + relative_url);
> +
> + return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
> +svn_cl__args_to_target_array(apr_array_header_t **targets_ret,
> + apr_getopt_t *os,
> + apr_array_header_t *known_targets,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + int i;
> + const char *root_url = NULL;
> + apr_array_header_t *targets;
> + svn_error_t *error;
> +
> + error = svn_opt_args_to_target_array3(&targets, os, known_targets, pool);
> +
> if (error)
> {
> if (error->apr_err == SVN_ERR_RESERVED_FILENAME_SPECIFIED)
> @@ -1028,10 +1108,96 @@
> else
> return error;
> }
> +
> + /*
> + * First determine whether to even bother with root urls,
> + * at least one relative url for it to matter.
> + */
> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> +
> + if (svn_cl__is_relative_url(target))
> + break;
> + }
> +
> + /* Was a relative url found? */
> + if (i >= targets->nelts)
> + {
> + *targets_ret = targets;
> + return SVN_NO_ERROR;
> + }
> +
> + /*
> + * Now determine the common root url to use.
> + */
> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> + const char *tmp_root_url;
> +
> + if (svn_cl__is_relative_url(target))
> + continue;
> +
> + if ((error = svn_client_root_url_from_path(&tmp_root_url, target,
> + ctx, pool)))
> + {
> + if ( (error->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
> + || (error->apr_err == SVN_ERR_WC_NOT_DIRECTORY))
I would like to see the practical effect of these exceptions: what sort of
commands will unexpectedly choose the current working directory's repository
because of this?
Here's a made-up example of what I mean. This example isn't showing a real
error message:
$ cd ~/tmp/sandbox
$ svn copy ^/branches/1.4.x ~/src/subversion/tags/1.5.x
svn: 'branches/1.4.x' not found in the repository of 'sandbox'
In these cases, where the target is a not-yet-existing path or URL (perhaps a
target of "svn copy"?), could we nevertheless determine the repository in which
it will exist so that we don't get these surprises? Or is that not the
problematic case?
> + {
> + svn_error_clear(error);
> + continue;
> + }
> + else
> + return error;
> + }
> + else if ( (root_url != NULL)
> + && (strcmp(root_url, tmp_root_url) != 0))
> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("All non-relative targets must have the same root url."));
> + else
> + root_url = tmp_root_url;
> + }
> +
> + /*
> + * Use the current directory's root url if one wasn't found using the
> + * arguments.
> + */
> + if (root_url == NULL)
> + {
> + if ((error = svn_client_root_url_from_path(&root_url,
> + svn_path_canonicalize(".", pool),
> + ctx, pool)))
> + return error;
> + }
> +
> + /*
> + * Finally, resolve any relative urls found in the targets array.
> + */
> +
> + *targets_ret = apr_array_make(pool, targets->nelts, sizeof(const char *));
> +
> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> +
> + if (svn_cl__is_relative_url(target))
> + {
> + const char *abs_target;
> +
> + SVN_ERR(svn_cl__resolve_relative_url(&abs_target, target,
> + root_url, pool));
(So here, the "resolve" function doesn't need to deal with any kinds of targets
other than repos-relative URLs, which is why I deleted that part of it. Keep it
simple... when we can.)
> + APR_ARRAY_PUSH(*targets_ret, const char *) = abs_target;
> + }
> + else
> + {
> + APR_ARRAY_PUSH(*targets_ret, const char *) = target;
> + }
> + }
> +
> return SVN_NO_ERROR;
> }
Now, I've attached a patch that I've been playing around with. It's no good
exactly as it is, because I've started making changes towards "option 1" and
towards "option 2". I'm just attaching it so you can have a look at some of
what I did.
Let me know what you think about all of this.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-07 00:12:45 CET