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.
> 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.
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
>
>
---------------------------------------------------------------------
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-01 15:31:00 CEST