Troy Curtis Jr wrote:
> On Thu, Mar 6, 2008 at 5:12 PM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
>>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.
My latest thought is that this would fit logically in libsvn_client. It might
not be generic to ALL clients - it's very much aimed at command-line clients -
but I don't think that means it would be wrong to put it in libsvn_client.
That would mean the present svn, svnlook, svnadmin, etc. could share it if they
want to, as well as third-party clients.
>>>+ 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?
I'm not sure how to do this. Pulling off components and re-trying would work in
some cases but not in general.
> 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.
I think we can just do the usual API-revision thing: the existing
svn_opt_args_to_target_array3() will remain available but deprecated, and the
new function will be svn_client_args_to_target_array() (name reflecting its new
location) will be the preferred replacement.
This is likely to involve some code duplication, but it need not be too much
because (as in the patch I attached to the last email [1]) we can factor out
some of the common parts and make them available from the lower-level library
as private inter-library API calls ("svn_opt__arg...").
I was previously concerned that we'd have to create a new library to do this
properly, which would be a bit of a hassle, but unless someone finds an
objection to it going in libsvn_client I think we have a relatively easy route.
So, let's say you create
svn_client_args_to_target_array()
- public, in new file libsvn_client/cmdline.c, like the version I called
svn_cl__args_to_target_array4() in [1], which calls
svn_wc_is_adm_dir()
- instead of duplicating the WC's knowledge of admin. dir. names;
svn_opt__arg_canonicalize_url()
svn_opt__arg_canonicalize_path()
- these two being private but inter-library APIs as suggested in [1], with
declarations in svn_opt.h, to avoid too much code duplication because the
deprecated svn_opt_args_to_target_array3() will still have to perform these two
kinds of processing.
Then it sounds like everybody wants the results returned in a structure with
separate fields for the "target kind", the path, and the peg revision. (I'd
leave aside the talk about an "operative revision" field for now; that's not
currently in the scope of these functions and would be an extra complication.)
You can decide whether to do the character-encoding and canonicalisation
before or after resolving to a relative URL. To do it before, you'd need to
ensure all of the conversion and checking funcs that are involved can accept a
relative URL, which at least one of them presently can't. To do it afterwards,
you can have the "inner" function (a file-static function that I called
svn_cl__args_to_target_array4()) return targets among which the relative-URLs
haven't been processed, and then the "outer" function that resolves them
against a repository root can process them afterwards. Both ways seem equally
good to me at the moment.
If you're up for this I'm willing to help.
Thanks.
- Julian
[1] My unfinished patch for suggestive purposes,
<http://svn.haxx.se/dev/archive-2008-03/att-0159/relative-url-jaf.patch>,
attached to email <http://svn.haxx.se/dev/archive-2008-03/0159.shtml>.
---------------------------------------------------------------------
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 11:22:50 CET