You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2008/08/27 20:44:53 UTC
Re: Incorrectness in svn_opt__split_arg_at_peg_revision() [was:
regression from 1.4.3 to 1.5.1 in handling filenames containg at (@) signs]
Karl Fogel wrote:
> I think svn_opt__split_arg_at_peg_revision() is buggy. This discovery
> was spurred by a bug report (quoted at the end of this mail).
>
> Here's the svn_opt__split_arg_at_peg_revision API:
>
> /* 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);
[...]
> SVN_ERR(svn_opt__split_arg_at_peg_revision(&true_target, &peg_rev,
> utf8_target, pool));
[...]
> ### Hmmm, I want to test what happens with "dir/@file@" instead,
> ### so I'll just set utf8_target to that:
[...]
> (gdb) p true_target
> $11 = 0x9e47598 "dir/@file"
> (gdb) p peg_rev
> $12 = 0x9e475a8 "@"
>
> ### Mmmm. Yup. So, 'peg_rev' should really be "", right?
No. It is by design that this function returns the entire peg-specifier
part of the input string, rather than a canonical peg specifier derived
from it. The callers rely on this.
Now, it's maybe not a great design, but that's how it is at the moment.
And the doc string is not clear on this point.
- Julian
> Should I just make the obvious fix -- that is, set 'peg_start' to NULL
> if we're on the first iteration of the loop -- or is there something
> deeper going on here?
[...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Incorrectness in svn_opt__split_arg_at_peg_revision()
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Wed, Aug 27, 2008 at 8:44 PM, <sc...@gmail.com> wrote:
> one more observation in case it helps... this problem seems limited to
> filenames with @ as the first character only... if the @ comes after the
> first, things seem ok to me:
>
> i.e. this works in 1.5.1:
>
> mkdir dir
> svn add dir
> svn ci -m test dir
> touch dir/a@file
> svn add dir/a@file
> svn ci -m test dir/a@file
>
> -Scott
Yep the reason here is that 'dir/@file' is split into 'dir/' and
'@file' (because the characters following the '@' symbol are detected
as a peg revision). Then 'dir/' is canonicalized into 'dir' (trailing
slashes are not canonical). Then it is stuck back together as
'dir@file' which of course doesn't exist.
dir/a@file is split into 'dir/a' and '@file', but now there is no
trailing slash for it to 'canonicalize' away so it is joined back
together as 'dir/a@file'.
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: Incorrectness in svn_opt__split_arg_at_peg_revision()
Posted by sc...@gmail.com.
one more observation in case it helps... this problem seems limited to
filenames with @ as the first character only... if the @ comes after the
first, things seem ok to me:
i.e. this works in 1.5.1:
mkdir dir
svn add dir
svn ci -m test dir
touch dir/a@file
svn add dir/a@file
svn ci -m test dir/a@file
-Scott
On Wed, 27 Aug 2008, Julian Foad wrote:
> On Wed, 2008-08-27 at 16:56 -0400, Karl Fogel wrote:
>> Julian Foad <ju...@btopenworld.com> writes:
>>> No. It is by design that this function returns the entire peg-specifier
>>> part of the input string, rather than a canonical peg specifier derived
>>> from it. The callers rely on this.
>>>
>>> Now, it's maybe not a great design, but that's how it is at the moment.
>>>
>>> And the doc string is not clear on this point.
>>
>> So in this instance, the caller is simply using the returned values
>> wrongly? (Forgetting for the moment the possibility of improving the
>> design -- I'm just talking about whether the caller is working correctly
>> with the API as it stands today.)
>
> This instance being svn_client_args_to_target_array()? No, that's using
> it correctly. It just splits off the peg-specifier part of the string
> temporarily, canonicalises the rest, and the re-appends the
> peg-specifier string, to leave a result string which is still properly
> peg-escaped (i.e. may end with "@" or with "@PEGREV" or with neither,
> just as it did on input).
>
>> Who is supposed to be responsible for peg-rev escaping? That is, if
>> someone puts "@" on the end of a path (presumably for the sole purpose
>> of establishing that there is *no* peg rev here), what API is
>> responsible for handling that?
>
> svn_opt_parse_path() - the API that parses the string into a path part
> and a peg. (Not this one that "splits" the string.)
>
> I notice now that svn_opt_parse_path()'s doc string doesn't mention its
> behaviour when just a trailing "@" is found, and furthermore the sole
> example of this shown in its doc string appears to be wrong, showing
> that the revision "base" would be returned in this case.
>
> - Julian
>
>
>> I would have thought it would be svn_opt__split_arg_at_peg_revision().
>> That is, it hadn't occurred to me to consider the final "@" to be "the
>> entire peg-specifier part of the input string". Rather, I'd have
>> considered it a marker that indicates that there is no peg-specifier
>> here at all. Clearly I'm wrong, but I still don't quite understand what
>> the system is supposed to be, then... Do you understand it?
>>
>> -Karl
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Incorrectness in svn_opt__split_arg_at_peg_revision()
Posted by Julian Foad <ju...@btopenworld.com>.
On Wed, 2008-08-27 at 16:56 -0400, Karl Fogel wrote:
> Julian Foad <ju...@btopenworld.com> writes:
> > No. It is by design that this function returns the entire peg-specifier
> > part of the input string, rather than a canonical peg specifier derived
> > from it. The callers rely on this.
> >
> > Now, it's maybe not a great design, but that's how it is at the moment.
> >
> > And the doc string is not clear on this point.
>
> So in this instance, the caller is simply using the returned values
> wrongly? (Forgetting for the moment the possibility of improving the
> design -- I'm just talking about whether the caller is working correctly
> with the API as it stands today.)
This instance being svn_client_args_to_target_array()? No, that's using
it correctly. It just splits off the peg-specifier part of the string
temporarily, canonicalises the rest, and the re-appends the
peg-specifier string, to leave a result string which is still properly
peg-escaped (i.e. may end with "@" or with "@PEGREV" or with neither,
just as it did on input).
> Who is supposed to be responsible for peg-rev escaping? That is, if
> someone puts "@" on the end of a path (presumably for the sole purpose
> of establishing that there is *no* peg rev here), what API is
> responsible for handling that?
svn_opt_parse_path() - the API that parses the string into a path part
and a peg. (Not this one that "splits" the string.)
I notice now that svn_opt_parse_path()'s doc string doesn't mention its
behaviour when just a trailing "@" is found, and furthermore the sole
example of this shown in its doc string appears to be wrong, showing
that the revision "base" would be returned in this case.
- Julian
> I would have thought it would be svn_opt__split_arg_at_peg_revision().
> That is, it hadn't occurred to me to consider the final "@" to be "the
> entire peg-specifier part of the input string". Rather, I'd have
> considered it a marker that indicates that there is no peg-specifier
> here at all. Clearly I'm wrong, but I still don't quite understand what
> the system is supposed to be, then... Do you understand it?
>
> -Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Incorrectness in svn_opt__split_arg_at_peg_revision()
Posted by Karl Fogel <kf...@red-bean.com>.
Julian Foad <ju...@btopenworld.com> writes:
> No. It is by design that this function returns the entire peg-specifier
> part of the input string, rather than a canonical peg specifier derived
> from it. The callers rely on this.
>
> Now, it's maybe not a great design, but that's how it is at the moment.
>
> And the doc string is not clear on this point.
So in this instance, the caller is simply using the returned values
wrongly? (Forgetting for the moment the possibility of improving the
design -- I'm just talking about whether the caller is working correctly
with the API as it stands today.)
Who is supposed to be responsible for peg-rev escaping? That is, if
someone puts "@" on the end of a path (presumably for the sole purpose
of establishing that there is *no* peg rev here), what API is
responsible for handling that?
I would have thought it would be svn_opt__split_arg_at_peg_revision().
That is, it hadn't occurred to me to consider the final "@" to be "the
entire peg-specifier part of the input string". Rather, I'd have
considered it a marker that indicates that there is no peg-specifier
here at all. Clearly I'm wrong, but I still don't quite understand what
the system is supposed to be, then... Do you understand it?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org