"Troy Curtis Jr" <troycurtisjr_at_gmail.com> writes:
> Here is my revised patch, incorporating various suggestions/comments
>
> [[[
> Fix peg revision support for repository root urls to address issue
> #3193.
>
> Move the duplicated peg revision parsing functionality from
> svn_client_args_to_target_array(), svn_opt_args_to_target_array3(),
> and
> svn_opt_parse_path() into a new function
> svn_opt__split_arg_at_peg_revision().
>
> * subversion/libsvn_subr/opt.c
> (svn_opt__split_arg_at_peg_revision): New function.
> (svn_opt_parse_path,
> svn_opt_args_to_target_array3): Replace in-line peg revision
> parsing with
> the new svn_opt__split_arg_at_peg_revision() function.
>
> * subversion/include/private/svn_opt_private.h
> (svn_opt__split_arg_at_peg_revision): New function prototype.
>
> * subversion/libsvn_client/cmdline.c
> (svn_opt_args_to_target_array3): Replace in-line peg revision
> parsing with
> the new svn_opt__split_arg_at_peg_revision() function.
>
> * subversion/tests/cmdline/basic_tests.py
> (basic_relative_url_with_peg_revisions): New test function.
> (test_list): Call the new test function.
> ]]]
I tightened up the log message; see r31458.
> --- subversion/libsvn_subr/opt.c (revision 31446)
> +++ subversion/libsvn_subr/opt.c (working copy)
> @@ -810,78 +810,68 @@ svn_opt_parse_path(svn_opt_revision_t *rev,
> const char *path /* UTF-8! */,
> apr_pool_t *pool)
> {
> - int i;
> + const char *peg_rev;
>
> - /* scanning from right to left, just to be friendly to any
> - screwed-up filenames that might *actually* contain @-signs. :-) */
> - for (i = (strlen(path) - 1); i >= 0; i--)
> + SVN_ERR(svn_opt__split_arg_at_peg_revision(truepath, &peg_rev, path, pool));
> +
> + /* Parse the peg revision, if one was found */
> + if (strlen(peg_rev))
This means that truepath (which is a return parameter) may get set even
if later parsing errors because the peg_rev is invalid. That's okay,
but it would be a good idea to change the svn_opt_parse_path() doc
string to make this clear, as it currently implies otherwise.
No big deal, tweaked it when I committed.
> @@ -1115,6 +1090,48 @@ svn_opt_parse_revprop(apr_hash_t **revprop_table_p
> }
>
> svn_error_t *
> +svn_opt__split_arg_at_peg_revision(const char **true_target,
> + const char **peg_revision,
> + const char *utf8_target,
> + apr_pool_t *pool)
> +{
> + const char *peg_start = NULL; /* pointer to the peg revision, if any */
> + int j;
> +
> + for (j = (strlen(utf8_target) - 1); j >= 0; --j)
> + {
> + /* If we hit a path separator, stop looking. This is OK
> + only because our revision specifiers can't contain
> + '/'. */
> + if (utf8_target[j] == '/')
> + break;
> +
> + if (utf8_target[j] == '@')
> + {
> + peg_start = &utf8_target[j];
> + break;
> + }
> + }
> +
> + if (peg_start)
> + {
> + *true_target = apr_pstrmemdup(pool,
> + utf8_target,
> + j);
Much vertical space could be saved by putting those arguments on the
same line. I wouldn't normally mention it, but I noticed it for the
comment above too -- the "'/'" could go on the previous line. It would
make things a little easier on readers, IMHO. Putting argument lists
into line groupings does make sense when there are subgroups within the
arguments, or when there are simply too many arguments to fit in the
standard 80-column width, but neither is the case here.
> + else
> + {
> + *true_target = utf8_target;
> +
> + *peg_revision = apr_pstrdup(pool, "");
> + }
> +
> + return SVN_NO_ERROR;
> +}
No need to strdup "" into a pool. Just assign "" directly; the static
storage won't hurt anyone, will it?
Oh, the blank line in the else-body: no need for it.
> --- subversion/include/private/svn_opt_private.h (revision 31446)
> +++ subversion/include/private/svn_opt_private.h (working copy)
> @@ -28,6 +28,24 @@
> extern "C" {
> #endif /* __cplusplus */
>
> +/* Extract the peg revision, if any, from UTF8_TARGET. Return the peg
> + * revision in *PEG_REVISION and the true target portion in *TRUE_TARGET.
> + * *PEG_REVISION will be an empty string if no peg revision is found.
> + *
> + * UTF8_TARGET need not be canonical. *TRUE_TARGET will not be canonical
> + * unless UTF8_TARGET is.
> + *
> + * Note that *PEG_REVISION will still contain the '@' symbol as the first
> + * character if a peg revision was found.
> + *
> + * All allocations are done in POOL.
> + */
> +svn_error_t *
> +svn_opt__split_arg_at_peg_revision(const char **true_target,
> + const char **peg_revision,
> + const char *utf8_target,
> + apr_pool_t *pool);
> +
Interestingly, because of the way you've worded it, this doc string
remains accurate if we assign "" directly as a static string :-).
> --- subversion/libsvn_client/cmdline.c (revision 31446)
> +++ subversion/libsvn_client/cmdline.c (working copy)
> @@ -196,40 +196,7 @@ svn_client_args_to_target_array(apr_array_header_t
> for (i = 0; i < input_targets->nelts; i++)
> {
> const char *utf8_target = APR_ARRAY_IDX(input_targets, i, const char *);
> - const char *peg_start = NULL; /* pointer to the peg revision, if any */
> - const char *target;
> - int j;
>
> - /* Remove a peg revision, if any, in the target so that it 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. */
> - for (j = (strlen(utf8_target) - 1); j >= 0; --j)
> - {
> - /* If we hit a path separator, stop looking. This is OK
> - only because our revision specifiers can't contain
> - '/'. */
> - if (utf8_target[j] == '/')
> - break;
> - if (utf8_target[j] == '@')
> - {
> - peg_start = utf8_target + j;
> - break;
> - }
> - }
> - if (peg_start)
> - utf8_target = apr_pstrmemdup(pool,
> - utf8_target,
> - peg_start - utf8_target);
> -
> /* Relative urls will be canonicalized when they are resolved later in
> * the function
> */
> @@ -239,37 +206,53 @@ svn_client_args_to_target_array(apr_array_header_t
> }
> else
> {
> + const char *true_target;
> + const char *peg_rev;
> + const char *target;
> +
> + /*
> + * This is needed so that the 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.
> + */
> + SVN_ERR(svn_opt__split_arg_at_peg_revision(&true_target, &peg_rev,
> + utf8_target, pool));
> +
It's too bad this comment and the code following it has to be repeated,
but further abstractions could (and probably should) be a separate
patch. In case it's not clear: I'm not complaining, just observing!
Applied in r31458, with doc string and formatting tweaks mentioned
above. Thanks!
While reviewing this, I didn't look ahead much to the "IP address
bogusly interpreted..." thread. If further changes are necessary for
that, at least they have a better base to start from.
Does this close issue #3193 yet? (By the way, you could "Accept" the
issue and mark it as started.
-Karl
---------------------------------------------------------------------
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-26 23:22:17 CEST