On Mon, May 26, 2008 at 4:22 PM, Karl Fogel <kfogel_at_red-bean.com> wrote:
> "Troy Curtis Jr" <troycurtisjr_at_gmail.com> writes:
>> + 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.
>
Yeah the original version was too long, I just neglected to join the
lines after I deleted things.
>> + 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?
>
Ok, I'm still trying to get used to all this pool based things. The
static storage is OK here because the string is never explicitly freed
by the caller, just the pool is freed at some point in the future. Do
I understand that correctly?
> 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
>
I have accepted it, and I do believe that this commit fixes it. Is
the standard procedure to have someone else verify it and close it?
Like the reporter?
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-27 03:54:06 CEST