You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Troy Curtis Jr <tr...@gmail.com> on 2008/05/11 20:57:30 UTC
[PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
My improved patch for fixing the peg revision support while parsing repository
root urls. I consolidated what I had previously split into two parts at
Blair's request once I figured out that applying the first patch caused 3 test
failures as a result of improperly parsing peg revision for "real" targets.
The improvement here is using the new split_arg_at_peg_revision() function
inside svn_opt_parse_path(). This was suggested by Daniel Shahaf to further
reduce duplication of peg revision parsing.
[[[
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() and svn_opt_args_to_target_array3() into
the common functions svn_opt__arg_canonicalize_url() and
svn_opt__arg_canonicalize_path().
* subversion/libsvn_subr/opt.c
(split_arg_at_peg_revision): New function.
(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().
(svn_opt_parse_path): Replace in-line peg revision parsing with the new
split_arg_at_peg_revision() function.
(svn_opt_args_to_target_array3): Remove the peg revision preserving logic,
relying instead on the new peg revision support in
svn_opt__arg_canonicalize_path() and svn_opt__arg_canonicalize_url().
* 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.
* subversion/libsvn_client/cmdline.c
(svn_client_args_to_target_array): Remove the peg revision preserving logic,
relying instead on the new peg support in svn_opt__arg_canonicalize_path()
and svn_opt__arg_canonicalize_url().
* subversion/tests/cmdline/basic_tests.py
(basic_relative_url_with_peg_revisions): New test functions.
(test_list): Call the new test function.
]]]
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/)
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Fri, May 23, 2008 at 5:40 AM, Julian Foad <ju...@btopenworld.com> wrote:
> Troy Curtis Jr wrote:
>>
>> On Thu, May 22, 2008 at 6:07 AM, Julian Foad <ju...@btopenworld.com>
>> wrote:
>
> [...]
>>>
>>> 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.
>>
>> That would certainly work, I was just trying to keep from having to
>> put another implementation in the relative url parsing block of the
>> code.
>
> I don't quite follow what you mean by this.
>
Sorry I was a bit terse wasn't I. When I set out to fix the peg
revision support for relative urls I realized I would need to
duplicate the peg revision code in the relative url resolving code
block that occurred directly after the main target processing loop.
My initial thought was something like I will be implementing now.
Simply encapsulating the peg revision splitting code.
But I thought that I could instead teach the *arg_canonicalize*()
functions (using a common arg splitting function) and so I would get
the peg revision support inside the relative url parsing block "for
free". Of course needing to check for the adm directory made my
solution a bit less optimal.
I decided to stick with it because at least there I was just doing the
extra rev parsing in one place. With the way I am going now (similar
to what I was initially thinking) I will have to split and concatenate
in two places. I felt that at that point they were basically equal,
and I had already implemented most of the patch that I subsequently
submitted.
Is that a better explanation of my thoughts? I just sort of banged
out that other email and clicked send.
Troy
>> I think that is an excellent compromise between my patch and
>> Karl's suggestions. I'll work up the patch and send it in when I get
>> a chance.
>
> - Julian
>
--
"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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository
root relative urls.
Posted by Julian Foad <ju...@btopenworld.com>.
Troy Curtis Jr wrote:
> On Thu, May 22, 2008 at 6:07 AM, Julian Foad <ju...@btopenworld.com> wrote:
[...]
>>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.
>
> That would certainly work, I was just trying to keep from having to
> put another implementation in the relative url parsing block of the
> code.
I don't quite follow what you mean by this.
> I think that is an excellent compromise between my patch and
> Karl's suggestions. I'll work up the patch and send it in when I get
> a chance.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository
root relative urls.
Posted by Folker Schamel <sc...@spinor.com>.
Troy Curtis Jr wrote:
> On Tue, May 27, 2008 at 10:39 AM, Karl Fogel <kf...@red-bean.com> wrote:
>> "Troy Curtis Jr" <tr...@gmail.com> writes:
>>> I interpret his question as this: Is it possible that the pool the
>>> returned array is in can be destroyed sometime after the dll
>>> containing the static string is unloaded, and that a caller might try
>>> to access the static string location after the dll is unloaded but
>>> before the pool is destroyed.
>>>
>>> I still think the answer is likely that it doesn't matter, but I don't
>>> know the details of the subversion dll loading/unloading procedures.
>> I don't understand this line of questioning at all, I have to admit.
>> What does the static string have to do with anything pool-related? Why
>> is the static string even coming up as an issue in the same sentence
>> with "pool"? :-) It's not allocated in the pool. It has nothing to do
>> with the pool. We could allocate and destroy pools all day long and it
>> would never affect that static string in the slightest...
>>
>
> Contrived illustration:
>
> - Caller loads dynamic library "libfoo"
> - Caller calls a function in "libfoo" which returns an string which
> the doc string (or his interpretation of it) claims is allocated in
> the given pool (but it turns out it is a pointer to a static string,
> as in our case)
> - Now that caller is done with the function it needed in "libfoo", so
> it unloads it (and presumably the static string memory location is
> unmapped. This may not be the case as I don't really know much about
> the way dynamic libraries work with static allocations)
> - Now the caller is ready to use the returned string, but that memory
> location is no longer mapped and you get a segfault (I think). It
> does this because it thinks everything is in the pool it provided, and
> it has not destroyed that pool yet, and so assumes the memory will
> still be valid.
Yes, this is exactly what I ment.
>
> Now I don't know too much about Subversion's use of it's libraries,
> but I highly doubt that the caller of these libraries will unload the
> libsvn_subr library and then try to process the returned string.
Very unlikely, right - but who really knows?
Especially in context of plugins of complex applications,
(which crash often enough)?
I mentioned it just because in my experience, users always do things
you never expect as API developer. And therefore I think that API
specifications should be 100% correct.
Never assume "no user will do that anyway...".
Folker
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
Folker Schamel <sc...@spinor.com> writes:
> Karl Fogel wrote:
>> We've been returning constant strings for about eight years now, and
>> we've never heard a bug report about it. Given that, I don't think we
>> should clutter up our API documentation by mentioning this concern.
>>
>> -Karl
>
> Wasn't meant offensive and I don't want to demand anything.
> I just noticed that aspect and wanted to mentioned it briefly
> in case you are not aware.
> Obviously you are aware of it and deliberately do it that way,
> so everything is fine.
> Sorry for the noise.
Huh -- oh, gosh, no need to apologize, Folker! You weren't offensive at
all. I just write replies very tersely because I write a lot of them;
please don't take the terseness as indicating anything else. Your post
was fine.
http://producingoss.com/en/communications.html#writing-tone
http://producingoss.com/en/communications.html#rudeness
:-)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository
root relative urls.
Posted by Folker Schamel <sc...@spinor.com>.
Karl Fogel wrote:
> Folker Schamel <sc...@spinor.com> writes:
>>> In any case, we're not going to go and add apr_pstrdup's around every
>>> single string constant in Subversion!
>> Its not about every constant string, its just about constant strings
>> directly returned by API functions.
>>
>> Alternatively, you might just add one global sentence to the API spec
>> saying basically that the lifetime of returned strings is not only
>> limited by pool lifetimes, but also by library lifetimes.
>
> We've been returning constant strings for about eight years now, and
> we've never heard a bug report about it. Given that, I don't think we
> should clutter up our API documentation by mentioning this concern.
>
> -Karl
Wasn't meant offensive and I don't want to demand anything.
I just noticed that aspect and wanted to mentioned it briefly
in case you are not aware.
Obviously you are aware of it and deliberately do it that way,
so everything is fine.
Sorry for the noise.
Folker
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
Folker Schamel <sc...@spinor.com> writes:
>> In any case, we're not going to go and add apr_pstrdup's around every
>> single string constant in Subversion!
>
> Its not about every constant string, its just about constant strings
> directly returned by API functions.
>
> Alternatively, you might just add one global sentence to the API spec
> saying basically that the lifetime of returned strings is not only
> limited by pool lifetimes, but also by library lifetimes.
We've been returning constant strings for about eight years now, and
we've never heard a bug report about it. Given that, I don't think we
should clutter up our API documentation by mentioning this concern.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository
root relative urls.
Posted by Folker Schamel <sc...@spinor.com>.
David Glasser wrote:
> On Tue, May 27, 2008 at 8:55 AM, Troy Curtis Jr <tr...@gmail.com> wrote:
>> On Tue, May 27, 2008 at 10:39 AM, Karl Fogel <kf...@red-bean.com> wrote:
>>> "Troy Curtis Jr" <tr...@gmail.com> writes:
>>>> I interpret his question as this: Is it possible that the pool the
>>>> returned array is in can be destroyed sometime after the dll
>>>> containing the static string is unloaded, and that a caller might try
>>>> to access the static string location after the dll is unloaded but
>>>> before the pool is destroyed.
>>>>
>>>> I still think the answer is likely that it doesn't matter, but I don't
>>>> know the details of the subversion dll loading/unloading procedures.
>>> I don't understand this line of questioning at all, I have to admit.
>>> What does the static string have to do with anything pool-related? Why
>>> is the static string even coming up as an issue in the same sentence
>>> with "pool"? :-) It's not allocated in the pool. It has nothing to do
>>> with the pool. We could allocate and destroy pools all day long and it
>>> would never affect that static string in the slightest...
>>>
>> Contrived illustration:
>>
>> - Caller loads dynamic library "libfoo"
>> - Caller calls a function in "libfoo" which returns an string which
>> the doc string (or his interpretation of it) claims is allocated in
>> the given pool (but it turns out it is a pointer to a static string,
>> as in our case)
>> - Now that caller is done with the function it needed in "libfoo", so
>> it unloads it (and presumably the static string memory location is
>> unmapped. This may not be the case as I don't really know much about
>> the way dynamic libraries work with static allocations)
>> - Now the caller is ready to use the returned string, but that memory
>> location is no longer mapped and you get a segfault (I think). It
>> does this because it thinks everything is in the pool it provided, and
>> it has not destroyed that pool yet, and so assumes the memory will
>> still be valid.
>>
>> Now I don't know too much about Subversion's use of it's libraries,
>> but I highly doubt that the caller of these libraries will unload the
>> libsvn_subr library and then try to process the returned string.
>>
>> All that said, I could be completely wrong on my interpretation of the
>> question, and on where the static locations are for DLLs.
>
> In practice, you just don't unload svn libraries.
There's a difference between "the API spec does not allow it"
and "it's allowed, but I don't think any user will do that".
As mentioned, in my experience, API users always do things you never expect.
>
> In any case, we're not going to go and add apr_pstrdup's around every
> single string constant in Subversion!
Its not about every constant string, its just about constant strings
directly returned by API functions.
Alternatively, you might just add one global sentence to the API spec
saying basically that the lifetime of returned strings is not only
limited by pool lifetimes, but also by library lifetimes.
Folker
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by David Glasser <gl...@davidglasser.net>.
On Tue, May 27, 2008 at 8:55 AM, Troy Curtis Jr <tr...@gmail.com> wrote:
> On Tue, May 27, 2008 at 10:39 AM, Karl Fogel <kf...@red-bean.com> wrote:
>> "Troy Curtis Jr" <tr...@gmail.com> writes:
>>> I interpret his question as this: Is it possible that the pool the
>>> returned array is in can be destroyed sometime after the dll
>>> containing the static string is unloaded, and that a caller might try
>>> to access the static string location after the dll is unloaded but
>>> before the pool is destroyed.
>>>
>>> I still think the answer is likely that it doesn't matter, but I don't
>>> know the details of the subversion dll loading/unloading procedures.
>>
>> I don't understand this line of questioning at all, I have to admit.
>> What does the static string have to do with anything pool-related? Why
>> is the static string even coming up as an issue in the same sentence
>> with "pool"? :-) It's not allocated in the pool. It has nothing to do
>> with the pool. We could allocate and destroy pools all day long and it
>> would never affect that static string in the slightest...
>>
>
> Contrived illustration:
>
> - Caller loads dynamic library "libfoo"
> - Caller calls a function in "libfoo" which returns an string which
> the doc string (or his interpretation of it) claims is allocated in
> the given pool (but it turns out it is a pointer to a static string,
> as in our case)
> - Now that caller is done with the function it needed in "libfoo", so
> it unloads it (and presumably the static string memory location is
> unmapped. This may not be the case as I don't really know much about
> the way dynamic libraries work with static allocations)
> - Now the caller is ready to use the returned string, but that memory
> location is no longer mapped and you get a segfault (I think). It
> does this because it thinks everything is in the pool it provided, and
> it has not destroyed that pool yet, and so assumes the memory will
> still be valid.
>
> Now I don't know too much about Subversion's use of it's libraries,
> but I highly doubt that the caller of these libraries will unload the
> libsvn_subr library and then try to process the returned string.
>
> All that said, I could be completely wrong on my interpretation of the
> question, and on where the static locations are for DLLs.
In practice, you just don't unload svn libraries.
In any case, we're not going to go and add apr_pstrdup's around every
single string constant in Subversion!
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Tue, May 27, 2008 at 10:39 AM, Karl Fogel <kf...@red-bean.com> wrote:
> "Troy Curtis Jr" <tr...@gmail.com> writes:
>> I interpret his question as this: Is it possible that the pool the
>> returned array is in can be destroyed sometime after the dll
>> containing the static string is unloaded, and that a caller might try
>> to access the static string location after the dll is unloaded but
>> before the pool is destroyed.
>>
>> I still think the answer is likely that it doesn't matter, but I don't
>> know the details of the subversion dll loading/unloading procedures.
>
> I don't understand this line of questioning at all, I have to admit.
> What does the static string have to do with anything pool-related? Why
> is the static string even coming up as an issue in the same sentence
> with "pool"? :-) It's not allocated in the pool. It has nothing to do
> with the pool. We could allocate and destroy pools all day long and it
> would never affect that static string in the slightest...
>
Contrived illustration:
- Caller loads dynamic library "libfoo"
- Caller calls a function in "libfoo" which returns an string which
the doc string (or his interpretation of it) claims is allocated in
the given pool (but it turns out it is a pointer to a static string,
as in our case)
- Now that caller is done with the function it needed in "libfoo", so
it unloads it (and presumably the static string memory location is
unmapped. This may not be the case as I don't really know much about
the way dynamic libraries work with static allocations)
- Now the caller is ready to use the returned string, but that memory
location is no longer mapped and you get a segfault (I think). It
does this because it thinks everything is in the pool it provided, and
it has not destroyed that pool yet, and so assumes the memory will
still be valid.
Now I don't know too much about Subversion's use of it's libraries,
but I highly doubt that the caller of these libraries will unload the
libsvn_subr library and then try to process the returned string.
All that said, I could be completely wrong on my interpretation of the
question, and on where the static locations are for DLLs.
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
"Troy Curtis Jr" <tr...@gmail.com> writes:
> I interpret his question as this: Is it possible that the pool the
> returned array is in can be destroyed sometime after the dll
> containing the static string is unloaded, and that a caller might try
> to access the static string location after the dll is unloaded but
> before the pool is destroyed.
>
> I still think the answer is likely that it doesn't matter, but I don't
> know the details of the subversion dll loading/unloading procedures.
I don't understand this line of questioning at all, I have to admit.
What does the static string have to do with anything pool-related? Why
is the static string even coming up as an issue in the same sentence
with "pool"? :-) It's not allocated in the pool. It has nothing to do
with the pool. We could allocate and destroy pools all day long and it
would never affect that static string in the slightest...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Tue, May 27, 2008 at 8:57 AM, Karl Fogel <kf...@red-bean.com> wrote:
> Folker Schamel <sc...@spinor.com> writes:
>>>>> 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?
>>>
>>> Since "" is a constant, it almost certainly will be compiled as a
>>> pointer to a fixed copy of the empty string in the program's text: no
>>> dynamic allocation or freeing involved at all.
>>
>> Is it possible that the DLL containing the code is unloaded
>> before the pool is destroyed?
>
> ?
>
> I'm not sure I understand this question, but to the extent that I do
> understand it, I think you might think something that isn't true.
>
> There is no "the pool" as far as that empty string is concerned. It's
> not allocated in a pool. Troy just meant that some other pool might be
> cleared or destroyed, and that this wouldn't affect the statically
> allocated (and probably shared) empty string.
>
> -Karl
>
>
I interpret his question as this: Is it possible that the pool the
returned array is in can be destroyed sometime after the dll
containing the static string is unloaded, and that a caller might try
to access the static string location after the dll is unloaded but
before the pool is destroyed.
I still think the answer is likely that it doesn't matter, but I don't
know the details of the subversion dll loading/unloading procedures.
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
Folker Schamel <sc...@spinor.com> writes:
>>>> 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?
>>
>> Since "" is a constant, it almost certainly will be compiled as a
>> pointer to a fixed copy of the empty string in the program's text: no
>> dynamic allocation or freeing involved at all.
>
> Is it possible that the DLL containing the code is unloaded
> before the pool is destroyed?
?
I'm not sure I understand this question, but to the extent that I do
understand it, I think you might think something that isn't true.
There is no "the pool" as far as that empty string is concerned. It's
not allocated in a pool. Troy just meant that some other pool might be
cleared or destroyed, and that this wouldn't affect the statically
allocated (and probably shared) empty string.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository
root relative urls.
Posted by Folker Schamel <sc...@spinor.com>.
David Glasser wrote:
> On Mon, May 26, 2008 at 6:53 PM, Troy Curtis Jr <tr...@gmail.com> wrote:
>> On Mon, May 26, 2008 at 4:22 PM, Karl Fogel <kf...@red-bean.com> wrote:
>>> "Troy Curtis Jr" <tr...@gmail.com> writes:
>>>> + *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?
>
> Since "" is a constant, it almost certainly will be compiled as a
> pointer to a fixed copy of the empty string in the program's text: no
> dynamic allocation or freeing involved at all.
Is it possible that the DLL containing the code is unloaded
before the pool is destroyed?
Folker
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by David Glasser <gl...@davidglasser.net>.
On Mon, May 26, 2008 at 6:53 PM, Troy Curtis Jr <tr...@gmail.com> wrote:
> On Mon, May 26, 2008 at 4:22 PM, Karl Fogel <kf...@red-bean.com> wrote:
>> "Troy Curtis Jr" <tr...@gmail.com> writes:
>>> + *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?
Since "" is a constant, it almost certainly will be compiled as a
pointer to a fixed copy of the empty string in the program's text: no
dynamic allocation or freeing involved at all.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
"Troy Curtis Jr" <tr...@gmail.com> writes:
>> 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?
That's correct. No one's going to be calling free() on anything, so
it's safe to return pointers to static strings (though not when they're
going to be modified in place, of course).
> 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?
Nope, you can just close it as FIXED.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Mon, May 26, 2008 at 4:22 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "Troy Curtis Jr" <tr...@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 ".@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 ".@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
"Troy Curtis Jr" <tr...@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 ".@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 ".@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
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.
]]]
--
"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/)
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Fri, May 23, 2008 at 12:15 PM, Karl Fogel <kf...@red-bean.com> wrote:
> Troy, FWIW, I think Julian's approach is best too.
>
> -Karl
>
There's big discussion right now at work about peer reviewing and how
we might integrate it in our work flow. There is lots of resistance.
But my experience with this relative url patch has really showed me
the benefit of doing them. And that's exactly what all this has been,
discussion of several implementations and the best one emerges!
Troy
> "Troy Curtis Jr" <tr...@gmail.com> writes:
>>> 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.
>>
>> That would certainly work, I was just trying to keep from having to
>> put another implementation in the relative url parsing block of the
>> code. I think that is an excellent compromise between my patch and
>> Karl's suggestions. I'll work up the patch and send it in when I get
>> a chance.
>
--
"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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
Troy, FWIW, I think Julian's approach is best too.
-Karl
"Troy Curtis Jr" <tr...@gmail.com> writes:
>> 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.
>
> That would certainly work, I was just trying to keep from having to
> put another implementation in the relative url parsing block of the
> code. I think that is an excellent compromise between my patch and
> Karl's suggestions. I'll work up the patch and send it in when I get
> a chance.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Thu, May 22, 2008 at 6:07 AM, Julian Foad <ju...@btopenworld.com> wrote:
> 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
>
>
That would certainly work, I was just trying to keep from having to
put another implementation in the relative url parsing block of the
code. I think that is an excellent compromise between my patch and
Karl's suggestions. I'll work up the patch and send it in when I get
a chance.
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository
root relative urls.
Posted by Julian Foad <ju...@btopenworld.com>.
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
>>> 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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
"Troy Curtis Jr" <tr...@gmail.com> writes:
>> Oh, I see: you're having the caller be responsible for detecting an
>> escaping '@'. That could work, but it would be a cleaner interface to
>> just have split_arg_at_peg_revision() take care of this, which is what
>> split_arg_at_peg_revision()'s doc string currently claims anyway.
>
> Well the doc string does explicity state that the '@' character is
> left in the peg revision parameter as the first character.
Yes, but that's only if there actually is a peg revision. If there is
no peg revision, then *peg_rev is supposed to be NULL. (IOW, assume
that your doc string is going to be taken absolutely literally. A "peg
revision" is a semantic thing -- it's not "anything, including the empty
string, following an @-sign at the end of a path").
>> "delimiters" :-)
>
> In fairness to this Texas boy, this is not a comment I wrote, it just looks
> like an 'add' because of the way the diff worked out. (See a couple of lines
> down)
*nod* Yup, sorry.
>> This assignment-in-condition construction got me a warning:
>>
>> subversion/libsvn_subr/opt.c: In function svn_opt_args_to_target_array3:
>> subversion/libsvn_subr/opt.c:1010: warning: suggest parentheses
>> around assignment used as truth value
>
> I have trouble seeing any warnings in the extremely verbose Subversion output,
> if it could build more like, say, the Linux kernel it'd be much easier :).
> Sure I could use 'make -s', but then it looks like it is hung! I like the
> warm fuzzy of status text scrolling by, but not TOO much.
Sure, if you're trying to detect them with unassisted human eyes,
they're hard to see. But if you build inside an IDE or some other of
environment that detects the "warning:" and "error:" lines for you, then
it's no problem. Or if you just manually run a search tool like grep
over the output, it's no problem either.
There are many build environments available. The one I happen to use is
Emacs's compilation mode, but that's a matter of taste. The main thing
is that you needn't (and shouldn't) depend on vgrep to find warnings
(ahem, http://en.wikipedia.org/wiki/Vgrep#Humorous_terminology).
>> 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
> just 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.
> 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.
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.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Mon, May 12, 2008 at 4:52 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "Troy Curtis Jr" <tr...@gmail.com> writes:
>> [[[
>> 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() and svn_opt_args_to_target_array3() into
>> the common functions svn_opt__arg_canonicalize_url() and
>> svn_opt__arg_canonicalize_path().
>>
>> * subversion/libsvn_subr/opt.c
>> (split_arg_at_peg_revision): New function.
>> (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().
>> (svn_opt_parse_path): Replace in-line peg revision parsing with the new
>> split_arg_at_peg_revision() function.
>> (svn_opt_args_to_target_array3): Remove the peg revision preserving logic,
>> relying instead on the new peg revision support in
>> svn_opt__arg_canonicalize_path() and svn_opt__arg_canonicalize_url().
>>
>> * 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.
>>
>> * subversion/libsvn_client/cmdline.c
>> (svn_client_args_to_target_array): Remove the peg revision
preserving logic,
>> relying instead on the new peg support in svn_opt__arg_canonicalize_path()
>> and svn_opt__arg_canonicalize_url().
>>
>> * subversion/tests/cmdline/basic_tests.py
>> (basic_relative_url_with_peg_revisions): New test functions.
>> (test_list): Call the new test function.
>> ]]]
>
> There's only one test function, so:
>
> * subversion/tests/cmdline/basic_tests.py
> (basic_relative_url_with_peg_revisions): New test.
> (test_list): Run it.
>
> :-) On to the code...
Copy-n-paste is a blessing and a curse I tell ya.
>
>> --- subversion/libsvn_subr/opt.c (revision 31120)
>> +++ subversion/libsvn_subr/opt.c (working copy)
>> @@ -803,81 +803,123 @@
>> return SVN_NO_ERROR;
>> }
>>
>> +/* 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 set to NULL if no peg revision is found.
>> + *
>> + * UTF8_TARGET need not be canonical. *TRUE_TARGET will not be canonical
>> + * unless UTF8_TARGET is.
>> + *
>> + * *PEG_REVISION does still contain the '@' symbol as the first character.
>> + *
>> + * 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)
>
> Wow, I can't believe we didn't have something like this before :-).
>
>> +{
>> + 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,
>> + peg_start - utf8_target);
>> +
>> + *peg_revision = apr_pstrdup(pool, peg_start);
>> + }
>> + else
>> + {
>> + *true_target = utf8_target;
>> +
>> + *peg_revision = NULL;
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
>
> Hmmm. The way we quote an '@'-sign in a path is by putting a blank
> peg rev at the end, for example:
>
> /path/to/a/file/with/a/real/@sign
>
> would be escaped like this:
>
> /path/to/a/file/with/a/real/@sign@
>
> In that case, *peg_revision would be a specially-allocated "@" in pool,
> but shouldn't it just be NULL instead?
>
>> svn_error_t *
>> svn_opt_parse_path(svn_opt_revision_t *rev,
>> const char **truepath,
>> 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(split_arg_at_peg_revision(truepath, &peg_rev, path, pool));
>> +
>> + /* Parse the peg revision, if one was found */
>> + if (peg_rev)
>> {
>> - /* If we hit a path separator, stop looking. */
>> - /* This is OK only because our revision specifiers can't
contain '/'. */
>> - if (path[i] == '/')
>> - break;
>> + int ret;
>> + svn_opt_revision_t start_revision, end_revision;
>>
>> - if (path[i] == '@')
>> + end_revision.kind = svn_opt_revision_unspecified;
>> +
>> + if (peg_rev[1] == '\0') /* looking at empty peg revision */
>> {
>
> Oh, I see: you're having the caller be responsible for detecting an
> escaping '@'. That could work, but it would be a cleaner interface to
> just have split_arg_at_peg_revision() take care of this, which is what
> split_arg_at_peg_revision()'s doc string currently claims anyway.
>
Well the doc string does explicity state that the '@' character is left in the
peg revision parameter as the first character.
>
>> - int ret;
>> - svn_opt_revision_t start_revision, end_revision;
>> + ret = 0;
>> + start_revision.kind = svn_opt_revision_unspecified;
>> + }
>> + else /* looking at non-empty peg revision */
>> + {
>> + const char *rev_str = peg_rev + 1;
>>
>> - end_revision.kind = svn_opt_revision_unspecified;
>> -
>> - if (path[i + 1] == '\0') /* looking at empty peg revision */
>> + /* URLs get treated differently from wc paths. */
>> + if (svn_path_is_url(path))
>> {
>> - ret = 0;
>> - start_revision.kind = svn_opt_revision_unspecified;
>> - }
>> - else /* looking at non-empty peg revision */
>> - {
>> - const char *rev_str = path + i + 1;
>> -
>> - /* URLs get treated differently from wc paths. */
>> - if (svn_path_is_url(path))
>> + /* URLs are URI-encoded, so we look for dates with
>> + URI-encoded delimeters. */
>
> "delimiters" :-)
In fairness to this Texas boy, this is not a comment I wrote, it just looks
like an 'add' because of the way the diff worked out. (See a couple of lines
down)
>
>> + int rev_len = strlen(rev_str);
>> + if (rev_len > 6
>> + && rev_str[0] == '%'
>> + && rev_str[1] == '7'
>> + && (rev_str[2] == 'B'
>> + || rev_str[2] == 'b')
>> + && rev_str[rev_len-3] == '%'
>> + && rev_str[rev_len-2] == '7'
>> + && (rev_str[rev_len-1] == 'D'
>> + || rev_str[rev_len-1] == 'd'))
>> {
>> - /* URLs are URI-encoded, so we look for dates with
>> - URI-encoded delimeters. */
>> - int rev_len = strlen(rev_str);
>> - if (rev_len > 6
>> - && rev_str[0] == '%'
>> - && rev_str[1] == '7'
>> - && (rev_str[2] == 'B'
>> - || rev_str[2] == 'b')
>> - && rev_str[rev_len-3] == '%'
>> - && rev_str[rev_len-2] == '7'
>> - && (rev_str[rev_len-1] == 'D'
>> - || rev_str[rev_len-1] == 'd'))
>> - {
>> - rev_str = svn_path_uri_decode(rev_str, pool);
>> - }
>> + rev_str = svn_path_uri_decode(rev_str, pool);
>> }
>> - ret = svn_opt_parse_revision(&start_revision,
>> - &end_revision,
>> - rev_str, pool);
>> }
>> + ret = svn_opt_parse_revision(&start_revision,
>> + &end_revision,
>> + rev_str, pool);
>> + }
>>
>> - if (ret || end_revision.kind != svn_opt_revision_unspecified)
>> - return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> - _("Syntax error parsing
revision '%s'"),
>> - path + i + 1);
>> + if (ret || end_revision.kind != svn_opt_revision_unspecified)
>> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> + _("Syntax error parsing revision '%s'"),
>> + peg_rev + 1);
>>
>> - *truepath = apr_pstrmemdup(pool, path, i);
>> - rev->kind = start_revision.kind;
>> - rev->value = start_revision.value;
>> -
>> - return SVN_NO_ERROR;
>> - }
>> + rev->kind = start_revision.kind;
>> + rev->value = start_revision.value;
>> }
>> + else
>> + {
>> + /* Didn't find an peg revision. */
>
> "a"
>
>> @@ -987,13 +997,19 @@
>> }
>> else /* not a url, so treat as a path */
>> {
>> - const char *base_name;
>> + char *base_name;
>> + char *peg_rev;
>>
>> SVN_ERR(svn_opt__arg_canonicalize_path(&target,
utf8_target, pool));
>>
>> /* If the target has the same name as a Subversion
>> working copy administrative dir, skip it. */
>> base_name = svn_path_basename(target, pool);
>> +
>> + /* "Delete" the peg revision */
>> + if (peg_rev = strrchr(base_name, '@'))
>> + *peg_rev = '\0';
>
> This assignment-in-condition construction got me a warning:
>
> subversion/libsvn_subr/opt.c: In function svn_opt_args_to_target_array3:
> subversion/libsvn_subr/opt.c:1010: warning: suggest parentheses
> around assignment used as truth value
I have trouble seeing any warnings in the extremely verbose Subversion output,
if it could build more like, say, the Linux kernel it'd be much easier :).
Sure I could use 'make -s', but then it looks like it is hung! I like the
warm fuzzy of status text scrolling by, but not TOO much.
>
> I guess we've asked GCC to play police here, so we should move the
> assignment to a separate line.
>
> But more importantly, I think there's an opportunity to remove some code
> duplication here.
>
> We have split_arg_at_peg_revision(), which turns a path-maybe-with-peg
> into a path and a peg (NULL if no peg). Its only callers are
>
> svn_opt_parse_path()
> svn_opt__arg_canonicalize_url()
> svn_opt__arg_canonicalize_path()
>
> The first of those returns the peg revision separately (as a struct);
> the latter two instead reassemble target+peg_rev, just in canonical
> form. But now, every caller of svn_opt__arg_canonicalize_path() further
> converts the target to a base_name and then does
>
> strrchr(base_name, '@')
>
> to find and hide a peg rev that we've already found once before.
>
> 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 just 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.
- 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.
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.
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.
In the end I could not find an alternative solution that I liked better and
figured the patch already reduced duplicate code by a good measure that
leaving this logic in place would be fine.
>
>> @@ -1114,14 +1125,30 @@
>> return SVN_NO_ERROR;
>> }
>>
>> +
>> 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;
>>
>> + /*
>> + * This is needed so that url_in can be properly canonicalized,
>> + * otherwise the canonicalization does not treat a ".@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(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 +1167,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);
>
> Yeah, the fact that the above huge comment and code...
>
>> *url_out = target;
>> return SVN_NO_ERROR;
>> }
>> @@ -1151,9 +1183,24 @@
>> const char *apr_target;
>> char *truenamed_target; /* APR-encoded */
>> apr_status_t apr_err;
>> + const char *peg_rev;
>>
>> + /*
>> + * This is needed so that url_in can be properly canonicalized,
>> + * otherwise the canonicalization does not treat a ".@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(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 +1221,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);
>> +
>> return SVN_NO_ERROR;
>> }
>
> ...are duplicated here signifies an opportunity, I think.
>
I really debated on where to move this comment. It was initially in the
args_to_target_array* functions. It commented the code that I consolidated
into split_arg_at_peg_revision() but I didn't think it really fit with that
function. It seemed to go in the usage part. It seemed to comment on why I
am using split_arg_at_peg_revision() instead of svn_opt_parse_path(), and not
so much why split_arg_at_peg_revision() exists.
Well the apr_pstrcat() duplication would simply be moved into the
*args_to_target_array() functions. I suppose it would get rid of the
"strrchr(base_name, '@')" code, but I don't know that it is worth it. I know
that trying to leave the peg revision seperated out and sticking into some
kind of array of target struct that svn_client_args_to_target_array() returns
is DEFINITELY not something I wanted to mess with. The assumption that an
array of targets as strings, possibly with peg revs, runs deep. (I chased it
down during my initial repository root relative url patch).
So in the end I could remove
+ if (peg_rev = strrchr(base_name, '@'))
+ *peg_rev = '\0';
and instead put in
+ if (peg_rev)
+ *path_out = apr_pstrcat(pool, *path_out, peg_rev, NULL);
instead. In both cases the caller has to be "peg aware", though not "peg
syntax aware" in the second case. This also at the cost of overloading the
meaning of svn_opt__arg_canonicalize_{url,path}(), or changing their names
entirely.
>
>> --- subversion/libsvn_client/cmdline.c (revision 31120)
>> +++ subversion/libsvn_client/cmdline.c (working copy)
>> @@ -247,7 +216,8 @@
>> }
>> else /* not a url, so treat as a path */
>> {
>> - const char *base_name;
>> + char *base_name;
>> + char *peg_rev;
>>
>> SVN_ERR(svn_opt__arg_canonicalize_path(&target,
>> utf8_target, pool));
>> @@ -256,6 +226,12 @@
>> working copy administrative dir, skip it. */
>> base_name = svn_path_basename(target, pool);
>>
>> + /* "Delete" the peg revision because
svn_wc_is_adm_dir() cannot
>> + * handle it.
>> + */
>> + if (peg_rev = strrchr(base_name, '@'))
>> + *peg_rev = '\0';
>> +
>> if (svn_wc_is_adm_dir(base_name, pool))
>> {
>> err =
svn_error_createf(SVN_ERR_RESERVED_FILENAME_SPECIFIED,
>
> And this duplicates code we saw earlier in svn_opt_args_to_target_array3().
>
> Thoughts?
>
> -Karl
>
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
"Troy Curtis Jr" <tr...@gmail.com> writes:
> [[[
> 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() and svn_opt_args_to_target_array3() into
> the common functions svn_opt__arg_canonicalize_url() and
> svn_opt__arg_canonicalize_path().
>
> * subversion/libsvn_subr/opt.c
> (split_arg_at_peg_revision): New function.
> (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().
> (svn_opt_parse_path): Replace in-line peg revision parsing with the new
> split_arg_at_peg_revision() function.
> (svn_opt_args_to_target_array3): Remove the peg revision preserving logic,
> relying instead on the new peg revision support in
> svn_opt__arg_canonicalize_path() and svn_opt__arg_canonicalize_url().
>
> * 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.
>
> * subversion/libsvn_client/cmdline.c
> (svn_client_args_to_target_array): Remove the peg revision preserving logic,
> relying instead on the new peg support in svn_opt__arg_canonicalize_path()
> and svn_opt__arg_canonicalize_url().
>
> * subversion/tests/cmdline/basic_tests.py
> (basic_relative_url_with_peg_revisions): New test functions.
> (test_list): Call the new test function.
> ]]]
There's only one test function, so:
* subversion/tests/cmdline/basic_tests.py
(basic_relative_url_with_peg_revisions): New test.
(test_list): Run it.
:-) On to the code...
> --- subversion/libsvn_subr/opt.c (revision 31120)
> +++ subversion/libsvn_subr/opt.c (working copy)
> @@ -803,81 +803,123 @@
> return SVN_NO_ERROR;
> }
>
> +/* 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 set to NULL if no peg revision is found.
> + *
> + * UTF8_TARGET need not be canonical. *TRUE_TARGET will not be canonical
> + * unless UTF8_TARGET is.
> + *
> + * *PEG_REVISION does still contain the '@' symbol as the first character.
> + *
> + * 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)
Wow, I can't believe we didn't have something like this before :-).
> +{
> + 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,
> + peg_start - utf8_target);
> +
> + *peg_revision = apr_pstrdup(pool, peg_start);
> + }
> + else
> + {
> + *true_target = utf8_target;
> +
> + *peg_revision = NULL;
> + }
> +
> + return SVN_NO_ERROR;
> +}
Hmmm. The way we quote an '@'-sign in a path is by putting a blank
peg rev at the end, for example:
/path/to/a/file/with/a/real/@sign
would be escaped like this:
/path/to/a/file/with/a/real/@sign@
In that case, *peg_revision would be a specially-allocated "@" in pool,
but shouldn't it just be NULL instead?
> svn_error_t *
> svn_opt_parse_path(svn_opt_revision_t *rev,
> const char **truepath,
> 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(split_arg_at_peg_revision(truepath, &peg_rev, path, pool));
> +
> + /* Parse the peg revision, if one was found */
> + if (peg_rev)
> {
> - /* If we hit a path separator, stop looking. */
> - /* This is OK only because our revision specifiers can't contain '/'. */
> - if (path[i] == '/')
> - break;
> + int ret;
> + svn_opt_revision_t start_revision, end_revision;
>
> - if (path[i] == '@')
> + end_revision.kind = svn_opt_revision_unspecified;
> +
> + if (peg_rev[1] == '\0') /* looking at empty peg revision */
> {
Oh, I see: you're having the caller be responsible for detecting an
escaping '@'. That could work, but it would be a cleaner interface to
just have split_arg_at_peg_revision() take care of this, which is what
split_arg_at_peg_revision()'s doc string currently claims anyway.
> - int ret;
> - svn_opt_revision_t start_revision, end_revision;
> + ret = 0;
> + start_revision.kind = svn_opt_revision_unspecified;
> + }
> + else /* looking at non-empty peg revision */
> + {
> + const char *rev_str = peg_rev + 1;
>
> - end_revision.kind = svn_opt_revision_unspecified;
> -
> - if (path[i + 1] == '\0') /* looking at empty peg revision */
> + /* URLs get treated differently from wc paths. */
> + if (svn_path_is_url(path))
> {
> - ret = 0;
> - start_revision.kind = svn_opt_revision_unspecified;
> - }
> - else /* looking at non-empty peg revision */
> - {
> - const char *rev_str = path + i + 1;
> -
> - /* URLs get treated differently from wc paths. */
> - if (svn_path_is_url(path))
> + /* URLs are URI-encoded, so we look for dates with
> + URI-encoded delimeters. */
"delimiters" :-)
> + int rev_len = strlen(rev_str);
> + if (rev_len > 6
> + && rev_str[0] == '%'
> + && rev_str[1] == '7'
> + && (rev_str[2] == 'B'
> + || rev_str[2] == 'b')
> + && rev_str[rev_len-3] == '%'
> + && rev_str[rev_len-2] == '7'
> + && (rev_str[rev_len-1] == 'D'
> + || rev_str[rev_len-1] == 'd'))
> {
> - /* URLs are URI-encoded, so we look for dates with
> - URI-encoded delimeters. */
> - int rev_len = strlen(rev_str);
> - if (rev_len > 6
> - && rev_str[0] == '%'
> - && rev_str[1] == '7'
> - && (rev_str[2] == 'B'
> - || rev_str[2] == 'b')
> - && rev_str[rev_len-3] == '%'
> - && rev_str[rev_len-2] == '7'
> - && (rev_str[rev_len-1] == 'D'
> - || rev_str[rev_len-1] == 'd'))
> - {
> - rev_str = svn_path_uri_decode(rev_str, pool);
> - }
> + rev_str = svn_path_uri_decode(rev_str, pool);
> }
> - ret = svn_opt_parse_revision(&start_revision,
> - &end_revision,
> - rev_str, pool);
> }
> + ret = svn_opt_parse_revision(&start_revision,
> + &end_revision,
> + rev_str, pool);
> + }
>
> - if (ret || end_revision.kind != svn_opt_revision_unspecified)
> - return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> - _("Syntax error parsing revision '%s'"),
> - path + i + 1);
> + if (ret || end_revision.kind != svn_opt_revision_unspecified)
> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Syntax error parsing revision '%s'"),
> + peg_rev + 1);
>
> - *truepath = apr_pstrmemdup(pool, path, i);
> - rev->kind = start_revision.kind;
> - rev->value = start_revision.value;
> -
> - return SVN_NO_ERROR;
> - }
> + rev->kind = start_revision.kind;
> + rev->value = start_revision.value;
> }
> + else
> + {
> + /* Didn't find an peg revision. */
"a"
> @@ -987,13 +997,19 @@
> }
> else /* not a url, so treat as a path */
> {
> - const char *base_name;
> + char *base_name;
> + char *peg_rev;
>
> SVN_ERR(svn_opt__arg_canonicalize_path(&target, utf8_target, pool));
>
> /* If the target has the same name as a Subversion
> working copy administrative dir, skip it. */
> base_name = svn_path_basename(target, pool);
> +
> + /* "Delete" the peg revision */
> + if (peg_rev = strrchr(base_name, '@'))
> + *peg_rev = '\0';
This assignment-in-condition construction got me a warning:
subversion/libsvn_subr/opt.c: In function ‘svn_opt_args_to_target_array3’:
subversion/libsvn_subr/opt.c:1010: warning: suggest parentheses
around assignment used as truth value
I guess we've asked GCC to play police here, so we should move the
assignment to a separate line.
But more importantly, I think there's an opportunity to remove some code
duplication here.
We have split_arg_at_peg_revision(), which turns a path-maybe-with-peg
into a path and a peg (NULL if no peg). Its only callers are
svn_opt_parse_path()
svn_opt__arg_canonicalize_url()
svn_opt__arg_canonicalize_path()
The first of those returns the peg revision separately (as a struct);
the latter two instead reassemble target+peg_rev, just in canonical
form. But now, every caller of svn_opt__arg_canonicalize_path() further
converts the target to a base_name and then does
strrchr(base_name, '@')
to find and hide a peg rev that we've already found once before.
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.
> @@ -1114,14 +1125,30 @@
> return SVN_NO_ERROR;
> }
>
> +
> 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;
>
> + /*
> + * This is needed so that url_in can be properly canonicalized,
> + * otherwise the canonicalization does not treat a ".@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(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 +1167,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);
Yeah, the fact that the above huge comment and code...
> *url_out = target;
> return SVN_NO_ERROR;
> }
> @@ -1151,9 +1183,24 @@
> const char *apr_target;
> char *truenamed_target; /* APR-encoded */
> apr_status_t apr_err;
> + const char *peg_rev;
>
> + /*
> + * This is needed so that url_in can be properly canonicalized,
> + * otherwise the canonicalization does not treat a ".@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(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 +1221,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);
> +
> return SVN_NO_ERROR;
> }
...are duplicated here signifies an opportunity, I think.
> --- subversion/libsvn_client/cmdline.c (revision 31120)
> +++ subversion/libsvn_client/cmdline.c (working copy)
> @@ -247,7 +216,8 @@
> }
> else /* not a url, so treat as a path */
> {
> - const char *base_name;
> + char *base_name;
> + char *peg_rev;
>
> SVN_ERR(svn_opt__arg_canonicalize_path(&target,
> utf8_target, pool));
> @@ -256,6 +226,12 @@
> working copy administrative dir, skip it. */
> base_name = svn_path_basename(target, pool);
>
> + /* "Delete" the peg revision because svn_wc_is_adm_dir() cannot
> + * handle it.
> + */
> + if (peg_rev = strrchr(base_name, '@'))
> + *peg_rev = '\0';
> +
> if (svn_wc_is_adm_dir(base_name, pool))
> {
> err = svn_error_createf(SVN_ERR_RESERVED_FILENAME_SPECIFIED,
And this duplicates code we saw earlier in svn_opt_args_to_target_array3().
Thoughts?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Karl Fogel <kf...@red-bean.com>.
Stefan Sperling <st...@elego.de> writes:
> > + if (utf8_target[j] == '@')
> > + {
> > + peg_start = utf8_target + j;
>
> Stylistic nit:
>
> With casting added, this becomes:
>
> peg_start = utf8_target + (const char *)j;
>
> This made me scratch my head at first, I hadn't seen pointers
> and ints being mixed like that in a while.
>
> I bet there's tons of code out there that does this, but
> I myself would tend to use an apr_size_t variable initialised
> to strlen(utf8_target), and decrement that in the loop instead of
> adding pointers and ints. That is, I'd try not to rely on automatic
> type conversion too much. See below for an example.
I'd still like to do a real review of the patch, but just regarding
this: FWIW, I think we use pointer arithmetic elsewhere in Subversion,
and I certainly don't mind. It's a completely reliable feature of C, I
think it makes the code more readable rather than less... I don't really
feel like it's "type mixing". Instead, It's just a way of writing an
array reference so that you get the address rather than the value.
Totally your call, of course, Troy.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository
root relative urls.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Troy Curtis Jr wrote on Mon, 12 May 2008 at 21:56 -0500:
> On Mon, May 12, 2008 at 11:29 AM, Daniel Shahaf <d....@daniel.shahaf.co.il> wrote:
> > Troy Curtis Jr wrote on Mon, 12 May 2008 at 06:49 -0500:
> >> I'm still learning about all this 'canonical' stuff, but if a
> >> zero-length path is considered canonical then should this function
> >> support that? Will apr_pstrmemdup() not output an empty path?
> >>
> >
> > Except the paths passed to this function are not canonical; they are user
> > input. If we allow the empty string here, wouldn't it allow
> >
> > svn up ''
> >
> > as synonym for
> >
> > svn up '.'
> >
>
> Makes sense to me, especially because I think the '.' is typically
> canonicalized to '' anyway, right? (Again don't be too hard on me,
> I'm still learning all this canonical stuff).
>
Right; but I don't think we should support '' as a synonym for '.'
in the UI :)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Mon, May 12, 2008 at 11:29 AM, Daniel Shahaf <d....@daniel.shahaf.co.il> wrote:
> Troy Curtis Jr wrote on Mon, 12 May 2008 at 06:49 -0500:
>> On Mon, May 12, 2008 at 5:52 AM, Stefan Sperling <st...@elego.de> wrote:
>> >> + if (utf8_target[j] == '@')
>> >> + {
>> >> + peg_start = utf8_target + j;
>> >
>> > Stylistic nit:
>> >
>> > With casting added, this becomes:
>> >
>> > peg_start = utf8_target + (const char *)j;
>> >
>> > This made me scratch my head at first, I hadn't seen pointers
>> > and ints being mixed like that in a while.
>> >
>> > I bet there's tons of code out there that does this, but
>>
>> In fact I consolidated code that did this in three different places,
>> into one. I tend not to use pointer arithmetic in my own newly
>> generated code because it does seem kind of confusing to me too!
>>
>> > I myself would tend to use an apr_size_t variable initialised
>> > to strlen(utf8_target), and decrement that in the loop instead of
>> > adding pointers and ints. That is, I'd try not to rely on automatic
>> > type conversion too much. See below for an example.
>> >
>
> I'd just do s/utf8_target + j/&utf8_target[j]/ and move on :)
>
Where's the fun in that? Too easy, something must be wrong with it!
>> > Note also how the exit condition of the loop ("j >= 0") also depends on
>> > j being a signed type (int), even though we're actually trying to
>> > determine an apr_size_t (which is unsigned).
>> >
>>
>> Though admittedly my reasons for not liking pointer arithmetic aren't
>> so methodical. It just makes me think too hard :).
>>
>> >> + break;
>> >> + }
>> >> + }
>> >> +
>> >> + if (peg_start)
>> >> + {
>> >> + *true_target = apr_pstrmemdup(pool,
>> >> + utf8_target,
>> >> + peg_start - utf8_target);
>> >
>> > Again, why not do an explicit cast if we're mixing types in
>> > expressions like that? For example:
>> >
>> > *true_target = apr_pstrmemdup(pool, utf8_target,
>> > (apr_size_t)(peg_start - utf8_target));
>> >
>> >
>> > Let me try to sketch out how we could do this without adding
>> > together variables of 3 different types (const char*, int, apr_size_t).
>> >
>> > 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)
>> > {
>> > const char *peg_start = NULL; /* pointer to the peg revision, if any */
>> > /* XXX: delete this comment before commit
>> > * Here's the only place where we rely on automatic type conversion
>> > * (size_t -> apr_size_t): */
>> > apr_size_t target_len = strlen(utf8_target);
>> > apr_size_t j;
>> >
>> > for (j = target_len; 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 - 1] == '/')
>> > break;
>> >
>> > /* If we find a peg revision, remember where we found it,
>> > * and calculate the actual length of the target URL. */
>> > if (utf8_target[j - 1] == '@')
>> > {
>> > peg_start = &(utf8_target[j - 1]);
>> >
>> > /* XXX: Delete this comment before commit, I just added
>> > * it to convince myself that the code is correct.
>> > * There is no off-by-one here, since:
>> > * [@|\0] --> target_len = 1 - 1 == 0
>> > * [h|@|\0] --> target_len = 2 - 1 == 1
>> > * [h|t|@|\0] --> target_len = 3 - 1 == 2
>> > * [h|t|t|@|\0] --> target_len = 4 - 1 == 3
>> > * [h|t|t|p|@|\0] --> target_len = 5 - 1 == 4
>> > * ...
>> > */
>
> I think of it this way: The length of a '\0'-terminated string is the
> index of the '\0', therefore the length of a '@'-terminated string is
> the index of the '@'.
>
>> > target_len = j - 1;
>> >
>> > break;
>> > }
>> > }
>>
>> I like it, though i'll probably do j = target_len -1 in the
>> initialization as you must do [j-1] at every usage point, and the
>> [j-1] is needed because here you are translating a length "j" into an
>> index "j-1". I tend to think of "j" as always meaning an index.
>>
>> >
>> > /* XXX: delete this comment before commit
>> > *
>> > * Note how we previously didn't handle target_len == 0 here!
>> > * But we should probably throw an error or something,
>> > * in this case, right? This case didn't even occur to me while
>> > * thinking about the problem in terms of pointer arithmetic,
>> > * but now it's rather obvious. We might also want to extend the
>> > * function's docstring to mention that it will raise an error
>> > * if it cannot split off a non-zero length target from the URL.
>> > * Note that zero-length paths are considered canonical.
>> > *
>> > * Pseudo code example: */
>>
>> I'm still learning about all this 'canonical' stuff, but if a
>> zero-length path is considered canonical then should this function
>> support that? Will apr_pstrmemdup() not output an empty path?
>>
>
> Except the paths passed to this function are not canonical; they are user
> input. If we allow the empty string here, wouldn't it allow
>
> svn up ''
>
> as synonym for
>
> svn up '.'
>
Makes sense to me, especially because I think the '.' is typically
canonicalized to '' anyway, right? (Again don't be too hard on me,
I'm still learning all this canonical stuff).
> ?
>
> Daniel
>
>
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository
root relative urls.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Troy Curtis Jr wrote on Mon, 12 May 2008 at 06:49 -0500:
> On Mon, May 12, 2008 at 5:52 AM, Stefan Sperling <st...@elego.de> wrote:
> >> + if (utf8_target[j] == '@')
> >> + {
> >> + peg_start = utf8_target + j;
> >
> > Stylistic nit:
> >
> > With casting added, this becomes:
> >
> > peg_start = utf8_target + (const char *)j;
> >
> > This made me scratch my head at first, I hadn't seen pointers
> > and ints being mixed like that in a while.
> >
> > I bet there's tons of code out there that does this, but
>
> In fact I consolidated code that did this in three different places,
> into one. I tend not to use pointer arithmetic in my own newly
> generated code because it does seem kind of confusing to me too!
>
> > I myself would tend to use an apr_size_t variable initialised
> > to strlen(utf8_target), and decrement that in the loop instead of
> > adding pointers and ints. That is, I'd try not to rely on automatic
> > type conversion too much. See below for an example.
> >
I'd just do s/utf8_target + j/&utf8_target[j]/ and move on :)
> > Note also how the exit condition of the loop ("j >= 0") also depends on
> > j being a signed type (int), even though we're actually trying to
> > determine an apr_size_t (which is unsigned).
> >
>
> Though admittedly my reasons for not liking pointer arithmetic aren't
> so methodical. It just makes me think too hard :).
>
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if (peg_start)
> >> + {
> >> + *true_target = apr_pstrmemdup(pool,
> >> + utf8_target,
> >> + peg_start - utf8_target);
> >
> > Again, why not do an explicit cast if we're mixing types in
> > expressions like that? For example:
> >
> > *true_target = apr_pstrmemdup(pool, utf8_target,
> > (apr_size_t)(peg_start - utf8_target));
> >
> >
> > Let me try to sketch out how we could do this without adding
> > together variables of 3 different types (const char*, int, apr_size_t).
> >
> > 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)
> > {
> > const char *peg_start = NULL; /* pointer to the peg revision, if any */
> > /* XXX: delete this comment before commit
> > * Here's the only place where we rely on automatic type conversion
> > * (size_t -> apr_size_t): */
> > apr_size_t target_len = strlen(utf8_target);
> > apr_size_t j;
> >
> > for (j = target_len; 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 - 1] == '/')
> > break;
> >
> > /* If we find a peg revision, remember where we found it,
> > * and calculate the actual length of the target URL. */
> > if (utf8_target[j - 1] == '@')
> > {
> > peg_start = &(utf8_target[j - 1]);
> >
> > /* XXX: Delete this comment before commit, I just added
> > * it to convince myself that the code is correct.
> > * There is no off-by-one here, since:
> > * [@|\0] --> target_len = 1 - 1 == 0
> > * [h|@|\0] --> target_len = 2 - 1 == 1
> > * [h|t|@|\0] --> target_len = 3 - 1 == 2
> > * [h|t|t|@|\0] --> target_len = 4 - 1 == 3
> > * [h|t|t|p|@|\0] --> target_len = 5 - 1 == 4
> > * ...
> > */
I think of it this way: The length of a '\0'-terminated string is the
index of the '\0', therefore the length of a '@'-terminated string is
the index of the '@'.
> > target_len = j - 1;
> >
> > break;
> > }
> > }
>
> I like it, though i'll probably do j = target_len -1 in the
> initialization as you must do [j-1] at every usage point, and the
> [j-1] is needed because here you are translating a length "j" into an
> index "j-1". I tend to think of "j" as always meaning an index.
>
> >
> > /* XXX: delete this comment before commit
> > *
> > * Note how we previously didn't handle target_len == 0 here!
> > * But we should probably throw an error or something,
> > * in this case, right? This case didn't even occur to me while
> > * thinking about the problem in terms of pointer arithmetic,
> > * but now it's rather obvious. We might also want to extend the
> > * function's docstring to mention that it will raise an error
> > * if it cannot split off a non-zero length target from the URL.
> > * Note that zero-length paths are considered canonical.
> > *
> > * Pseudo code example: */
>
> I'm still learning about all this 'canonical' stuff, but if a
> zero-length path is considered canonical then should this function
> support that? Will apr_pstrmemdup() not output an empty path?
>
Except the paths passed to this function are not canonical; they are user
input. If we allow the empty string here, wouldn't it allow
svn up ''
as synonym for
svn up '.'
?
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI
repository root relative urls.
Posted by Stefan Sperling <st...@elego.de>.
On Mon, May 12, 2008 at 06:49:55AM -0500, Troy Curtis Jr wrote:
> > Stylistic nit:
> >
> > With casting added, this becomes:
> >
> > peg_start = utf8_target + (const char *)j;
> >
> > This made me scratch my head at first, I hadn't seen pointers
> > and ints being mixed like that in a while.
> >
> > I bet there's tons of code out there that does this, but
>
> In fact I consolidated code that did this in three different places,
> into one. I tend not to use pointer arithmetic in my own newly
> generated code because it does seem kind of confusing to me too!
>
> > I myself would tend to use an apr_size_t variable initialised
> > to strlen(utf8_target), and decrement that in the loop instead of
> > adding pointers and ints. That is, I'd try not to rely on automatic
> > type conversion too much. See below for an example.
> >
> > Note also how the exit condition of the loop ("j >= 0") also depends on
> > j being a signed type (int), even though we're actually trying to
> > determine an apr_size_t (which is unsigned).
> >
>
> Though admittedly my reasons for not liking pointer arithmetic aren't
> so methodical. It just makes me think too hard :).
That's another problem with it, yeah.
It makes me think too hard, too :)
> > for (j = target_len; 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 - 1] == '/')
> > break;
> >
> > /* If we find a peg revision, remember where we found it,
> > * and calculate the actual length of the target URL. */
> > if (utf8_target[j - 1] == '@')
> > {
> > peg_start = &(utf8_target[j - 1]);
> >
> > /* XXX: Delete this comment before commit, I just added
> > * it to convince myself that the code is correct.
> > * There is no off-by-one here, since:
> > * [@|\0] --> target_len = 1 - 1 == 0
> > * [h|@|\0] --> target_len = 2 - 1 == 1
> > * [h|t|@|\0] --> target_len = 3 - 1 == 2
> > * [h|t|t|@|\0] --> target_len = 4 - 1 == 3
> > * [h|t|t|p|@|\0] --> target_len = 5 - 1 == 4
> > * ...
> > */
> > target_len = j - 1;
> >
> > break;
> > }
> > }
>
> I like it, though i'll probably do j = target_len -1 in the
> initialization as you must do [j-1] at every usage point, and the
> [j-1] is needed because here you are translating a length "j" into an
> index "j-1". I tend to think of "j" as always meaning an index.
Sure, feel free to do that. The reason I started out with
j = target_len is because initially I wasn't sure what we
should set target_len to once we found a peg rev, as you
can see from the comment. Starting out with j == target_len
simply made it easier for me to think about it. But you are right
of course. Since we're actually using j - 1 all over the place,
we might as well initialise it to target_len - 1 and just use 'j'.
> >
> > /* XXX: delete this comment before commit
> > *
> > * Note how we previously didn't handle target_len == 0 here!
> > * But we should probably throw an error or something,
> > * in this case, right? This case didn't even occur to me while
> > * thinking about the problem in terms of pointer arithmetic,
> > * but now it's rather obvious. We might also want to extend the
> > * function's docstring to mention that it will raise an error
> > * if it cannot split off a non-zero length target from the URL.
> > * Note that zero-length paths are considered canonical.
> > *
> > * Pseudo code example: */
>
> I'm still learning about all this 'canonical' stuff, but if a
> zero-length path is considered canonical then should this function
> support that?
What do you mean exactly by supporting zero-length paths?
> Will apr_pstrmemdup() not output an empty path?
It will, yes, even though its docstring doesn't explicitly say so:
/**
* duplicate a string into memory allocated out of a pool
* @param p The pool to allocate out of
* @param s The string to duplicate
* @return The new string
*/
But assuming this code won't change, it's safe to pass
in a zero-length string:
APR_DECLARE(char *) apr_pstrmemdup(apr_pool_t *a, const char *s, apr_size_t n)
{
char *res;
if (s == NULL) {
return NULL;
}
res = apr_palloc(a, n + 1);
memcpy(res, s, n);
res[n] = '\0';
return res;
}
The real quesion is, though: How will callers of split_arg_at_peg_revision
handle an empty target? Do we even want to hand the burden of dealing
with this to them? A zero-length URL target is certainly not useful,
so why not just throw an error?
Stefan
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI repository root relative urls.
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Mon, May 12, 2008 at 5:52 AM, Stefan Sperling <st...@elego.de> wrote:
>
> Hey Troy,
>
> just two minor nits, no big review, I guess Julian is more in touch
> with the topic of your patches than I am.
>
> On Sun, May 11, 2008 at 03:57:30PM -0500, Troy Curtis Jr wrote:
>> +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)
>> +{
>> + 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;
>
> Stylistic nit:
>
> With casting added, this becomes:
>
> peg_start = utf8_target + (const char *)j;
>
> This made me scratch my head at first, I hadn't seen pointers
> and ints being mixed like that in a while.
>
> I bet there's tons of code out there that does this, but
In fact I consolidated code that did this in three different places,
into one. I tend not to use pointer arithmetic in my own newly
generated code because it does seem kind of confusing to me too!
> I myself would tend to use an apr_size_t variable initialised
> to strlen(utf8_target), and decrement that in the loop instead of
> adding pointers and ints. That is, I'd try not to rely on automatic
> type conversion too much. See below for an example.
>
> Note also how the exit condition of the loop ("j >= 0") also depends on
> j being a signed type (int), even though we're actually trying to
> determine an apr_size_t (which is unsigned).
>
Though admittedly my reasons for not liking pointer arithmetic aren't
so methodical. It just makes me think too hard :).
>> + break;
>> + }
>> + }
>> +
>> + if (peg_start)
>> + {
>> + *true_target = apr_pstrmemdup(pool,
>> + utf8_target,
>> + peg_start - utf8_target);
>
> Again, why not do an explicit cast if we're mixing types in
> expressions like that? For example:
>
> *true_target = apr_pstrmemdup(pool, utf8_target,
> (apr_size_t)(peg_start - utf8_target));
>
>
> Let me try to sketch out how we could do this without adding
> together variables of 3 different types (const char*, int, apr_size_t).
>
> 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)
> {
> const char *peg_start = NULL; /* pointer to the peg revision, if any */
> /* XXX: delete this comment before commit
> * Here's the only place where we rely on automatic type conversion
> * (size_t -> apr_size_t): */
> apr_size_t target_len = strlen(utf8_target);
> apr_size_t j;
>
> for (j = target_len; 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 - 1] == '/')
> break;
>
> /* If we find a peg revision, remember where we found it,
> * and calculate the actual length of the target URL. */
> if (utf8_target[j - 1] == '@')
> {
> peg_start = &(utf8_target[j - 1]);
>
> /* XXX: Delete this comment before commit, I just added
> * it to convince myself that the code is correct.
> * There is no off-by-one here, since:
> * [@|\0] --> target_len = 1 - 1 == 0
> * [h|@|\0] --> target_len = 2 - 1 == 1
> * [h|t|@|\0] --> target_len = 3 - 1 == 2
> * [h|t|t|@|\0] --> target_len = 4 - 1 == 3
> * [h|t|t|p|@|\0] --> target_len = 5 - 1 == 4
> * ...
> */
> target_len = j - 1;
>
> break;
> }
> }
I like it, though i'll probably do j = target_len -1 in the
initialization as you must do [j-1] at every usage point, and the
[j-1] is needed because here you are translating a length "j" into an
index "j-1". I tend to think of "j" as always meaning an index.
>
> /* XXX: delete this comment before commit
> *
> * Note how we previously didn't handle target_len == 0 here!
> * But we should probably throw an error or something,
> * in this case, right? This case didn't even occur to me while
> * thinking about the problem in terms of pointer arithmetic,
> * but now it's rather obvious. We might also want to extend the
> * function's docstring to mention that it will raise an error
> * if it cannot split off a non-zero length target from the URL.
> * Note that zero-length paths are considered canonical.
> *
> * Pseudo code example: */
I'm still learning about all this 'canonical' stuff, but if a
zero-length path is considered canonical then should this function
support that? Will apr_pstrmemdup() not output an empty path?
> if (target_len == 0)
> return svn_error_createf(..., "Invalid argument", ...);
>
> if (peg_start)
> {
> *true_target = apr_pstrmemdup(pool,
> utf8_target,
> target_len);
> *peg_revision = apr_pstrdup(pool, peg_start);
> }
> else
> {
> *true_target = utf8_target;
> *peg_revision = NULL;
> }
>
> return SVN_NO_ERROR;
> }
>
>
>> +
>> + *peg_revision = apr_pstrdup(pool, peg_start);
>> + }
>> + else
>> + {
>> + *true_target = utf8_target;
>> +
>> + *peg_revision = NULL;
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> svn_error_t *
>> svn_opt_parse_path(svn_opt_revision_t *rev,
>> const char **truepath,
>> const char *path /* UTF-8! */,
>> apr_pool_t *pool)
>> {
>
>
>> }
>> + else
>> + {
>> + /* Didn't find an peg revision. */
>
> Really minor nit: It's "a peg revision".
>
Oh I see, a grammar nazi. I'm from Texas, that's the way we talk
'round des parts!
> :)
>
> Thanks,
> Stefan
>
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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] ISSUE #3193 Fix peg revision parsing for CLI
repository root relative urls.
Posted by Stefan Sperling <st...@elego.de>.
Hey Troy,
just two minor nits, no big review, I guess Julian is more in touch
with the topic of your patches than I am.
On Sun, May 11, 2008 at 03:57:30PM -0500, Troy Curtis Jr wrote:
> +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)
> +{
> + 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;
Stylistic nit:
With casting added, this becomes:
peg_start = utf8_target + (const char *)j;
This made me scratch my head at first, I hadn't seen pointers
and ints being mixed like that in a while.
I bet there's tons of code out there that does this, but
I myself would tend to use an apr_size_t variable initialised
to strlen(utf8_target), and decrement that in the loop instead of
adding pointers and ints. That is, I'd try not to rely on automatic
type conversion too much. See below for an example.
Note also how the exit condition of the loop ("j >= 0") also depends on
j being a signed type (int), even though we're actually trying to
determine an apr_size_t (which is unsigned).
> + break;
> + }
> + }
> +
> + if (peg_start)
> + {
> + *true_target = apr_pstrmemdup(pool,
> + utf8_target,
> + peg_start - utf8_target);
Again, why not do an explicit cast if we're mixing types in
expressions like that? For example:
*true_target = apr_pstrmemdup(pool, utf8_target,
(apr_size_t)(peg_start - utf8_target));
Let me try to sketch out how we could do this without adding
together variables of 3 different types (const char*, int, apr_size_t).
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)
{
const char *peg_start = NULL; /* pointer to the peg revision, if any */
/* XXX: delete this comment before commit
* Here's the only place where we rely on automatic type conversion
* (size_t -> apr_size_t): */
apr_size_t target_len = strlen(utf8_target);
apr_size_t j;
for (j = target_len; 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 - 1] == '/')
break;
/* If we find a peg revision, remember where we found it,
* and calculate the actual length of the target URL. */
if (utf8_target[j - 1] == '@')
{
peg_start = &(utf8_target[j - 1]);
/* XXX: Delete this comment before commit, I just added
* it to convince myself that the code is correct.
* There is no off-by-one here, since:
* [@|\0] --> target_len = 1 - 1 == 0
* [h|@|\0] --> target_len = 2 - 1 == 1
* [h|t|@|\0] --> target_len = 3 - 1 == 2
* [h|t|t|@|\0] --> target_len = 4 - 1 == 3
* [h|t|t|p|@|\0] --> target_len = 5 - 1 == 4
* ...
*/
target_len = j - 1;
break;
}
}
/* XXX: delete this comment before commit
*
* Note how we previously didn't handle target_len == 0 here!
* But we should probably throw an error or something,
* in this case, right? This case didn't even occur to me while
* thinking about the problem in terms of pointer arithmetic,
* but now it's rather obvious. We might also want to extend the
* function's docstring to mention that it will raise an error
* if it cannot split off a non-zero length target from the URL.
* Note that zero-length paths are considered canonical.
*
* Pseudo code example: */
if (target_len == 0)
return svn_error_createf(..., "Invalid argument", ...);
if (peg_start)
{
*true_target = apr_pstrmemdup(pool,
utf8_target,
target_len);
*peg_revision = apr_pstrdup(pool, peg_start);
}
else
{
*true_target = utf8_target;
*peg_revision = NULL;
}
return SVN_NO_ERROR;
}
> +
> + *peg_revision = apr_pstrdup(pool, peg_start);
> + }
> + else
> + {
> + *true_target = utf8_target;
> +
> + *peg_revision = NULL;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_opt_parse_path(svn_opt_revision_t *rev,
> const char **truepath,
> const char *path /* UTF-8! */,
> apr_pool_t *pool)
> {
> }
> + else
> + {
> + /* Didn't find an peg revision. */
Really minor nit: It's "a peg revision".
:)
Thanks,
Stefan