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...@wandisco.com> on 2010/12/20 11:45:53 UTC
Re: Input validation observations - svn info - changing "(Not a
valid URL)" to the specific error message
Re:
* "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
in revision REV"?
On Wed, 2010-12-08, Noorul Islam K M wrote:
[...]
> One of the solution that you suggested was to keep what the existing
> code is doing but saying something more helpful than "Not a valid
> URL". I thought that the error returned by the API might have useful
> information and could be printed using svn_err_best_message(). For
> example, take the following two cases.
>
> 1. a) With the patch
>
> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
> ^/non-existing
> URL 'file:///tmp/latestrepo/non-existing' non-existent in revision 0
>
> svn: A problem occurred; see other errors for details
>
> 1. b) Without the patch
>
> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
> ^/non-existing
> file:///tmp/latestrepo/non-existing: (Not a valid URL)
>
> svn: A problem occurred; see other errors for details
>
> 2. a) With the patch
>
> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
> invalid://no-domain
> Unrecognized URL scheme for 'invalid://non-domain'
>
> svn: A problem occurred; see other errors for details
>
> 2. b) Without patch
>
> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
> invalid://no-domain
> invalid://no-domain: (Not a valid URL)
>
> svn: A problem occurred; see other errors for details
>
> In both the above cases the patch helps the user to have better
> information (what actually went wrong). Also If a user is using the
> client API, I think he will be having these messages than the one that
> we hard coded as of today.
Thanks for these examples and comments. Yes, I agree the output is much
more helpful with the patch.
What about the "(Not a versioned resource)" warning? Just above the
code you changed, there is code to print "(Not a versioned resource)".
When is that message used? Should we change it in the same way? I
don't think it makes sense to change one of these and not the other.
> I ran the test suite and found that one of the test cases was failing
> and I fixed the same. There were no other failures.
Thanks.
[...]
> Make "svn info" to display the actual error message returned by API when
> illegal URL is passed.
>
> * subversion/svn/info-cmd.c
> (svn_cl__info): Display the actual error message returned by
> svn_client_info3() instead of a custom one.
>
> * subversion/tests/cmdline/basic_tests.py
> (info_nonexisting_file): Modify test case
[...]
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 1042948)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -2270,7 +2270,8 @@
>
> # Check for the correct error message
> for line in errput:
> - if re.match(".*\(Not a valid URL\).*", line):
> + if re.match(".*" + idonotexist_url + ".*non-existent in revision 1.*",
> + line):
> return
>
> # Else never matched the expected error output, so the test failed.
> Index: subversion/svn/info-cmd.c
> ===================================================================
> --- subversion/svn/info-cmd.c (revision 1042948)
> +++ subversion/svn/info-cmd.c (working copy)
> @@ -504,6 +504,7 @@
> svn_opt_revision_t peg_revision;
> svn_info_receiver_t receiver;
> const char *path_prefix;
> + char errbuf[256];
Please move this variable to the inner scope.
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> @@ -584,12 +585,9 @@
> }
> else if (err->apr_err == SVN_ERR_RA_ILLEGAL_URL)
> {
> - SVN_ERR(svn_cmdline_fprintf
> - (stderr, subpool,
> - _("%s: (Not a valid URL)\n\n"),
> - svn_path_is_url(truepath)
> - ? truepath
> - : svn_dirent_local_style(truepath, pool)));
> + SVN_ERR(svn_cmdline_fprintf(
> + stderr, subpool, _("%s\n\n"),
You can remove the _() wrapper because this string no longer contains
any local-language text to be translated.
> + svn_err_best_message(err, errbuf, sizeof(errbuf))));
> }
I think if we are going to print the error message, we should print it
with an "svn: " prefix like we normally do.
The old error message included a fixed string (the same for all possible
errors), which was helpful for people to recognize it as an error
message and for scripts to detect it. The "best message" doesn't have
any fixed part. I think adding an "svn: " prefix would enable people
and scripts to recognize it.
- Julian
Re: Input validation observations - svn info - changing "(Not a valid URL)" to the specific error message
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Julian Foad <ju...@wandisco.com> writes:
>
>> Re:
>> * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
>> in revision REV"?
>>
>> On Wed, 2010-12-08, Noorul Islam K M wrote:
>> [...]
>>> One of the solution that you suggested was to keep what the existing
>>> code is doing but saying something more helpful than "Not a valid
>>> URL". I thought that the error returned by the API might have useful
>>> information and could be printed using svn_err_best_message(). For
>>> example, take the following two cases.
>>>
>>> 1. a) With the patch
>>>
>>> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
>>> ^/non-existing
>>> URL 'file:///tmp/latestrepo/non-existing' non-existent in revision 0
>>>
>>> svn: A problem occurred; see other errors for details
>>>
>>> 1. b) Without the patch
>>>
>>> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
>>> ^/non-existing
>>> file:///tmp/latestrepo/non-existing: (Not a valid URL)
>>>
>>> svn: A problem occurred; see other errors for details
>>>
>>> 2. a) With the patch
>>>
>>> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
>>> invalid://no-domain
>>> Unrecognized URL scheme for 'invalid://non-domain'
>>>
>>> svn: A problem occurred; see other errors for details
>>>
>>> 2. b) Without patch
>>>
>>> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
>>> invalid://no-domain
>>> invalid://no-domain: (Not a valid URL)
>>>
>>> svn: A problem occurred; see other errors for details
>>>
>>> In both the above cases the patch helps the user to have better
>>> information (what actually went wrong). Also If a user is using the
>>> client API, I think he will be having these messages than the one that
>>> we hard coded as of today.
>>
>> Thanks for these examples and comments. Yes, I agree the output is much
>> more helpful with the patch.
>>
>> What about the "(Not a versioned resource)" warning? Just above the
>> code you changed, there is code to print "(Not a versioned resource)".
>> When is that message used? Should we change it in the same way? I
>> don't think it makes sense to change one of these and not the other.
>>
>>
>
> With version 1.7 I am not able to find a scenario for which this
> particular block gets executed. With 1.6, if I try to find information
> for non-existing wc path, then that particular block gets executed.
>
> Eg:-
>
> With 1.6
>
> $ svn info non-existent
>
> non-existent: (Not a versioned resource)
>
> ../subversion/libsvn_client/info.c:266: (apr_err=150000)
> svn: 'non-existent' is not under version control
>
> With 1.7 (trunk)
>
> $ svn info non-existent
> svn: The node '/tmp/wc/repo1.7/non-existent' was not found.
>
>
> Another thing that I observed is that when there are multiple targets
> specified for info command, 1.6 and 1.7 has behavioral difference.
>
> Eg:-
>
> With 1.6
>
> $ svn info non-existent A
>
> non-existent: (Not a versioned resource)
>
> Path: A
> Name: A
> URL: file:///tmp/repo1.6/A
> Repository Root: file:///tmp/repo1.6
> Repository UUID: 99761077-9087-4fca-ba50-95f68245589a
> Revision: 1
> Node Kind: file
> Schedule: normal
> Last Changed Author: noorul
> Last Changed Rev: 1
> Last Changed Date: 2010-12-23 11:40:29 +0530 (Thu, 23 Dec 2010)
> Text Last Updated: 2010-12-23 11:40:15 +0530 (Thu, 23 Dec 2010)
> Checksum: d41d8cd98f00b204e9800998ecf8427e
>
> ../subversion/libsvn_client/info.c:266: (apr_err=150000)
> svn: 'non-existent' is not under version control
>
>
> With 1.7
>
> $ svn info non-existent A
> svn: The node '/tmp/wc/repo1.7/non-existent' was not found.
>
> Ideally it should have displayed information for A since that particular
> file exists, instead it errors out.
>
> I fixed this by catching the exact error code SVN_ERR_WC_PATH_NOT_FOUND
> in this particular scenario. I think we don't have to catch
> SVN_ERR_UNVERSIONED_RESOURCE and SVN_ERR_ENTRY_NOT_FOUND any more as
> this was the case in 1.6.
>
>
>>> I ran the test suite and found that one of the test cases was failing
>>> and I fixed the same. There were no other failures.
>>
>> Thanks.
>>
>> [...]
>>> Make "svn info" to display the actual error message returned by API when
>>> illegal URL is passed.
>>>
>>> * subversion/svn/info-cmd.c
>>> (svn_cl__info): Display the actual error message returned by
>>> svn_client_info3() instead of a custom one.
>>>
>>> * subversion/tests/cmdline/basic_tests.py
>>> (info_nonexisting_file): Modify test case
>> [...]
>>
>>> Index: subversion/tests/cmdline/basic_tests.py
>>> ===================================================================
>>> --- subversion/tests/cmdline/basic_tests.py (revision 1042948)
>>> +++ subversion/tests/cmdline/basic_tests.py (working copy)
>>> @@ -2270,7 +2270,8 @@
>>>
>>> # Check for the correct error message
>>> for line in errput:
>>> - if re.match(".*\(Not a valid URL\).*", line):
>>> + if re.match(".*" + idonotexist_url + ".*non-existent in revision 1.*",
>>> + line):
>>> return
>>>
>>> # Else never matched the expected error output, so the test failed.
>>> Index: subversion/svn/info-cmd.c
>>> ===================================================================
>>> --- subversion/svn/info-cmd.c (revision 1042948)
>>> +++ subversion/svn/info-cmd.c (working copy)
>>> @@ -504,6 +504,7 @@
>>> svn_opt_revision_t peg_revision;
>>> svn_info_receiver_t receiver;
>>> const char *path_prefix;
>>> + char errbuf[256];
>>
>> Please move this variable to the inner scope.
>>
>
> Done!
>
>>>
>>> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
>>> opt_state->targets,
>>> @@ -584,12 +585,9 @@
>>> }
>>> else if (err->apr_err == SVN_ERR_RA_ILLEGAL_URL)
>>> {
>>> - SVN_ERR(svn_cmdline_fprintf
>>> - (stderr, subpool,
>>> - _("%s: (Not a valid URL)\n\n"),
>>> - svn_path_is_url(truepath)
>>> - ? truepath
>>> - : svn_dirent_local_style(truepath, pool)));
>>> + SVN_ERR(svn_cmdline_fprintf(
>>> + stderr, subpool, _("%s\n\n"),
>>
>> You can remove the _() wrapper because this string no longer contains
>> any local-language text to be translated.
>>
>
> Done!
>
>>> + svn_err_best_message(err, errbuf, sizeof(errbuf))));
>>> }
>>
>> I think if we are going to print the error message, we should print it
>> with an "svn: " prefix like we normally do.
>>
>> The old error message included a fixed string (the same for all possible
>> errors), which was helpful for people to recognize it as an error
>> message and for scripts to detect it. The "best message" doesn't have
>> any fixed part. I think adding an "svn: " prefix would enable people
>> and scripts to recognize it.
>
> I think you are right. As of now this is not happening but I added 'svn'
> prefix to the message.
>
> Please find attached the updated patch with review comments incorporated
> and enhancements. All tests pass with this patch.
>
> Log
> [[[
>
> Make "svn info" to display the actual error message returned by API when
> one of the targets is a non-existent URL or wc-entry.
>
> * subversion/svn/info-cmd.c
> (svn_cl__info): Display the actual error message returned by
> svn_client_info3() instead of a custom one. Catch error
> SVN_ERR_WC_PATH_NOT_FOUND instead of SVN_ERR_UNVERSIONED_RESOURCE
> and SVN_ERR_ENTRY_NOT_FOUND for non-existent wc-entry.
>
> * subversion/tests/cmdline/basic_tests.py
> (info_nonexisting_file): Modify test case
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
>
Forgot to attach the patch. Here it is.
Re: Input validation observations - svn info - changing "(Not a valid URL)" to the specific error message
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Re:
> * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> in revision REV"?
>
> On Wed, 2010-12-08, Noorul Islam K M wrote:
> [...]
>> One of the solution that you suggested was to keep what the existing
>> code is doing but saying something more helpful than "Not a valid
>> URL". I thought that the error returned by the API might have useful
>> information and could be printed using svn_err_best_message(). For
>> example, take the following two cases.
>>
>> 1. a) With the patch
>>
>> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
>> ^/non-existing
>> URL 'file:///tmp/latestrepo/non-existing' non-existent in revision 0
>>
>> svn: A problem occurred; see other errors for details
>>
>> 1. b) Without the patch
>>
>> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
>> ^/non-existing
>> file:///tmp/latestrepo/non-existing: (Not a valid URL)
>>
>> svn: A problem occurred; see other errors for details
>>
>> 2. a) With the patch
>>
>> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
>> invalid://no-domain
>> Unrecognized URL scheme for 'invalid://non-domain'
>>
>> svn: A problem occurred; see other errors for details
>>
>> 2. b) Without patch
>>
>> noorul@noorul:/tmp/wc/latestrepo$ ~/projects/subversion/builds/trunk/bin/svn info
>> invalid://no-domain
>> invalid://no-domain: (Not a valid URL)
>>
>> svn: A problem occurred; see other errors for details
>>
>> In both the above cases the patch helps the user to have better
>> information (what actually went wrong). Also If a user is using the
>> client API, I think he will be having these messages than the one that
>> we hard coded as of today.
>
> Thanks for these examples and comments. Yes, I agree the output is much
> more helpful with the patch.
>
> What about the "(Not a versioned resource)" warning? Just above the
> code you changed, there is code to print "(Not a versioned resource)".
> When is that message used? Should we change it in the same way? I
> don't think it makes sense to change one of these and not the other.
>
>
With version 1.7 I am not able to find a scenario for which this
particular block gets executed. With 1.6, if I try to find information
for non-existing wc path, then that particular block gets executed.
Eg:-
With 1.6
$ svn info non-existent
non-existent: (Not a versioned resource)
../subversion/libsvn_client/info.c:266: (apr_err=150000)
svn: 'non-existent' is not under version control
With 1.7 (trunk)
$ svn info non-existent
svn: The node '/tmp/wc/repo1.7/non-existent' was not found.
Another thing that I observed is that when there are multiple targets
specified for info command, 1.6 and 1.7 has behavioral difference.
Eg:-
With 1.6
$ svn info non-existent A
non-existent: (Not a versioned resource)
Path: A
Name: A
URL: file:///tmp/repo1.6/A
Repository Root: file:///tmp/repo1.6
Repository UUID: 99761077-9087-4fca-ba50-95f68245589a
Revision: 1
Node Kind: file
Schedule: normal
Last Changed Author: noorul
Last Changed Rev: 1
Last Changed Date: 2010-12-23 11:40:29 +0530 (Thu, 23 Dec 2010)
Text Last Updated: 2010-12-23 11:40:15 +0530 (Thu, 23 Dec 2010)
Checksum: d41d8cd98f00b204e9800998ecf8427e
../subversion/libsvn_client/info.c:266: (apr_err=150000)
svn: 'non-existent' is not under version control
With 1.7
$ svn info non-existent A
svn: The node '/tmp/wc/repo1.7/non-existent' was not found.
Ideally it should have displayed information for A since that particular
file exists, instead it errors out.
I fixed this by catching the exact error code SVN_ERR_WC_PATH_NOT_FOUND
in this particular scenario. I think we don't have to catch
SVN_ERR_UNVERSIONED_RESOURCE and SVN_ERR_ENTRY_NOT_FOUND any more as
this was the case in 1.6.
>> I ran the test suite and found that one of the test cases was failing
>> and I fixed the same. There were no other failures.
>
> Thanks.
>
> [...]
>> Make "svn info" to display the actual error message returned by API when
>> illegal URL is passed.
>>
>> * subversion/svn/info-cmd.c
>> (svn_cl__info): Display the actual error message returned by
>> svn_client_info3() instead of a custom one.
>>
>> * subversion/tests/cmdline/basic_tests.py
>> (info_nonexisting_file): Modify test case
> [...]
>
>> Index: subversion/tests/cmdline/basic_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/basic_tests.py (revision 1042948)
>> +++ subversion/tests/cmdline/basic_tests.py (working copy)
>> @@ -2270,7 +2270,8 @@
>>
>> # Check for the correct error message
>> for line in errput:
>> - if re.match(".*\(Not a valid URL\).*", line):
>> + if re.match(".*" + idonotexist_url + ".*non-existent in revision 1.*",
>> + line):
>> return
>>
>> # Else never matched the expected error output, so the test failed.
>> Index: subversion/svn/info-cmd.c
>> ===================================================================
>> --- subversion/svn/info-cmd.c (revision 1042948)
>> +++ subversion/svn/info-cmd.c (working copy)
>> @@ -504,6 +504,7 @@
>> svn_opt_revision_t peg_revision;
>> svn_info_receiver_t receiver;
>> const char *path_prefix;
>> + char errbuf[256];
>
> Please move this variable to the inner scope.
>
Done!
>>
>> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
>> opt_state->targets,
>> @@ -584,12 +585,9 @@
>> }
>> else if (err->apr_err == SVN_ERR_RA_ILLEGAL_URL)
>> {
>> - SVN_ERR(svn_cmdline_fprintf
>> - (stderr, subpool,
>> - _("%s: (Not a valid URL)\n\n"),
>> - svn_path_is_url(truepath)
>> - ? truepath
>> - : svn_dirent_local_style(truepath, pool)));
>> + SVN_ERR(svn_cmdline_fprintf(
>> + stderr, subpool, _("%s\n\n"),
>
> You can remove the _() wrapper because this string no longer contains
> any local-language text to be translated.
>
Done!
>> + svn_err_best_message(err, errbuf, sizeof(errbuf))));
>> }
>
> I think if we are going to print the error message, we should print it
> with an "svn: " prefix like we normally do.
>
> The old error message included a fixed string (the same for all possible
> errors), which was helpful for people to recognize it as an error
> message and for scripts to detect it. The "best message" doesn't have
> any fixed part. I think adding an "svn: " prefix would enable people
> and scripts to recognize it.
I think you are right. As of now this is not happening but I added 'svn'
prefix to the message.
Please find attached the updated patch with review comments incorporated
and enhancements. All tests pass with this patch.
Log
[[[
Make "svn info" to display the actual error message returned by API when
one of the targets is a non-existent URL or wc-entry.
* subversion/svn/info-cmd.c
(svn_cl__info): Display the actual error message returned by
svn_client_info3() instead of a custom one. Catch error
SVN_ERR_WC_PATH_NOT_FOUND instead of SVN_ERR_UNVERSIONED_RESOURCE
and SVN_ERR_ENTRY_NOT_FOUND for non-existent wc-entry.
* subversion/tests/cmdline/basic_tests.py
(info_nonexisting_file): Modify test case
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul