You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jens Seidel <je...@users.sourceforge.net> on 2008/05/23 20:03:00 UTC
Bug: IP address bogusly interpreted as peg revision
Hi,
I noticed the following bug in the 1.5 branch:
A IP address may bogusly interpreted as peg revision as demonstrated by
$ svn log svn+ssh://svn@10.0.1.1
subversion/svn/main.c:2035: (apr_err=205000)
svn: Try 'svn help' for more info
subversion/libsvn_subr/opt.c:863: (apr_err=205000)
svn: Syntax error parsing revision '10.0.1.1'
Accessing svn+ssh://svn@10.0.1.1@HEAD works.
Subversion 1.4 reports instead:
svn: Syntax error parsing revision '10.0.1.1'
Please CC: me.
Jens
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Mon, Jun 16, 2008 at 4:02 PM, Troy Curtis Jr <tr...@gmail.com> wrote:
> On Thu, May 29, 2008 at 2:15 AM, Julian Foad <ju...@btopenworld.com> wrote:
>> Troy Curtis Jr wrote:
>>>
>>> On Wed, May 28, 2008 at 12:56 PM, Julian Foad wrote:
>>>>
>>>> I took a look. This is completely localised within subversion/svn/; no
>>>> public library functions are affected. That makes it rather easy, I
>>>> think.
>>>>
>>>> Basically I'm talking about changing svn_client__args_to_target_array()
>>>> to
>>>> return an array of (const char *path, svn_opt_revision_t peg) pairs.
>>
>> [...]
>>>
>>> My first thought was "no thank you, I've already looked into that and
>>> its too much for me". But your comment about it being limited to
>>> subversion/svn caught my interest, so I looked at it again. Sure
>>> enough, you were right, who would've thought :)
>>>
>>> I'm not sure what I was thinking before, I seemed to think that the
>>> peg revs were processed quite deep, and I have comments in this thread
>>> that imply as much. But it really isn't like that. Perhaps it was
>>> because a couple of other people were asking for additional things in
>>> that returned structure that actually WERE processed deep in the API?
>>> I don't remember exactly.
>>>
>>> So on further consideration I've decided that yes, I'd like to give it a
>>> shot.
>>
>> I'm glad!
>>
>> BTW, I said it's only used in "svn" but it's used a bit in one or more of
>> the accompanying programs "svnadmin", "svnserve", etc. as well.
>>
>>
>>> There are several APIs that expect arrays of targets, but they
>>> invariably deal with working paths only or are modification commands
>>> that can affect HEAD only (i.e. svn_client_mkdir(),
>>> svn_client_delete(), etc) (thus not needing pegs, right?). I guess I
>>> will also need to make sure that no peg revisions were specified for
>>> those types of commands, and give an appropriate error message if one
>>> is found.
>>
>> Yes, that sounds right.
>>
>>
>>> This was brought up last time and so I'll bring it up again, should I
>>> create a brand-new structure to return (like svn_client_target_t for
>>> instance), or should I try to use the existing
>>> svn_client_copy_source_t. The name on that current one would be
>>> generally misleading of course. Should I create my new
>>> svn_client_target_t structure and also rev svn_client_copy4() to use
>>> it instead of it's special purpose svn_client_copy_source_t?
>>
>> Create a new one. As it's local to the command-line client program(s) and
>> not a public thing exposed by libraries, ... oh, but it will be exposed by
>> the public svn_client_args_to_target_arrayN(). In that case, yes, like you
>> said, "svn_client_target_t" would be a good name.
>>
>> Your struct will only have two of the fields that svn_client_copy_source_t
>> has, so doesn't replace it, so no point in changing the ...copy4() API. And
>> I think your struct might as well have the revision struct embedded in like
>> svn_wc_external_item2_t does, rather than another level of indirection like
>> ..._copy_source_t uses, since it's small and constant-size.
>>
>> - Julian
>>
>
> Well I hit a bit of a snag and would like some input. I started down
> the path of creating a new structure svn_client_target_t that is
> returned by svn_client_opt_args_to_target_array(). Things were going
> well as I was updating the svn cli commands until I got to
>
> --
> "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/)
>
Needless to say the above was an accident. I clicked 'send' trying to
resize my terminal. And it shouldn't have been sent as I figured it
out....
--
"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: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Thu, May 29, 2008 at 2:15 AM, Julian Foad <ju...@btopenworld.com> wrote:
> Troy Curtis Jr wrote:
>>
>> On Wed, May 28, 2008 at 12:56 PM, Julian Foad wrote:
>>>
>>> I took a look. This is completely localised within subversion/svn/; no
>>> public library functions are affected. That makes it rather easy, I
>>> think.
>>>
>>> Basically I'm talking about changing svn_client__args_to_target_array()
>>> to
>>> return an array of (const char *path, svn_opt_revision_t peg) pairs.
>
> [...]
>>
>> My first thought was "no thank you, I've already looked into that and
>> its too much for me". But your comment about it being limited to
>> subversion/svn caught my interest, so I looked at it again. Sure
>> enough, you were right, who would've thought :)
>>
>> I'm not sure what I was thinking before, I seemed to think that the
>> peg revs were processed quite deep, and I have comments in this thread
>> that imply as much. But it really isn't like that. Perhaps it was
>> because a couple of other people were asking for additional things in
>> that returned structure that actually WERE processed deep in the API?
>> I don't remember exactly.
>>
>> So on further consideration I've decided that yes, I'd like to give it a
>> shot.
>
> I'm glad!
>
> BTW, I said it's only used in "svn" but it's used a bit in one or more of
> the accompanying programs "svnadmin", "svnserve", etc. as well.
>
>
>> There are several APIs that expect arrays of targets, but they
>> invariably deal with working paths only or are modification commands
>> that can affect HEAD only (i.e. svn_client_mkdir(),
>> svn_client_delete(), etc) (thus not needing pegs, right?). I guess I
>> will also need to make sure that no peg revisions were specified for
>> those types of commands, and give an appropriate error message if one
>> is found.
>
> Yes, that sounds right.
>
>
>> This was brought up last time and so I'll bring it up again, should I
>> create a brand-new structure to return (like svn_client_target_t for
>> instance), or should I try to use the existing
>> svn_client_copy_source_t. The name on that current one would be
>> generally misleading of course. Should I create my new
>> svn_client_target_t structure and also rev svn_client_copy4() to use
>> it instead of it's special purpose svn_client_copy_source_t?
>
> Create a new one. As it's local to the command-line client program(s) and
> not a public thing exposed by libraries, ... oh, but it will be exposed by
> the public svn_client_args_to_target_arrayN(). In that case, yes, like you
> said, "svn_client_target_t" would be a good name.
>
> Your struct will only have two of the fields that svn_client_copy_source_t
> has, so doesn't replace it, so no point in changing the ...copy4() API. And
> I think your struct might as well have the revision struct embedded in like
> svn_wc_external_item2_t does, rather than another level of indirection like
> ..._copy_source_t uses, since it's small and constant-size.
>
> - Julian
>
Well I hit a bit of a snag and would like some input. I started down
the path of creating a new structure svn_client_target_t that is
returned by svn_client_opt_args_to_target_array(). Things were going
well as I was updating the svn cli commands until I got to
--
"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: Bug: IP address bogusly interpreted as peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
Troy Curtis Jr wrote:
> On Wed, May 28, 2008 at 12:56 PM, Julian Foad wrote:
>>I took a look. This is completely localised within subversion/svn/; no
>>public library functions are affected. That makes it rather easy, I think.
>>
>>Basically I'm talking about changing svn_client__args_to_target_array() to
>>return an array of (const char *path, svn_opt_revision_t peg) pairs.
[...]
>
> My first thought was "no thank you, I've already looked into that and
> its too much for me". But your comment about it being limited to
> subversion/svn caught my interest, so I looked at it again. Sure
> enough, you were right, who would've thought :)
>
> I'm not sure what I was thinking before, I seemed to think that the
> peg revs were processed quite deep, and I have comments in this thread
> that imply as much. But it really isn't like that. Perhaps it was
> because a couple of other people were asking for additional things in
> that returned structure that actually WERE processed deep in the API?
> I don't remember exactly.
>
> So on further consideration I've decided that yes, I'd like to give it a shot.
I'm glad!
BTW, I said it's only used in "svn" but it's used a bit in one or more of the
accompanying programs "svnadmin", "svnserve", etc. as well.
> There are several APIs that expect arrays of targets, but they
> invariably deal with working paths only or are modification commands
> that can affect HEAD only (i.e. svn_client_mkdir(),
> svn_client_delete(), etc) (thus not needing pegs, right?). I guess I
> will also need to make sure that no peg revisions were specified for
> those types of commands, and give an appropriate error message if one
> is found.
Yes, that sounds right.
> This was brought up last time and so I'll bring it up again, should I
> create a brand-new structure to return (like svn_client_target_t for
> instance), or should I try to use the existing
> svn_client_copy_source_t. The name on that current one would be
> generally misleading of course. Should I create my new
> svn_client_target_t structure and also rev svn_client_copy4() to use
> it instead of it's special purpose svn_client_copy_source_t?
Create a new one. As it's local to the command-line client program(s) and not a
public thing exposed by libraries, ... oh, but it will be exposed by the public
svn_client_args_to_target_arrayN(). In that case, yes, like you said,
"svn_client_target_t" would be a good name.
Your struct will only have two of the fields that svn_client_copy_source_t has,
so doesn't replace it, so no point in changing the ...copy4() API. And I think
your struct might as well have the revision struct embedded in like
svn_wc_external_item2_t does, rather than another level of indirection like
..._copy_source_t uses, since it's small and constant-size.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Wed, May 28, 2008 at 12:56 PM, Julian Foad
<ju...@btopenworld.com> wrote:
> Julian Foad wrote:
>>
>> Karl Fogel wrote:
>>
>>> Julian Foad <ju...@btopenworld.com> writes:
>>>
>>>> It sounds to me like what's broken is the idea that we can validly stick
>>>> the
>>>> "@PEG" back on to a path or URL again after parsing and canonicalising
>>>> it.
>>>>
>>>> Do you think we can get rid of all the code that relies on that, and
>>>> deprecate the functions that do it and any functions that currently
>>>> take "<canonical path or URL>@PEG" as input? Once we've parsed the
>>>> syntax, keep the results separate. That sounds like the Right
>>>> Thing. (The alternative, if we insist on glueing the two parts back
>>>> together, is to define how we're going to do it in a way that doesn't
>>>> lose information.)
>>>
>>> Public APIs would still take
>>>
>>> <canonical path or URL>@PEG
>>>
>>> right?
>>
>> No. I was suggesting deprecating all functions that currently do so.
>> Haven't looked to see how feasible that would be, though. It might mean most
>> of libsvn_client.
>
> I took a look. This is completely localised within subversion/svn/; no
> public library functions are affected. That makes it rather easy, I think.
>
> Basically I'm talking about changing svn_client__args_to_target_array() to
> return an array of (const char *path, svn_opt_revision_t peg) pairs.
>
> Troy, I recommended against making this change while you were working on the
> relative-URL patch, but now, as a separate improvement, it looks like it
> would be great... Would you care to have a go at this?
>
> - Julian
>
My first thought was "no thank you, I've already looked into that and
its too much for me". But your comment about it being limited to
subversion/svn caught my interest, so I looked at it again. Sure
enough, you were right, who would've thought :)
I'm not sure what I was thinking before, I seemed to think that the
peg revs were processed quite deep, and I have comments in this thread
that imply as much. But it really isn't like that. Perhaps it was
because a couple of other people were asking for additional things in
that returned structure that actually WERE processed deep in the API?
I don't remember exactly.
So on further consideration I've decided that yes, I'd like to give it a shot.
There are several APIs that expect arrays of targets, but they
invariably deal with working paths only or are modification commands
that can affect HEAD only (i.e. svn_client_mkdir(),
svn_client_delete(), etc) (thus not needing pegs, right?). I guess I
will also need to make sure that no peg revisions were specified for
those types of commands, and give an appropriate error message if one
is found.
This was brought up last time and so I'll bring it up again, should I
create a brand-new structure to return (like svn_client_target_t for
instance), or should I try to use the existing
svn_client_copy_source_t. The name on that current one would be
generally misleading of course. Should I create my new
svn_client_target_t structure and also rev svn_client_copy4() to use
it instead of it's special purpose svn_client_copy_source_t?
Thoughts?
>
>> Or, to develop the idea a bit further, the more precise problem is
>> thinking that the "@PEG" is optional in such APIs: you can't pass an
>> arbitrary <canonical path or URL> without a peg to such an API and guarantee
>> that it will be interpreted as not having a peg. Therefore another solution
>> is to deprecate those APIs that take
>>
>> <canonical path or URL>[@PEG]
>>
>> and replace them with ones that take
>>
>> <canonical path or URL>@[PEG]
>>
>> where the peg specifier field (starting with '@') has to be there even if
>> there is no peg as such.
>>
>> But if we're considering doing that, I think we'd want to take the
>> opportunity to separate the (optional) peg into a separate argument, because
>> that's cleaner, given that it will already have been parsed once (before
>> canonicalisation) anyway.
>>
>> (BTW, when I say "canonicalisation" in this thread, I really mean
>> everything we do to arguments, which includes character-coding conversions
>> and auto-escaping as well.)
>>
>>> Then we have the problem of one public API calling another one.
>>
>> Er... I don't follow. I guess you mean how would a deprecated function
>> taking, say, "THING[@PEG]", convert its argument to a form suitable for
>> passing to a different API taking, say, "THING@[PEG]". In the cases where
>> such a conversion is defined, I don't know why it would be a problem. In the
>> cases where it's ambiguous, like the one that started this thread
>> ("http://name@10.0.0.1") then we won't be any worse off than we are now.
>>
>>> Or are you proposing that users of our APIs would call some public
>>> function to separate THING[@PEG] into a { THING, [PEG] } structure,
>>> and then all the other public APIs would take that structure?
>>
>> Yes, something more like that. This peg-separating operation needs to be
>> done before or at the same time as "canonicalising" the raw arguments, so
>> probably during the *_args_to_target_array*() functions.
>>
>> - 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: Bug: IP address bogusly interpreted as peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> Karl Fogel wrote:
>
>> Julian Foad <ju...@btopenworld.com> writes:
>>
>>> It sounds to me like what's broken is the idea that we can validly
>>> stick the
>>> "@PEG" back on to a path or URL again after parsing and
>>> canonicalising it.
>>>
>>> Do you think we can get rid of all the code that relies on that, and
>>> deprecate the functions that do it and any functions that currently
>>> take "<canonical path or URL>@PEG" as input? Once we've parsed the
>>> syntax, keep the results separate. That sounds like the Right
>>> Thing. (The alternative, if we insist on glueing the two parts back
>>> together, is to define how we're going to do it in a way that doesn't
>>> lose information.)
>>
>> Public APIs would still take
>>
>> <canonical path or URL>@PEG
>>
>> right?
>
> No. I was suggesting deprecating all functions that currently do so.
> Haven't looked to see how feasible that would be, though. It might mean
> most of libsvn_client.
I took a look. This is completely localised within subversion/svn/; no public
library functions are affected. That makes it rather easy, I think.
Basically I'm talking about changing svn_client__args_to_target_array() to
return an array of (const char *path, svn_opt_revision_t peg) pairs.
Troy, I recommended against making this change while you were working on the
relative-URL patch, but now, as a separate improvement, it looks like it would
be great... Would you care to have a go at this?
- Julian
> Or, to develop the idea a bit further, the more precise problem is
> thinking that the "@PEG" is optional in such APIs: you can't pass an
> arbitrary <canonical path or URL> without a peg to such an API and
> guarantee that it will be interpreted as not having a peg. Therefore
> another solution is to deprecate those APIs that take
>
> <canonical path or URL>[@PEG]
>
> and replace them with ones that take
>
> <canonical path or URL>@[PEG]
>
> where the peg specifier field (starting with '@') has to be there even
> if there is no peg as such.
>
> But if we're considering doing that, I think we'd want to take the
> opportunity to separate the (optional) peg into a separate argument,
> because that's cleaner, given that it will already have been parsed once
> (before canonicalisation) anyway.
>
> (BTW, when I say "canonicalisation" in this thread, I really mean
> everything we do to arguments, which includes character-coding
> conversions and auto-escaping as well.)
>
>> Then we have the problem of one public API calling another one.
>
> Er... I don't follow. I guess you mean how would a deprecated function
> taking, say, "THING[@PEG]", convert its argument to a form suitable for
> passing to a different API taking, say, "THING@[PEG]". In the cases
> where such a conversion is defined, I don't know why it would be a
> problem. In the cases where it's ambiguous, like the one that started
> this thread ("http://name@10.0.0.1") then we won't be any worse off than
> we are now.
>
>> Or are you proposing that users of our APIs would call some public
>> function to separate THING[@PEG] into a { THING, [PEG] } structure,
>> and then all the other public APIs would take that structure?
>
> Yes, something more like that. This peg-separating operation needs to be
> done before or at the same time as "canonicalising" the raw arguments,
> so probably during the *_args_to_target_array*() functions.
>
> - Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
Karl Fogel wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>It sounds to me like what's broken is the idea that we can validly stick the
>>"@PEG" back on to a path or URL again after parsing and canonicalising it.
>>
>>Do you think we can get rid of all the code that relies on that, and
>>deprecate the functions that do it and any functions that currently
>>take "<canonical path or URL>@PEG" as input? Once we've parsed the
>>syntax, keep the results separate. That sounds like the Right
>>Thing. (The alternative, if we insist on glueing the two parts back
>>together, is to define how we're going to do it in a way that doesn't
>>lose information.)
>
>
> Public APIs would still take
>
> <canonical path or URL>@PEG
>
> right?
No. I was suggesting deprecating all functions that currently do so. Haven't
looked to see how feasible that would be, though. It might mean most of
libsvn_client.
Or, to develop the idea a bit further, the more precise problem is thinking
that the "@PEG" is optional in such APIs: you can't pass an arbitrary
<canonical path or URL> without a peg to such an API and guarantee that it will
be interpreted as not having a peg. Therefore another solution is to deprecate
those APIs that take
<canonical path or URL>[@PEG]
and replace them with ones that take
<canonical path or URL>@[PEG]
where the peg specifier field (starting with '@') has to be there even if there
is no peg as such.
But if we're considering doing that, I think we'd want to take the opportunity
to separate the (optional) peg into a separate argument, because that's
cleaner, given that it will already have been parsed once (before
canonicalisation) anyway.
(BTW, when I say "canonicalisation" in this thread, I really mean everything we
do to arguments, which includes character-coding conversions and auto-escaping
as well.)
> Then we have the problem of one public API calling another one.
Er... I don't follow. I guess you mean how would a deprecated function taking,
say, "THING[@PEG]", convert its argument to a form suitable for passing to a
different API taking, say, "THING@[PEG]". In the cases where such a conversion
is defined, I don't know why it would be a problem. In the cases where it's
ambiguous, like the one that started this thread ("http://name@10.0.0.1") then
we won't be any worse off than we are now.
> Or are you proposing that users of our APIs would call some public
> function to separate THING[@PEG] into a { THING, [PEG] } structure,
> and then all the other public APIs would take that structure?
Yes, something more like that. This peg-separating operation needs to be done
before or at the same time as "canonicalising" the raw arguments, so probably
during the *_args_to_target_array*() functions.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Karl Fogel <kf...@red-bean.com>.
Julian Foad <ju...@btopenworld.com> writes:
> It sounds to me like what's broken is the idea that we can validly stick the
> "@PEG" back on to a path or URL again after parsing and canonicalising it.
>
> Do you think we can get rid of all the code that relies on that, and
> deprecate the functions that do it and any functions that currently
> take "<canonical path or URL>@PEG" as input? Once we've parsed the
> syntax, keep the results separate. That sounds like the Right
> Thing. (The alternative, if we insist on glueing the two parts back
> together, is to define how we're going to do it in a way that doesn't
> lose information.)
Public APIs would still take
<canonical path or URL>@PEG
right? Then we have the problem of one public API calling another one.
Or are you proposing that users of our APIs would call some public
function to separate THING[@PEG] into a { THING, [PEG] } structure,
and then all the other public APIs would take that structure?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
Troy Curtis Jr wrote:
> It turns out this simplistic approach doesn't work. Surprising, no?
>
> My proposal hinged on the assumption that all the downstream functions
> handle peg revision and so we can just stick an empty peg revision on
> any target that doesn't already have one. But of course this isn't
> true of commands expecting only paths and I suspect now that I think
> of it there may be cases where peg revs aren't actually handled for
> certain url cases either.
It sounds to me like what's broken is the idea that we can validly stick the
"@PEG" back on to a path or URL again after parsing and canonicalising it.
Do you think we can get rid of all the code that relies on that, and deprecate
the functions that do it and any functions that currently take "<canonical path
or URL>@PEG" as input? Once we've parsed the syntax, keep the results separate.
That sounds like the Right Thing. (The alternative, if we insist on glueing the
two parts back together, is to define how we're going to do it in a way that
doesn't lose information.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Sun, May 25, 2008 at 2:04 PM, Troy Curtis Jr <tr...@gmail.com> wrote:
> On Sun, May 25, 2008 at 1:56 PM, Troy Curtis Jr <tr...@gmail.com> wrote:
>>>>> There are various work-arounds: the canonical way to "escape" an argument
>>>>> with no peg revision is to add an "@":
>>>>>
>>>>> $ svn log svn+ssh://svn@10.0.1.1@
>>>>>
>>>>> but in this particular example you might prefer to end the URL with "/":
>>>>>
>>>>> $ svn log svn+ssh://svn@10.0.1.1/
>>>>
>>>>
>>>> No, this leads to the same error.
>>>
>>> Oh, so it does! That's a bug: we're not supposed to be looking for a peg
>>> after the last "/". I suspect what is happening is the path is being
>>> "canonicalised" first, which whould strip off the final "/", but that's
>>> wrong and Troy Curtis is already working on a patch to fix the parsing of
>>> peg-revs with repository-relative URLs, and hopefully that will fix this
>>> too.
>>>
>>
>> Long Answer:
>>
>> Actually what is happening is this:
>> - Inside svn_client_args_to_target_array():
>> - It is determined there is no peg revision (the peg splitting logic
>> bails on the trailing slash)
>> - Then the string 'svn+ssh://svn@10.0.1.1/' is passed the
>> canonicalising function
>> - The trailing slash is removed to make it canonical
>> - The result is passed back in the targets array
>> - Inside subsequent command processing functions:
>> - At some point the target is passed to svn_opt_parse_path() to
>> extract the peg revision (if any)
>> - svn_opt_parse_path() does not see a peg revision (because we
>> removed the trailing slash) and so interprets
>> 10.0.1.1 as a peg, and an invalid one at that.
>>
>> The "solution" here is the only proper way to "escape" the '@' inside
>> a path is by putting a trailing '@' in the target string.
>>
>> Short Answer: It's not a bug, it's a feature! :)
>>
>> Troy
>>
>>>
>>>> I just asked to get the permission to file a bug. I didn't assumed the
>>>> behaviour could be accepted but it is indeed not a big deal so it is OK
>>>> for me.
>>>
>>> Thanks. If you'd like to help see if Troy's patch fixes the above usage,
>>> that would be great. (No worries if not.) His last revision of the patch is
>>> at <http://svn.haxx.se/dev/archive-2008-05/0477.shtml> and he hopes to send
>>> a revised version soon.
>>>
>>> - Julian
>>>
>>
> You know after thinking about it I could "fix" this behavior. I'm
> working on my revised patch right now and I think the trailing slash
> workaround would work if I returned "@" for no peg revision found
> instead of "" in my new fangled svn_opt__split_arg_at_peg_revision()
> function. This would mean that every single target coming out of the
> svn_client_args_to_target_array() would have a peg revision, but some
> of them would be blank.
>
> This actually changes the current CLI behavior (in that the trailing
> slash thing would not have worked before), but is it something that we
> want to include?
>
> Troy
>
It turns out this simplistic approach doesn't work. Surprising, no?
My proposal hinged on the assumption that all the downstream functions
handle peg revision and so we can just stick an empty peg revision on
any target that doesn't already have one. But of course this isn't
true of commands expecting only paths and I suspect now that I think
of it there may be cases where peg revs aren't actually handled for
certain url cases either.
Of course it took me actually trying to make the change and getting a
load of test failures to figure that out. :)
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: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Mon, May 26, 2008 at 1:16 AM, Jens Seidel <je...@users.sf.net> wrote:
> On Sun, May 25, 2008 at 09:45:56PM -0500, Troy Curtis Jr wrote:
>> Behavior if we always use '@' for no peg specified:
>> "." "@"
>> ".@BASE" "@BASE"
>> "foo///bar" "foo/bar@"
>> "http://a//b////" "http://a/b@"
>> "http://a///b@27" "http://a/b@27"
>> "http://a/b//@COMMITTED" "http://a/b@COMMITTED"
>> "foo///@bar@HEAD" "foo/@bar@HEAD"
>> "foo@///bar" "foo@/bar@"
>> "foo@HEAD///bar" "foo@HEAD/bar@"
>> Adding:
>> "svn+ssh://svn@10.0.1.1" "svn+ssh://svn@10.0.1.1"
>> "svn+ssh://svn@10.0.1.1/" "svn+ssh://svn@10.0.1.1@"
>> "svn+ssh://svn@10.0.1.1@" "svn+ssh://svn@10.0.1.1@"
>
> A stupid question: What would happen if there exists filenames with "@"
> in it? Could there be a filename conflict if @ is added?
>
No because the peg revision is always stripped off before the target
is treated as a url or path, which is the crux of the problem. In the
trailing slash case, where it gets removed during canonicalization,
@10.0.1.1 gets removed because subversion thinks it's a peg revision
and the resulting peg revision is not valid.
> PS: There is no longer any need for CC:ing me. I'm currently subscribed.
>
The standard method is typically to "Reply All", so if you don't want
a separate cced message you have to do what you just did here and
remove your name from the reply-to field :-).
Troy
> Jens
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
--
"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: Bug: IP address bogusly interpreted as peg revision
Posted by Jens Seidel <je...@users.sourceforge.net>.
On Sun, May 25, 2008 at 09:45:56PM -0500, Troy Curtis Jr wrote:
> Behavior if we always use '@' for no peg specified:
> "." "@"
> ".@BASE" "@BASE"
> "foo///bar" "foo/bar@"
> "http://a//b////" "http://a/b@"
> "http://a///b@27" "http://a/b@27"
> "http://a/b//@COMMITTED" "http://a/b@COMMITTED"
> "foo///@bar@HEAD" "foo/@bar@HEAD"
> "foo@///bar" "foo@/bar@"
> "foo@HEAD///bar" "foo@HEAD/bar@"
> Adding:
> "svn+ssh://svn@10.0.1.1" "svn+ssh://svn@10.0.1.1"
> "svn+ssh://svn@10.0.1.1/" "svn+ssh://svn@10.0.1.1@"
> "svn+ssh://svn@10.0.1.1@" "svn+ssh://svn@10.0.1.1@"
A stupid question: What would happen if there exists filenames with "@"
in it? Could there be a filename conflict if @ is added?
PS: There is no longer any need for CC:ing me. I'm currently subscribed.
Jens
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Mon, May 26, 2008 at 1:14 AM, Daniel Shahaf <d....@daniel.shahaf.co.il> wrote:
> Troy Curtis Jr wrote on Sun, 25 May 2008 at 21:45 -0500:
>> On Sun, May 25, 2008 at 8:39 PM, Karl Fogel <kf...@red-bean.com> wrote:
>> > "Troy Curtis Jr" <tr...@gmail.com> writes:
>> >> You know after thinking about it I could "fix" this behavior. I'm
>> >> working on my revised patch right now and I think the trailing slash
>> >> workaround would work if I returned "@" for no peg revision found
>> >> instead of "" in my new fangled svn_opt__split_arg_at_peg_revision()
>> >> function. This would mean that every single target coming out of the
>> >> svn_client_args_to_target_array() would have a peg revision, but some
>> >> of them would be blank.
>> >
>> > What's the connection betweening return "@" instead of "" and making
>> > trailing slashes prevent peg-revision interpretation?
>> >
>> > I'd hate to see an improvement to our peg revision interpretation affect
>> > APIs that way. Why can't we say "anything appearing before the last
>> > slash in a path is not a peg revision" without having the API effect you
>> > describe above?
>> >
>>
>
> Maybe append an "@" to the canonicalized path only if the latter contains
> an "@"? It is not necessary otherwise.
>
> Daniel
>
Sure, that's true but you'd have to put a bit of extra logic in there
and it only saves you a character. Though I suppose if later
functions print out error messages that have a trailing '@' in the
paths that the user doesn't expect it could be somewhat confusing. If
you wanted to only stick a trailing @ on when it mattered than the
logic would need to try to re-parse a peg revision on the already
parsed (and canonicalized) path. If a peg revision is "found" on the
second pass then the '@' is needed, otherwise it is not. Oh actually
that won't work because canonicalization happens somewhere else so the
svn_opt__split_arg_at_peg_revision() function could only check for the
existence of '@', not anything more intelligent than that.
Troy
>> The issue is where the canonicalization happens and when the peg
>> revision is actually parsed. When the paths are canonicalization
>> happens the peg revision is split off, the path is canonicallized and
>> the peg is stuck back on for later processing. Unfortunately a
>> trailing slash is removed during canonicallization, so during the
>> later peg processing it is not there to 'escape' the non-peg '@' sign.
>>
>> For all later interpretation a trailing '@' symbol and no @ symbol at
>> all mean the same thing, no specified peg revision. By having
>> svn_opt__split_arg_at_peg_revision() return "@" for an empty peg
>> revision (instead of the empty string) you can always get the desired
>> behavior. In the case of a trailing slash on a target, you are
>> effectively replacing the trailing slash "escaping" method (which is
>> not canonical) with the "real" trailing '@' escaping method. But you
>> do guarantee that all targets will have an '@' specifier, but most of
>> them will not have anything following them.
>>
>> Taking the some of the paths from the test_args_to_target_array() function in
>> subversion/tests/libsvn_client/client-test.c and adding a couple more:
>>
>> Current behavior:
>> "." ""
>> ".@BASE" "@BASE"
>> "foo///bar" "foo/bar"
>> "http://a//b////" "http://a/b"
>> "http://a///b@27" "http://a/b@27"
>> "http://a/b//@COMMITTED" "http://a/b@COMMITTED"
>> "foo@///bar" "foo@/bar"
>> "foo@HEAD///bar" "foo@HEAD/bar"
>> Adding:
>> "svn+ssh://svn@10.0.1.1" "svn+ssh://svn@10.0.1.1"
>> "svn+ssh://svn@10.0.1.1/" "svn+ssh://svn@10.0.1.1"
>> "svn+ssh://svn@10.0.1.1@" "svn+ssh://svn@10.0.1.1@"
>>
>> Behavior if we always use '@' for no peg specified:
>> "." "@"
>> ".@BASE" "@BASE"
>> "foo///bar" "foo/bar@"
>> "http://a//b////" "http://a/b@"
>> "http://a///b@27" "http://a/b@27"
>> "http://a/b//@COMMITTED" "http://a/b@COMMITTED"
>> "foo///@bar@HEAD" "foo/@bar@HEAD"
>> "foo@///bar" "foo@/bar@"
>> "foo@HEAD///bar" "foo@HEAD/bar@"
>> Adding:
>> "svn+ssh://svn@10.0.1.1" "svn+ssh://svn@10.0.1.1"
>> "svn+ssh://svn@10.0.1.1/" "svn+ssh://svn@10.0.1.1@"
>> "svn+ssh://svn@10.0.1.1@" "svn+ssh://svn@10.0.1.1@"
>>
>> All of the downstream APIs don't have to change and in fact don't have to know
>> about this modification. A trailing '@' IS a valid peg revision. If it is
>> there the APIs can take it. If not, it is handled just the same, unspecified.
>>
>> This isn't really a change in our specification at the APIs, just in how we
>> interpret the user specified args. It is probably valid for a user to believe
>> a trailing slash should tell Subversion that the characters before it are a
>> target and NOT a peg revision, despite any '@' symbols that may be in it.
>>
>> 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: Bug: IP address bogusly interpreted as peg revision
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Troy Curtis Jr wrote on Sun, 25 May 2008 at 21:45 -0500:
> On Sun, May 25, 2008 at 8:39 PM, Karl Fogel <kf...@red-bean.com> wrote:
> > "Troy Curtis Jr" <tr...@gmail.com> writes:
> >> You know after thinking about it I could "fix" this behavior. I'm
> >> working on my revised patch right now and I think the trailing slash
> >> workaround would work if I returned "@" for no peg revision found
> >> instead of "" in my new fangled svn_opt__split_arg_at_peg_revision()
> >> function. This would mean that every single target coming out of the
> >> svn_client_args_to_target_array() would have a peg revision, but some
> >> of them would be blank.
> >
> > What's the connection betweening return "@" instead of "" and making
> > trailing slashes prevent peg-revision interpretation?
> >
> > I'd hate to see an improvement to our peg revision interpretation affect
> > APIs that way. Why can't we say "anything appearing before the last
> > slash in a path is not a peg revision" without having the API effect you
> > describe above?
> >
>
Maybe append an "@" to the canonicalized path only if the latter contains
an "@"? It is not necessary otherwise.
Daniel
> The issue is where the canonicalization happens and when the peg
> revision is actually parsed. When the paths are canonicalization
> happens the peg revision is split off, the path is canonicallized and
> the peg is stuck back on for later processing. Unfortunately a
> trailing slash is removed during canonicallization, so during the
> later peg processing it is not there to 'escape' the non-peg '@' sign.
>
> For all later interpretation a trailing '@' symbol and no @ symbol at
> all mean the same thing, no specified peg revision. By having
> svn_opt__split_arg_at_peg_revision() return "@" for an empty peg
> revision (instead of the empty string) you can always get the desired
> behavior. In the case of a trailing slash on a target, you are
> effectively replacing the trailing slash "escaping" method (which is
> not canonical) with the "real" trailing '@' escaping method. But you
> do guarantee that all targets will have an '@' specifier, but most of
> them will not have anything following them.
>
> Taking the some of the paths from the test_args_to_target_array() function in
> subversion/tests/libsvn_client/client-test.c and adding a couple more:
>
> Current behavior:
> "." ""
> ".@BASE" "@BASE"
> "foo///bar" "foo/bar"
> "http://a//b////" "http://a/b"
> "http://a///b@27" "http://a/b@27"
> "http://a/b//@COMMITTED" "http://a/b@COMMITTED"
> "foo@///bar" "foo@/bar"
> "foo@HEAD///bar" "foo@HEAD/bar"
> Adding:
> "svn+ssh://svn@10.0.1.1" "svn+ssh://svn@10.0.1.1"
> "svn+ssh://svn@10.0.1.1/" "svn+ssh://svn@10.0.1.1"
> "svn+ssh://svn@10.0.1.1@" "svn+ssh://svn@10.0.1.1@"
>
> Behavior if we always use '@' for no peg specified:
> "." "@"
> ".@BASE" "@BASE"
> "foo///bar" "foo/bar@"
> "http://a//b////" "http://a/b@"
> "http://a///b@27" "http://a/b@27"
> "http://a/b//@COMMITTED" "http://a/b@COMMITTED"
> "foo///@bar@HEAD" "foo/@bar@HEAD"
> "foo@///bar" "foo@/bar@"
> "foo@HEAD///bar" "foo@HEAD/bar@"
> Adding:
> "svn+ssh://svn@10.0.1.1" "svn+ssh://svn@10.0.1.1"
> "svn+ssh://svn@10.0.1.1/" "svn+ssh://svn@10.0.1.1@"
> "svn+ssh://svn@10.0.1.1@" "svn+ssh://svn@10.0.1.1@"
>
> All of the downstream APIs don't have to change and in fact don't have to know
> about this modification. A trailing '@' IS a valid peg revision. If it is
> there the APIs can take it. If not, it is handled just the same, unspecified.
>
> This isn't really a change in our specification at the APIs, just in how we
> interpret the user specified args. It is probably valid for a user to believe
> a trailing slash should tell Subversion that the characters before it are a
> target and NOT a peg revision, despite any '@' symbols that may be in it.
>
> Troy
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Karl Fogel <kf...@red-bean.com>.
"Troy Curtis Jr" <tr...@gmail.com> writes:
> Nope. This issue isn't that it gets parsed incorrectly when the args
> are canonicalized, the issue is that it gets parsed incorrectly when
> the previously canonicalized string is used by whatever function is
> actually doing the work (update, commit, etc). Part of
> canonicalization is removing the trailing slash. So by the time it
> gets to the function doing the work, the trailing slash is no longer
> present to signal to the peg parsing to stop looking for a peg.
Ahhh, thank you. Right, you'd said this before, I just forgot.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Mon, May 26, 2008 at 11:38 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "Troy Curtis Jr" <tr...@gmail.com> writes:
>> This isn't really a change in our specification at the APIs, just in how we
>> interpret the user specified args. It is probably valid for a user to believe
>> a trailing slash should tell Subversion that the characters before it are a
>> target and NOT a peg revision, despite any '@' symbols that may be in it.
>
> I think r31458 made this happen, right? Does this mean that the "just
> add a trailing slash to the IP address" workaround now actually works?
>
> -K
>
Nope. This issue isn't that it gets parsed incorrectly when the args
are canonicalized, the issue is that it gets parsed incorrectly when
the previously canonicalized string is used by whatever function is
actually doing the work (update, commit, etc). Part of
canonicalization is removing the trailing slash. So by the time it
gets to the function doing the work, the trailing slash is no longer
present to signal to the peg parsing to stop looking for a peg.
So
User Input: svn+ssh://svn@10.0.1.1/
After svn_client_args_to_target_array(): svn+ssh://svn@10.0.1.1
Now this is passed to svn_opt_parse_path() to actually 'parse' the peg
revision (and return it in a svn_opt_revision_t structure). But of
course it will now think 10.0.1.1 is the peg revision.
So still the only valid escaping method is a trailing '@'. Just for
confirmation I tried it on my wife's computer using 1.4.6:
<test>
holly[~]$ svn --version
svn, version 1.4.6 (r28521)
compiled Mar 30 2008, 19:51:11
Copyright (C) 2000-2007 CollabNet.
Subversion is open source software, see http://subversion.tigris.org/
This product includes software developed by CollabNet (http://www.Collab.Net/).
The following repository access (RA) modules are available:
* ra_dav : Module for accessing a repository via WebDAV (DeltaV) protocol.
- handles 'http' scheme
- handles 'https' scheme
* ra_svn : Module for accessing a repository using the svn network protocol.
- handles 'svn' scheme
* ra_local : Module for accessing a repository on local disk.
- handles 'file' scheme
holly[~]$ svn ls svn+ssh://svn@10.0.1.1
svn: Syntax error parsing revision '10.0.1.1'
holly[~]$ svn ls svn+ssh://svn@10.0.1.1/
svn: Syntax error parsing revision '10.0.1.1'
holly[~]$ svn ls svn+ssh://svn@10.0.1.1/@
[Here I hit Ctrl-C after a pause...]
svn: Connection closed unexpectedly
holly[~]$
</test>
After re-reading the OP I'm not actually sure what he is saying is
really different between 1.5 and 1.4. They both try to parse 10.0.1.1
as a peg revision and print an error. 1.5 also suggests that he check
'svn help' for usage.
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: Bug: IP address bogusly interpreted as peg revision
Posted by Karl Fogel <kf...@red-bean.com>.
"Troy Curtis Jr" <tr...@gmail.com> writes:
> This isn't really a change in our specification at the APIs, just in how we
> interpret the user specified args. It is probably valid for a user to believe
> a trailing slash should tell Subversion that the characters before it are a
> target and NOT a peg revision, despite any '@' symbols that may be in it.
I think r31458 made this happen, right? Does this mean that the "just
add a trailing slash to the IP address" workaround now actually works?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Sun, May 25, 2008 at 8:39 PM, Karl Fogel <kf...@red-bean.com> wrote:
> "Troy Curtis Jr" <tr...@gmail.com> writes:
>> You know after thinking about it I could "fix" this behavior. I'm
>> working on my revised patch right now and I think the trailing slash
>> workaround would work if I returned "@" for no peg revision found
>> instead of "" in my new fangled svn_opt__split_arg_at_peg_revision()
>> function. This would mean that every single target coming out of the
>> svn_client_args_to_target_array() would have a peg revision, but some
>> of them would be blank.
>
> What's the connection betweening return "@" instead of "" and making
> trailing slashes prevent peg-revision interpretation?
>
> I'd hate to see an improvement to our peg revision interpretation affect
> APIs that way. Why can't we say "anything appearing before the last
> slash in a path is not a peg revision" without having the API effect you
> describe above?
>
The issue is where the canonicalization happens and when the peg
revision is actually parsed. When the paths are canonicalization
happens the peg revision is split off, the path is canonicallized and
the peg is stuck back on for later processing. Unfortunately a
trailing slash is removed during canonicallization, so during the
later peg processing it is not there to 'escape' the non-peg '@' sign.
For all later interpretation a trailing '@' symbol and no @ symbol at
all mean the same thing, no specified peg revision. By having
svn_opt__split_arg_at_peg_revision() return "@" for an empty peg
revision (instead of the empty string) you can always get the desired
behavior. In the case of a trailing slash on a target, you are
effectively replacing the trailing slash "escaping" method (which is
not canonical) with the "real" trailing '@' escaping method. But you
do guarantee that all targets will have an '@' specifier, but most of
them will not have anything following them.
Taking the some of the paths from the test_args_to_target_array() function in
subversion/tests/libsvn_client/client-test.c and adding a couple more:
Current behavior:
"." ""
".@BASE" "@BASE"
"foo///bar" "foo/bar"
"http://a//b////" "http://a/b"
"http://a///b@27" "http://a/b@27"
"http://a/b//@COMMITTED" "http://a/b@COMMITTED"
"foo@///bar" "foo@/bar"
"foo@HEAD///bar" "foo@HEAD/bar"
Adding:
"svn+ssh://svn@10.0.1.1" "svn+ssh://svn@10.0.1.1"
"svn+ssh://svn@10.0.1.1/" "svn+ssh://svn@10.0.1.1"
"svn+ssh://svn@10.0.1.1@" "svn+ssh://svn@10.0.1.1@"
Behavior if we always use '@' for no peg specified:
"." "@"
".@BASE" "@BASE"
"foo///bar" "foo/bar@"
"http://a//b////" "http://a/b@"
"http://a///b@27" "http://a/b@27"
"http://a/b//@COMMITTED" "http://a/b@COMMITTED"
"foo///@bar@HEAD" "foo/@bar@HEAD"
"foo@///bar" "foo@/bar@"
"foo@HEAD///bar" "foo@HEAD/bar@"
Adding:
"svn+ssh://svn@10.0.1.1" "svn+ssh://svn@10.0.1.1"
"svn+ssh://svn@10.0.1.1/" "svn+ssh://svn@10.0.1.1@"
"svn+ssh://svn@10.0.1.1@" "svn+ssh://svn@10.0.1.1@"
All of the downstream APIs don't have to change and in fact don't have to know
about this modification. A trailing '@' IS a valid peg revision. If it is
there the APIs can take it. If not, it is handled just the same, unspecified.
This isn't really a change in our specification at the APIs, just in how we
interpret the user specified args. It is probably valid for a user to believe
a trailing slash should tell Subversion that the characters before it are a
target and NOT a peg revision, despite any '@' symbols that may be in it.
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: Bug: IP address bogusly interpreted as peg revision
Posted by Karl Fogel <kf...@red-bean.com>.
"Troy Curtis Jr" <tr...@gmail.com> writes:
> You know after thinking about it I could "fix" this behavior. I'm
> working on my revised patch right now and I think the trailing slash
> workaround would work if I returned "@" for no peg revision found
> instead of "" in my new fangled svn_opt__split_arg_at_peg_revision()
> function. This would mean that every single target coming out of the
> svn_client_args_to_target_array() would have a peg revision, but some
> of them would be blank.
What's the connection betweening return "@" instead of "" and making
trailing slashes prevent peg-revision interpretation?
I'd hate to see an improvement to our peg revision interpretation affect
APIs that way. Why can't we say "anything appearing before the last
slash in a path is not a peg revision" without having the API effect you
describe above?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Sun, May 25, 2008 at 1:56 PM, Troy Curtis Jr <tr...@gmail.com> wrote:
>>>> There are various work-arounds: the canonical way to "escape" an argument
>>>> with no peg revision is to add an "@":
>>>>
>>>> $ svn log svn+ssh://svn@10.0.1.1@
>>>>
>>>> but in this particular example you might prefer to end the URL with "/":
>>>>
>>>> $ svn log svn+ssh://svn@10.0.1.1/
>>>
>>>
>>> No, this leads to the same error.
>>
>> Oh, so it does! That's a bug: we're not supposed to be looking for a peg
>> after the last "/". I suspect what is happening is the path is being
>> "canonicalised" first, which whould strip off the final "/", but that's
>> wrong and Troy Curtis is already working on a patch to fix the parsing of
>> peg-revs with repository-relative URLs, and hopefully that will fix this
>> too.
>>
>
> Long Answer:
>
> Actually what is happening is this:
> - Inside svn_client_args_to_target_array():
> - It is determined there is no peg revision (the peg splitting logic
> bails on the trailing slash)
> - Then the string 'svn+ssh://svn@10.0.1.1/' is passed the
> canonicalising function
> - The trailing slash is removed to make it canonical
> - The result is passed back in the targets array
> - Inside subsequent command processing functions:
> - At some point the target is passed to svn_opt_parse_path() to
> extract the peg revision (if any)
> - svn_opt_parse_path() does not see a peg revision (because we
> removed the trailing slash) and so interprets
> 10.0.1.1 as a peg, and an invalid one at that.
>
> The "solution" here is the only proper way to "escape" the '@' inside
> a path is by putting a trailing '@' in the target string.
>
> Short Answer: It's not a bug, it's a feature! :)
>
> Troy
>
>>
>>> I just asked to get the permission to file a bug. I didn't assumed the
>>> behaviour could be accepted but it is indeed not a big deal so it is OK
>>> for me.
>>
>> Thanks. If you'd like to help see if Troy's patch fixes the above usage,
>> that would be great. (No worries if not.) His last revision of the patch is
>> at <http://svn.haxx.se/dev/archive-2008-05/0477.shtml> and he hopes to send
>> a revised version soon.
>>
>> - Julian
>>
>
You know after thinking about it I could "fix" this behavior. I'm
working on my revised patch right now and I think the trailing slash
workaround would work if I returned "@" for no peg revision found
instead of "" in my new fangled svn_opt__split_arg_at_peg_revision()
function. This would mean that every single target coming out of the
svn_client_args_to_target_array() would have a peg revision, but some
of them would be blank.
This actually changes the current CLI behavior (in that the trailing
slash thing would not have worked before), but is it something that we
want to include?
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: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Sun, May 25, 2008 at 1:56 PM, Troy Curtis Jr <tr...@gmail.com> wrote:
>>>> There are various work-arounds: the canonical way to "escape" an argument
>>>> with no peg revision is to add an "@":
>>>>
>>>> $ svn log svn+ssh://svn@10.0.1.1@
>>>>
>>>> but in this particular example you might prefer to end the URL with "/":
>>>>
>>>> $ svn log svn+ssh://svn@10.0.1.1/
>>>
>>>
>>> No, this leads to the same error.
>>
>> Oh, so it does! That's a bug: we're not supposed to be looking for a peg
>> after the last "/". I suspect what is happening is the path is being
>> "canonicalised" first, which whould strip off the final "/", but that's
>> wrong and Troy Curtis is already working on a patch to fix the parsing of
>> peg-revs with repository-relative URLs, and hopefully that will fix this
>> too.
>>
>
> Long Answer:
>
> Actually what is happening is this:
> - Inside svn_client_args_to_target_array():
> - It is determined there is no peg revision (the peg splitting logic
> bails on the trailing slash)
> - Then the string 'svn+ssh://svn@10.0.1.1/' is passed the
> canonicalising function
> - The trailing slash is removed to make it canonical
> - The result is passed back in the targets array
> - Inside subsequent command processing functions:
> - At some point the target is passed to svn_opt_parse_path() to
> extract the peg revision (if any)
> - svn_opt_parse_path() does not see a peg revision (because we
It may be obvious to you, but the above SHOULD have said "does not see
a trailing slash"
> removed the trailing slash) and so interprets
> 10.0.1.1 as a peg, and an invalid one at that.
>
> The "solution" here is the only proper way to "escape" the '@' inside
> a path is by putting a trailing '@' in the target string.
>
> Short Answer: It's not a bug, it's a feature! :)
>
> Troy
>
>>
>>> I just asked to get the permission to file a bug. I didn't assumed the
>>> behaviour could be accepted but it is indeed not a big deal so it is OK
>>> for me.
>>
>> Thanks. If you'd like to help see if Troy's patch fixes the above usage,
>> that would be great. (No worries if not.) His last revision of the patch is
>> at <http://svn.haxx.se/dev/archive-2008-05/0477.shtml> and he hopes to send
>> a revised version soon.
>>
>> - 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/)
>
--
"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: Bug: IP address bogusly interpreted as peg revision
Posted by Troy Curtis Jr <tr...@gmail.com>.
>>> There are various work-arounds: the canonical way to "escape" an argument
>>> with no peg revision is to add an "@":
>>>
>>> $ svn log svn+ssh://svn@10.0.1.1@
>>>
>>> but in this particular example you might prefer to end the URL with "/":
>>>
>>> $ svn log svn+ssh://svn@10.0.1.1/
>>
>>
>> No, this leads to the same error.
>
> Oh, so it does! That's a bug: we're not supposed to be looking for a peg
> after the last "/". I suspect what is happening is the path is being
> "canonicalised" first, which whould strip off the final "/", but that's
> wrong and Troy Curtis is already working on a patch to fix the parsing of
> peg-revs with repository-relative URLs, and hopefully that will fix this
> too.
>
Long Answer:
Actually what is happening is this:
- Inside svn_client_args_to_target_array():
- It is determined there is no peg revision (the peg splitting logic
bails on the trailing slash)
- Then the string 'svn+ssh://svn@10.0.1.1/' is passed the
canonicalising function
- The trailing slash is removed to make it canonical
- The result is passed back in the targets array
- Inside subsequent command processing functions:
- At some point the target is passed to svn_opt_parse_path() to
extract the peg revision (if any)
- svn_opt_parse_path() does not see a peg revision (because we
removed the trailing slash) and so interprets
10.0.1.1 as a peg, and an invalid one at that.
The "solution" here is the only proper way to "escape" the '@' inside
a path is by putting a trailing '@' in the target string.
Short Answer: It's not a bug, it's a feature! :)
Troy
>
>> I just asked to get the permission to file a bug. I didn't assumed the
>> behaviour could be accepted but it is indeed not a big deal so it is OK
>> for me.
>
> Thanks. If you'd like to help see if Troy's patch fixes the above usage,
> that would be great. (No worries if not.) His last revision of the patch is
> at <http://svn.haxx.se/dev/archive-2008-05/0477.shtml> and he hopes to send
> a revised version soon.
>
> - 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: Bug: IP address bogusly interpreted as peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
(Troy - CC'ing you.)
Jens Seidel wrote:
> On Fri, May 23, 2008 at 09:17:50PM +0100, Julian Foad wrote:
>
>>Jens Seidel wrote:
>>
>>>I noticed the following bug in the 1.5 branch:
>>>A IP address may bogusly interpreted as peg revision as demonstrated by
>>>
>>>$ svn log svn+ssh://svn@10.0.1.1
>>
>>[...]
>>
>>>svn: Syntax error parsing revision '10.0.1.1'
>>
>>That's unfortunate, but not a bug. It's a natural consequence of defining
>
> Not a bug? OK, I trust you. But a workaround should not be difficult.
I'm not sure we mean the same thing by "workaround"!
>>the command-line syntax as
>>
>> PATH-OR-URL[@PEG]
>
> Never considered this as strong syntax definition.
Well, that's just how it is at the moment, as far as I know, not necessarily a
strong decision. We might be able to define it some other way, maybe something
like:
PATH[@PEG] | SCHEME://HOST[/[URLPATH][@PEG]]
such that your usage would do what you want. That might be a good idea. I don't
know.
>>There are various work-arounds: the canonical way to "escape" an argument
>>with no peg revision is to add an "@":
>>
>> $ svn log svn+ssh://svn@10.0.1.1@
>>
>>but in this particular example you might prefer to end the URL with "/":
>>
>> $ svn log svn+ssh://svn@10.0.1.1/
>
>
> No, this leads to the same error.
Oh, so it does! That's a bug: we're not supposed to be looking for a peg after
the last "/". I suspect what is happening is the path is being "canonicalised"
first, which whould strip off the final "/", but that's wrong and Troy Curtis
is already working on a patch to fix the parsing of peg-revs with
repository-relative URLs, and hopefully that will fix this too.
> I just asked to get the permission to file a bug. I didn't assumed the
> behaviour could be accepted but it is indeed not a big deal so it is OK
> for me.
Thanks. If you'd like to help see if Troy's patch fixes the above usage, that
would be great. (No worries if not.) His last revision of the patch is at
<http://svn.haxx.se/dev/archive-2008-05/0477.shtml> and he hopes to send a
revised version soon.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2008-05-23 at 19:11 -0500, Peter Samuelson wrote:
> Server configuration, for svn+ssh://? I seem to recall that svnserve
> doesn't honor -r if -t is set. If that's true, this repository would
> have to be in the literal root of your filesystem.
svnserve honors -r in combination with -t just fine. See
notes/ssh-tricks.
At any rate, this problem is not, at least in theory, specific to the
svn+ssh scheme. "user@" can appear in the authority part of any scheme
following the standard syntax.
> 3. Consider any @ before a / to not be a peg rev delimiter. (Or is
> there some peg rev syntax that allows / in it?)
The original URL in this example was svn+ssh://svn@ipaddr with no
trailing slash, so this change would only address the suggested
workaround (svn+ssh://svn@ipaddr/) and not the original use case. But
it's not necessarily a bad compromise.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Peter Samuelson <pe...@p12n.org>.
[Greg Hudson]
> The URL you mentioned *could* be interpreted as the repository "svn
> +ssh://svn" at the mis-specified peg rev 10.0.1.1. This
> interpretation would only make sense if (1) "svn" was a valid
> hostname with your resolver configuration, (2) there was a repository
> located at the virtual root of that host (i.e. no repository path as
> part of the URL; it could be located anywhere in the filesystem via
> server configuration),
Server configuration, for svn+ssh://? I seem to recall that svnserve
doesn't honor -r if -t is set. If that's true, this repository would
have to be in the literal root of your filesystem.
> 1. We could make peg-rev processing aware of the URL syntax and ignore
> '@' signs which occur before the path component, but that would cut off
> the ability to do scheme://hostname@pegrev for a repository located at
> the virtual root of a host. One could still use
> scheme://hostname/@pegrev so this might be acceptable.
>
> 2. We could make peg-rev processing aware of the peg-rev syntax and
> ignore IP addresses, hostnames, etc.. That would, at the very least,
> lead to bad error messages; if you misspell or misremember what a
> peg-rev specification should look like, instead of being told you have
> an invalid peg-rev specification, you get a path-not-found error.
3. Consider any @ before a / to not be a peg rev delimiter. (Or is
there some peg rev syntax that allows / in it?)
--
Peter Samuelson | org-tld!p12n!peter | http://p12n.org/
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Greg Hudson <gh...@MIT.EDU>.
The URL you mentioned *could* be interpreted as the repository "svn
+ssh://svn" at the mis-specified peg rev 10.0.1.1. This interpretation
would only make sense if (1) "svn" was a valid hostname with your
resolver configuration, (2) there was a repository located at the
virtual root of that host (i.e. no repository path as part of the URL;
it could be located anywhere in the filesystem via server
configuration), and (3) 10.0.1.1 was actually a typo for a valid peg-rev
specification and not something entirely different. All pretty
far-fetched, but it's not a parser's job to decide what is far-fetched.
So it's not instantly obvious how to be cleverer about this. I can see
two options:
1. We could make peg-rev processing aware of the URL syntax and ignore
'@' signs which occur before the path component, but that would cut off
the ability to do scheme://hostname@pegrev for a repository located at
the virtual root of a host. One could still use
scheme://hostname/@pegrev so this might be acceptable.
2. We could make peg-rev processing aware of the peg-rev syntax and
ignore IP addresses, hostnames, etc.. That would, at the very least,
lead to bad error messages; if you misspell or misremember what a
peg-rev specification should look like, instead of being told you have
an invalid peg-rev specification, you get a path-not-found error.
On Fri, 2008-05-23 at 22:32 +0200, Jens Seidel wrote:
> On Fri, May 23, 2008 at 09:17:50PM +0100, Julian Foad wrote:
> > Jens Seidel wrote:
> > >I noticed the following bug in the 1.5 branch:
> > >A IP address may bogusly interpreted as peg revision as demonstrated by
> > >
> > >$ svn log svn+ssh://svn@10.0.1.1
> > [...]
> > >svn: Syntax error parsing revision '10.0.1.1'
> >
> > That's unfortunate, but not a bug. It's a natural consequence of defining
>
> Not a bug? OK, I trust you. But a workaround should not be difficult.
>
> > the command-line syntax as
> >
> > PATH-OR-URL[@PEG]
>
> Never considered this as strong syntax definition.
>
> > There are various work-arounds: the canonical way to "escape" an argument
> > with no peg revision is to add an "@":
> >
> > $ svn log svn+ssh://svn@10.0.1.1@
> >
> > but in this particular example you might prefer to end the URL with "/":
> >
> > $ svn log svn+ssh://svn@10.0.1.1/
>
> No, this leads to the same error.
>
> I just asked to get the permission to file a bug. I didn't assumed the
> behaviour could be accepted but it is indeed not a big deal so it is OK
> for me.
>
> Jens
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Jens Seidel <je...@users.sourceforge.net>.
On Fri, May 23, 2008 at 09:17:50PM +0100, Julian Foad wrote:
> Jens Seidel wrote:
> >I noticed the following bug in the 1.5 branch:
> >A IP address may bogusly interpreted as peg revision as demonstrated by
> >
> >$ svn log svn+ssh://svn@10.0.1.1
> [...]
> >svn: Syntax error parsing revision '10.0.1.1'
>
> That's unfortunate, but not a bug. It's a natural consequence of defining
Not a bug? OK, I trust you. But a workaround should not be difficult.
> the command-line syntax as
>
> PATH-OR-URL[@PEG]
Never considered this as strong syntax definition.
> There are various work-arounds: the canonical way to "escape" an argument
> with no peg revision is to add an "@":
>
> $ svn log svn+ssh://svn@10.0.1.1@
>
> but in this particular example you might prefer to end the URL with "/":
>
> $ svn log svn+ssh://svn@10.0.1.1/
No, this leads to the same error.
I just asked to get the permission to file a bug. I didn't assumed the
behaviour could be accepted but it is indeed not a big deal so it is OK
for me.
Jens
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Bug: IP address bogusly interpreted as peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
Jens Seidel wrote:
> I noticed the following bug in the 1.5 branch:
> A IP address may bogusly interpreted as peg revision as demonstrated by
>
> $ svn log svn+ssh://svn@10.0.1.1
[...]
> svn: Syntax error parsing revision '10.0.1.1'
That's unfortunate, but not a bug. It's a natural consequence of defining the
command-line syntax as
PATH-OR-URL[@PEG]
There are various work-arounds: the canonical way to "escape" an argument with
no peg revision is to add an "@":
$ svn log svn+ssh://svn@10.0.1.1@
but in this particular example you might prefer to end the URL with "/":
$ svn log svn+ssh://svn@10.0.1.1/
or, as you showed, you can use "@HEAD".
Thanks for pointing it out.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org