On Fri, Mar 7, 2008 at 4:22 AM, Julian Foad <julianfoad_at_btopenworld.com> wrote:
> 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.
>
>
Yes I really think this sounds like a much better solution. The
previous one's were
really hacks aimed at changing as little as possible instead of just
"doing it right".
>
>
> >>>+ 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'll dig around some, surely there is some existing way to determine
where a non-existent
path/url should go. I certainly could use a bit more knowledge about
the available APIs
(as evidenced by not knowing about svn_path_join()).
>
> > 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.
Ok, so these would go into something like
subversion/include/private/svn_opt.h (new file)?
I would think the existing svn_opt.h would be strictly public (based on my
search '/opt__' not matching any lines). Or am I missing something?
>
> 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.
Definitely...though you just signed up to be my mentor, not that you haven't
already done a fair amount of mentoring to me already! :) I'm sure there'll
be lots of questions, but I guess you'd realize that from our previous
conversations.
>
> 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>.
>
I'll come up with the structure, and some prototypes in various places and see
what you think. I'll also spend a bit of extra time looking around at more
existing functionality so that I can get a better idea what kinds of things
are around that are useful. In particular I'll need to really look at what it
is that canonicalisation is trying to accomplish and try to reduce my
ignorance of the various encoding issues (utf8, etc..).
Troy
PS Just noticed the C function names feature in your diff...hadn't realized
that it was implemented! I'll be using that a lot more often!
--
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-08 06:00:39 CET