Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.

>>> Maybe it would be better to just have svn_opt__arg_canonicalize_path()
>>> take a new *peg_rev parameter, which (if the parameter itself is
>>> non-NULL) returns any peg rev found, as well as returning the
>>> canonicalized true target. Then we'd have the logic for parsing peg
>>> revs centralized in one place (including handling escaping), and some
>>> duplicate code could go away.
>>>
>>> Was this discussed already? I didn't follow the thread in detail.
>>
>> Only between the voices in my head. I thought about doing something like this
>> but decided against it for two reasons:
>> - Returning a peg revision seperately does not really do what the function
>> name implies. The function 'canonicalizes' the given value, it
>> happens to also be able to cope with peg revisions. Returning
>> a peg revision would be like a mix of a 'canonicalize' function and my
>> split_arg_at_peg_revision() function.
>
> Sure, but remember it's fine to change the name of an internal function
> (that is, any file-static function, and any of the two-underscore
> functions like "svn_foo__bar_baz_qux()").
>
>> - If I did go that route I knew that I would need to implement the 'if peg
>> rev argument is NULL don't return one there' and thought the devs
>> did not like that kind of thing in one of my previous patches.
>
> Hmm, I don't know who said what, but we do that in other places, and I
> don't recall any objections to the practice. I think it's fine.

So say I change svn_opt__arg_canonicalize_path() to return the peg revision. It
probably makes the most sense for it to return the value of the peg revision,
but not the '@' symbol itself, as it is really a delimiter.
However, the *args_to_target_array*() functions would still have to
reconstruct the target so that their caller would receive something that it
expected. That would mean sticking the '@' symbol back into the string with
something like
   path_out = apr_pstrcat(pool, true_path, "@", peg_rev, NULL);

So really the caller is still involved with peg revision syntax.

Or if the '@' is left in, then if any caller needed to use it for anything
other than sticking it back onto the returned true_path, then it too would
have to be aware of peg revision syntax. (Granted the only users at this
point would not need it, but I assume making a general purpose interface
is best?)

The net result of having the svn_opt__arg_canonicalize_path() return
the peg revision separately replaces this line in the path processing section
of the *args_to_target_array functions:

   if (peg_rev = strrchr(base_name, '@'))
     *peg_rev = '\0';

With this at the end:
   if (peg_rev)
    target = apr_pstrcat(pool, target, peg_rev, NULL);
       - or -
    target = apr_pstrcat(pool, target, '@', peg_rev, NULL);

Do you still think this is worth while?

>
>> I could create a helper function that took a target and
>> determined whether it is a adm dir, but could recognoize peg revisions. Not
>> sure what I would call such a function at the moment, but it is an idea.
> Karl Said:
> The term "adm dir" usually means a .svn/ subdirectory in a working
> copy. Is that what you meant here?
>
>> When I initially coded this I did not like the duplication that much either,
>> but decided to keep it because I really felt like the
>> svn_client_args_to_target_array() and svn_opt_args_to_target_array3()
>> already had so much in common that this fit right in. I couldn't create a
>> private helper function like I did with split_arg_at_peg_revision() because
>> they are in two different libraries.
>
> I think this is what the subversion/include/private/ section of the code
> is for -- it's okay to have non-public but cross-library functions in
> Subversion.

I really was meaning a helper function that encloses all of the
basename, peg_rev NULLing,
adm dir name checking code into a common function. But one like that
would run into the same
issue that already exists inside the svn_opt_args_to_target_array3() function
where it can't use the svn_wc_is_adm_dir() function because of circular
dependencies.

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