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 2014/10/29 13:22:50 UTC
Don't reject arguments of the form ".@PEG"
An 'svn' command-line argument of the form ".@PEG" raises the error
svn: E125001: '@PEG' is just a peg revision. Maybe try '@PEG@' instead?
That error is annoying to me, and unnecessary.
This happens for pretty much any 'svn' command, with any path that is converted to "" on canonicalization and conversion to "internal style". It is inconsistent with how we interpret other arguments containing a peg specifier, including arguments such as "foo/..@PEG" which also refer to "this directory" at a semantic level but which are not collapsed to "@PEG" at a syntactic level.
I think the reason that error was introduced (by stsp in r878062) was to prevent people being caught out when we changed the interpretation of a filename that starts with "@" such as "@file". Before r878062 that had been interpreted as a filename, and afterwards as a peg revision.
It seems to me that check should only reject command-line arguments of the form "@PEG", and not of the form ".@PEG". However, the check was in a low-level function () that is used both after conversion to internal style as well as before conversion, so arguments of the form ".@PEG" were also being rejected.
I intend to correct this, to make arguments of the form ".@PEG" syntactically acceptable.
- Julian
Re: Don't reject arguments of the form ".@PEG"
Posted by Stefan Sperling <st...@apache.org>.
On Wed, Oct 29, 2014 at 01:26:26PM +0000, Julian Foad wrote:
> The change to svn_client_args_to_target_array2 is relaxing an error behaviour that it didn't document in the first place. All usage that worked (didn't error) before will still work. It seems unlikely that anything is relying on this error behaviour in a way that will cause problems, so I don't think that's necessary.
>
> (And svn_opt__split_arg_at_peg_revision is private.)
>
> - Julian
>
OK, sounds good then. Thanks for fixing this!
Re: Don't reject arguments of the form ".@PEG"
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
>> Don't you think we should bump the API for this?
>
> The change to svn_client_args_to_target_array2 is relaxing an error behaviour
> that it didn't document in the first place. All usage that worked
> (didn't error) before will still work. It seems unlikely that anything is
> relying on this error behaviour in a way that will cause problems, so I
> don't think that's necessary.
Bah, I got that wrong. svn_client_args_to_target_array2() was NOT the problem, and its behaviour is unchanged by this commit.
The problem was AFTER svn_client_args_to_target_array2 had converted paths to "internal style", then when we call svn_opt__split_arg_at_peg_revision() (typically via svn_opt_parse_path() or svn_cl__eat_peg_revisions()).
So svn_opt_parse_path() is the only public function whose behaviour changes. I think it would be sufficient to annotate its doc string like this:
* @since New in 1.1.
+ * @since Since 1.6.5, this returns an error if @a path contains a peg
+ * specifier with no path before it, such as "@abc".
+ * @since Since 1.9.0, this no longer returns an error if @a path contains a peg
+ * specifier with no path before it, such as "@abc".
r1635151 adds this note, and also tweaks the docs of svn_client_args_to_target_array*().
- Julian
Re: Don't reject arguments of the form ".@PEG"
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Wed, Oct 29, 2014 at 01:04:21PM +0000, Julian Foad wrote:
>> I (Julian Foad) wrote:
>>> An 'svn' command-line argument of the form ".@PEG" raises the error
>>>
>>> svn: E125001: '@PEG' is just a peg revision. Maybe try '@PEG@' instead?
>> [...]
>>> I intend to correct this, to make arguments of the form ".@PEG"
>>> syntactically acceptable.
>>
>> Committed r1635118.
>
> Don't you think we should bump the API for this?
>
> Whenever I made similar changes in the past (changing just the
> semantics of function arguments) others usually called for a bump.
The change to svn_client_args_to_target_array2 is relaxing an error behaviour that it didn't document in the first place. All usage that worked (didn't error) before will still work. It seems unlikely that anything is relying on this error behaviour in a way that will cause problems, so I don't think that's necessary.
(And svn_opt__split_arg_at_peg_revision is private.)
- Julian
Re: Don't reject arguments of the form ".@PEG"
Posted by Stefan Sperling <st...@apache.org>.
On Wed, Oct 29, 2014 at 01:04:21PM +0000, Julian Foad wrote:
> I (Julian Foad) wrote:
> > An 'svn' command-line argument of the form ".@PEG" raises the error
> >
> > svn: E125001: '@PEG' is just a peg revision. Maybe try '@PEG@' instead?
> [...]
> > I intend to correct this, to make arguments of the form ".@PEG"
> > syntactically acceptable.
>
> Committed r1635118.
>
> - Julian
Don't you think we should bump the API for this?
Whenever I made similar changes in the past (changing just the
semantics of function arguments) others usually called for a bump.
Re: Don't reject arguments of the form ".@PEG"
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> An 'svn' command-line argument of the form ".@PEG" raises the error
>
> svn: E125001: '@PEG' is just a peg revision. Maybe try '@PEG@' instead?
[...]
> I intend to correct this, to make arguments of the form ".@PEG"
> syntactically acceptable.
Committed r1635118.
- Julian
Re: Don't reject arguments of the form ".@PEG"
Posted by Stefan Sperling <st...@apache.org>.
On Wed, Oct 29, 2014 at 12:22:50PM +0000, Julian Foad wrote:
> An 'svn' command-line argument of the form ".@PEG" raises the error
>
> svn: E125001: '@PEG' is just a peg revision. Maybe try '@PEG@' instead?
>
> That error is annoying to me, and unnecessary.
>
> This happens for pretty much any 'svn' command, with any path that is converted to "" on canonicalization and conversion to "internal style". It is inconsistent with how we interpret other arguments containing a peg specifier, including arguments such as "foo/..@PEG" which also refer to "this directory" at a semantic level but which are not collapsed to "@PEG" at a syntactic level.
>
> I think the reason that error was introduced (by stsp in r878062) was to prevent people being caught out when we changed the interpretation of a filename that starts with "@" such as "@file". Before r878062 that had been interpreted as a filename, and afterwards as a peg revision.
>
> It seems to me that check should only reject command-line arguments of the form "@PEG", and not of the form ".@PEG". However, the check was in a low-level function () that is used both after conversion to internal style as well as before conversion, so arguments of the form ".@PEG" were also being rejected.
>
> I intend to correct this, to make arguments of the form ".@PEG" syntactically acceptable.
>
> - Julian
I agree this should be fixed.
I once tried but couldn't figure out a good fix because of the
chicken-and-egg problem of canonicalization vs. peg-revision parsing.
The current implementation canonicalizes first and then looks for peg
revisions. Perhaps we should change the order of these operations,
however these operations currrently happen in different layers of
code. And we might end up interpreting non-canonicalized input, which
may or may not pose a problem, dunno...
Or we could mark empty paths in some special way during canonicalization
so the peg revision parser has a chance to interpret a '.' path.
Perhaps we could even internally replace '.' with the absolute path of
the current working directory during canonicalization.