Re: [PATCH] Issue 2318 : Inconsistent behavior with trailing '/'

"Madan US" <madan@collab.net> writes:

>> "Madan US" <madan@collab.net> writes:
>>
>>>> Madan U Sreenivasan <madan@collab.net> writes:
>>>>
>>>>> On Wed, 2005-10-05 at 02:56, Philip Martin wrote:
>>>>>> "Madan US" <madan@collab.net> writes:
>>>>>> >
>>>>>> > * subversion/libsvn_subr/path.c
>>>>>> > (svn_path_canonicalize): Modified to replace the trailing
>>>>>> slash
>>>>>> > in the filename to @ if the last segment of the file path
>>>>>> > contained an @.
>>>>
>>>> I think svn_path_canonicalize is the wrong place to be handling '@'
>>>> for peg revisions. All peg handling should happen in opt.c.
>>>
>>> We are not handling the peg revisions here. We are only adjusting the
>>> input path so that a filename with @ and followed by a trailing / would
>>> be
>>> recognised by the code that handles peg revisions (opt.c?) ... in this
>>> case, by replacing the trailing / with an @, so that
>>> svn_opt_parse_path()
>>> recognises the @ in the filename and not as an indicator for a peg
>>> revision...
>>
>> It's possible that there are clients that don't use the '@nnn' syntax
>> for peg revisions, it's a bit of a hack after all. Those clients will
>> want to use svn_path_canonicalize so that function cannot treat '@' as
>> special.
> .... and those client will use libsvn_subr?

They may well use svn_path_canonicalize.

> which takes the @ as a peg
> revision marker?

At present only svn_opt_parse_path in opt.c hadles '@' as a peg
revision. Your patch spreads that special behaviour into a second
function in a different file, that's bad.

> am not too sure...but maybe its just my ignorance...
>>
>>> Can you think of a better way to handle this in opt.c? If so, pl. let me
>>> know...
>>
>> I don't understand the issue: I'd like a description of the behaviour
>> you are trying to implement.
> Pl. see the tests I have added to path_test.c. It would clarify a little..
> anyways, I have elaborated my understanding below...
>
>> Why can't we simply require the user to input a trailing '@' instead
>> of a trailing '/'? When did trailing '/' become special?
> Yes..That was the requirement for issue #2317, which r15289 accompolished...

As far as I can see r15289 make a trailing '@' work, it doesn't make a
trailing '/' special.

> My understanding of 2318 is as following....
>
> A filename with a peg revision could be followed by a trailing slash as in
> file:///path/to/filewith_at_marker/ and expect svn to understand that
> filewith@marker is actually a filename and not a peg revisioned file.

I think that's silly.

> Similar to the way file:///path/to/filewith_at_marker@ will be treated (issue
> #2317)

That's the correct way to do it, why should we invent another?

> The rational behind the request was the following. I think its a little
> shallow myself....
>
> file:///path/to/filename/ is treated as if filename is a file(and not a
> dir due to the trailing slash)
> but
> file:///path/to/filewith_at_mark/ errors out saying that 'mark' is not a
> valid revision(and hence looks like filewith@mark is not treated as a
> filename as a whole)
>
> The reality is
> file:///path/to/file/ is treated as file:///path/to/file
> ...thanks to svn_path_canonocalize(). Similarly...
> file:///path/to/filewith_at_mark/ is treated as file:///path/to/filewith_at_mark
> and hence errors out saying that 'mark' is not a valid revision.
>
> Since its svn_path_canonicalize() that causes the symptom in the first
> place, I have tried to make it understand (a little intelligently) that
> the user might have used the trailing / intentionally - if the last
> segment of the filename contains an @ symbol (The trailing slash
> wouldnt make sense if what comes after that @ is a revision number).
> Hence the behavior of using a trailing / should be treated as if an
> trailing @ was present.
>
> Questions? Swearing? ;)

I think it's a mistake for svn_path_canonicalize to treat '@' as
special. I think it's a mistake to try to make a trailing '/' behave
in the same way as a trailing '@'. If there are circumstances when a
trailing '@' doesn't work then it would be better to fix them.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org