On Thu, May 1, 2008 at 8:30 AM, Daniel Shahaf <d.s_at_daniel.shahaf.co.il> wrote:
> Please CC the list.
>
> Troy Curtis Jr wrote on Thu, 1 May 2008 at 06:54 -0500:
>
> > On Thu, May 1, 2008 at 3:02 AM, Daniel Shahaf <d.s_at_daniel.shahaf.co.il> wrote:
> > > Troy Curtis Jr wrote on Wed, 30 Apr 2008 at 22:28 -0500:
> > >
> > > > This is the first part of the patch necessary to address correctly handling
> > > > peg revision specifications in repository root urls.
> > > >
> > >
> > > Is it orthogonal to your "Fix assertion failure with root relative url"
> > > patch?
> > >
> >
> > Yes in reality it is. The assertion failure was caused by the use of
> > svn_path_join() which requires canonicalized inputs. I submitted them
> > together before because they were "root relative url fixes".
> >
>
> OK, that wasn't clear to me. That means I or someone else can review
> the other patch in parallel to reviewing this one.
>
>
> > >
> > > > [[[
> > > > Add peg revision support to svn_opt__arg_canonicalize_url() and
> > > > svn_opt__arg_canonicalize_path(). These functions are meant to process
> > > > user-input and so should handle the peg revision case.
> > > >
> > > > * subversion/libsvn_subr/opt.c
> > > > (split_arg_at_peg_revision): New function.
> > >
> > > How is it different from svn_opt_parse_path ?
> > >
> >
> > split_arg_at_peg_revision() deals strictly with text, whereas
> > svn_opt_parse_path() puts the peg revision in to a revision struct.
>
> In other words, the former returns the peg as string, and the latter as
> an svn_opt_revision_t.
>
> In this case, it seems to me that you use split_arg_at_peg_revision() as
> a helper function in svn_opt_parse_path(), instead of embedding the
> knowledge of peg-rev syntax in both of them.
>
Really split_arg_at_peg_revision() was created to prevent
duplication in the svn_opt__arg_canonicalize_*() functions. It seems to me
that this patch has a specific purpose of teaching
svn_opt__arg_canonicalize_*() about peg revision syntax. Perhaps a follow on
patch could also make use of split_arg_at_peg_revision() in
svn_opt_parse_path(), but I don't think it really 'fits' this patch.
>
>
> > Actually I copied a comment explaining this reasoning from the
> > svn_opt_args_to_target_array3() function to the doc string of this
> > function to address just this issue.
> >
> > >
> > > > (svn_opt__arg_canonicalize_url,
> > > > svn_opt__arg_canonicalize_path): Allow the input argument to contain a peg
> > > > revision specifier, and preserve it in the output utilizing
> > > > split_arg_at_peg_revision().
> > > >
> > > > * subversion/include/private/svn_opt_private.h
> > > > (svn_opt__arg_canonicalize_url,
> > > > svn_opt__arg_canonicalize_path): Update the doc string to reflect the fact
> > > > that these functions can handle a peg revision in the input argument.
> > > > ]]]
> > > >
> > > >
> > >
> > > > Index: subversion/libsvn_subr/opt.c
> > > > ===================================================================
> > > > --- subversion/libsvn_subr/opt.c (revision 30798)
> > > > +++ subversion/libsvn_subr/opt.c (working copy)
> > > > @@ -1114,14 +1083,75 @@
> > > > svn_error_t *
> > > > svn_opt__arg_canonicalize_url(const char **url_out, const char *url_in,
> > > > apr_pool_t *pool)
> > > > {
> > > > const char *target;
> > > > + const char *peg_rev;
> > > >
> > > > + SVN_ERR(split_arg_at_peg_revision(&target, &peg_rev, url_in, pool));
> > > > +
> > > > /* Convert to URI. */
> > > > - target = svn_path_uri_from_iri(url_in, pool);
> > > > + target = svn_path_uri_from_iri(target, pool);
> > > > /* Auto-escape some ASCII characters. */
> > > > target = svn_path_uri_autoescape(target, pool);
> > > >
> > > > @@ -1140,6 +1170,11 @@
> > > > /* strip any trailing '/' and collapse other redundant elements */
> > > > target = svn_path_canonicalize(target, pool);
> > > >
> > > > + /* Append the peg revision back to the canonicalized target if
> > > > + there was a peg revision. */
> > > > + if (peg_rev)
> > > > + target = apr_pstrcat(pool, target, peg_rev, NULL);
> > > > +
> > >
> > > So we parse the peg rev out, canonicalize, and concat it back (later
> > > to be parsed again). Why did you choose this way?
> > >
> >
> > The doc string of split_arg_at_peg_revision explains why.
> (the docstring is quoted below)
>
> > I kept it
> > this way because I believed the original reasoning made sense. In my
> > new implementation I would have duplicated the logic doing this in
> > three diffrenent places: back in the old
> > svn_opt_args_to_target_arrray3(), non root relative targets in
> > svn_client_args_to_target_array(), and root relative target in
> > svn_client_args_to_target_array(). This is why I factored it out. Of
> > course this change does not actually fix the ultimate issue, it lays
> > the foundation work for the 2 of 2 patch. This was suggested by
> > Blair.
> >
>
> The part that bothers me is "concat it back". The only callers of
> split_arg_at_peg_revision() are svn_opt__arg_canonicalize_{url,path};
> and you could modify them to return target and target's peg separately.
>
> Looking deeper, I see now that svn_opt_args_to_target_array3() returns
> an array of const char *, and subcommands expect to get an array of
> targets with peg-revs appended, which -- eventually -- is why you have
> to fold everything back to strings. It would be nicer to use
> svn_opt_parse_path and teach subcommands to expect structs, of course,
> but this can be done later.
Yeah this was discussed when I was originally moving the arg processing into
libsvn_client. We talked about returning an array of structure with
several bits
of useful information. I actually started down the path to figure out how to do
it, but the expectation of an array of const char * was extremely deeply
embedded.
>
>
> This is the docstring:
>
> +/* Extract the peg revision, if any, from UTF8_TARGET. Return the
> + * peg revision in *PEG_REVISION and the true target portion
> + * in *TRUE_TARGET.
> + *
> + * This is needed so that UTF8_TARGET can be properly canonicalized,
> + * otherwise the canonicalization does not treat a "._at_BASE" as a "."
> + * with a BASE peg revision, and it is not canonicalized to "@BASE".
> + * If any peg revision exists, it is appended to the final
> + * canonicalized path or URL. Do not use svn_opt_parse_path()
> + * because the resulting peg revision is a structure that would have
> + * to be converted back into a string. Converting from a string date
> + * to the apr_time_t field in the svn_opt_revision_value_t and back to
> + * a string would not necessarily preserve the exact bytes of the
> + * input date, so its easier just to keep it in string form.
> + *
> + * All allocations are done in POOL.
> + */
> +static svn_error_t *
> +split_arg_at_peg_revision(const char **true_target,
> + const char **peg_revision,
> + const char *utf8_target,
> + apr_pool_t *pool)
>
>
>
>
> > > > *url_out = target;
> > > > return SVN_NO_ERROR;
> > > > }
> > > > @@ -1151,9 +1186,12 @@
> > > > const char *apr_target;
> > > > char *truenamed_target; /* APR-encoded */
> > > > apr_status_t apr_err;
> > > > + const char *peg_rev;
> > > >
> > > > + SVN_ERR(split_arg_at_peg_revision(&apr_target, &peg_rev, path_in, pool));
> > > > +
> > > > /* canonicalize case, and change all separators to '/'. */
> > > > - SVN_ERR(svn_path_cstring_from_utf8(&apr_target, path_in, pool));
> > > > + SVN_ERR(svn_path_cstring_from_utf8(&apr_target, apr_target, pool));
> > > > apr_err = apr_filepath_merge(&truenamed_target, "", apr_target,
> > > > APR_FILEPATH_TRUENAME, pool);
> > > >
> > > > @@ -1174,6 +1212,11 @@
> > > > SVN_ERR(svn_path_cstring_to_utf8(path_out, apr_target, pool));
> > > > *path_out = svn_path_canonicalize(*path_out, pool);
> > > >
> > > > + /* Append the peg revision back to the canonicalized target if
> > > > + there was a peg revision. */
> > > > + if (peg_rev)
> > > > + *path_out = apr_pstrcat(pool, *path_out, peg_rev, NULL);
> > > > +
> > >
> > > I've seen this hunk before. Do you need an helper function here?
> > >
> >
> > I don't think so, no where it makes sense. Pretty much it would be
> > replacing apr_pstrcat() with some other function that just did an
> > apr_pstrcat(). I'd still need the check there to see IF I needed to
> > append the revision.
> >
>
> I agree now. Earlier, I saw the svn_path_canonicalize() call in both
> hunks and the identical four lines added, and automatically assumed
> these two functions might have enough in common to justify a shared
> helper. Looking at the actual functions now, that is not the case.
> Sorry for misleading you.
>
> Daniel
>
>
>
> > > > return SVN_NO_ERROR;
> > > > }
> > > >
> > >
> >
> > Troy
> >
> >
>
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-05-04 22:37:13 CEST