Troy,
I saw that others were reviewing this for you, so I didn't join in. Now there's
been a pause, so I took a quick look at this. My thoughts are:
The pointer arithmetic was perfectly fine as it was! Might be simpler to use
strrchr("/") followed by strrchr("@") though.
From the point of view of parsing command-line syntax, I think removing the
peg revision specifier comes logically before canonicalising the rest of the
argument. Therefore, may I recommend not making the
svn_opt__arg_canonicalize_path()/_url() functions "cope with" a peg revision,
but leave them as they were, to operate on just a path or URL. Instead, make
your split_arg_at_peg_revision() function semi-public (maybe called
"svn_opt__arg_split_at_peg") and use it where previously the
*_args_to_target_array*() and other functions did their own parsing.
If your split_arg_at_peg_revision() will return the peg string with the "@"
still on it, then it could represent "no peg" with either NULL or the empty
string. I suggest it should never return NULL but rather the empty string when
there is no peg, so that callers can simply re-concatenate the peg without
checking for NULL. This seems a reasonable way to go for now, given that (as
you point out) some client functions still want to add the peg back on and then
take it off again later. In the long term, the clean way to organise it would
be for this initial splitting to pass the peg back separately (without its "@")
and for the path and the peg to be passed around separately thereafter. But
there's no need to go into that now.
That seems to make it all simpler. Does that make sense and is there anything
else I could help with? It would be great to see an update when you have time.
Thanks.
- Julian
Troy Curtis Jr wrote:
>>>>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
---------------------------------------------------------------------
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-22 13:07:52 CEST