Julian,
My comments are below.
On Thu, Mar 6, 2008 at 5:12 PM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> 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.
Yeah it always felt as though it really belonged inside
svn_opt_args_to_target_arrayN().
>
> 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".
>
Oh yes, I did not consider that last case there.
>
> 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.
>
I think it is certainly the best route, but one that I definetly was not
confortable submitting out of the gate: "Hey you don't know me bu
here's my patch
that completely changes the way svn parsing command-line arguments, oh and I'm
removing a piece of the public API to boot!". Though now that it's being
discussed I don't mind taking a stab at doing some of the "grunt" work needed.
>
>
> 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.
Ah yes, I just took it for granted that the path stuff would do the necessary
operations, but they are definetly not the same!
>
>
> > /* 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.
>
>
Ah my ignorance of svn_path_* shows! That code was copy-and-pasted from the
externals code, but of course he did that method so that he could pop
backpaths out of the components array.
>
> > + 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?
This is exactly the case. One of the cases is for a non-existant url and the
other is for a non-existant filesystem path. Discovered by trial-and-error
as I am still a Subversion code novice (but maybe not so much after this all
gets implemented!). It certainly seems like a good idea to try a bit harder
to find out what repository a non-existant target is meaning. We could keep
pulling components off the end until we find a match, and if we don't then
fall through.
Would it be enough to just try removing one component? Is it even valid to
have a path specifying non-existant parents?
>
>
> > + {
> > + 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
>
I definetly think option 2 is really the right way to do things. Also coupled
with Pilato's and Wright's request it could be useful for things other than
relative url parsing.
So are you advocating putting all these things inside the command line client
code, or perhaps stick them in libsvn_client. It seems to me if we
effectively deprecate the svn_opt_args_to_target_array3() function, then we
ought to provide a public equivalent for the "Next Generation". Or it may be
that no other projects use that API and it is unneeded.
Troy
--
Beware of spyware. If you can, use the Firefox browser. - USA Today
Download now at http://getfirefox.com
Registered Linux User #354814 ( http://counter.li.org/)
---------------------------------------------------------------------
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 06:13:23 CET