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/11/30 11:48:42 UTC
Input validation observations
I tried some potentially invalid inputs to "svn" a week or two ago and
made notes on what I found. Just posting here in case anyone wants to
do something about one or more of them.
Noorul, I'm including you in the "To" addresses because you said you
were looking for more small tasks to do, so feel free to pick one of
these if you want to.
Where I end with a question mark, it means I'm not sure if we want this
change, it's just a suggestion.
* "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
WC path".
* "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
* "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
in lib; should reject them at CLI level?
* "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
in revision REV"?
* "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
mix URL and local targets"?
* "svn mkdir a ^/" -> "Can't create directory
'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
a local path.
* "svn mv ^/ ^/" -> "Cannot move path
'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
as if it's a local path.
* "svn update ^/a" -> "Skipped
'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
WC path"?
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Mon, 2010-12-13, Hyrum K. Wright wrote:
>
>> Random community nit: The attached log message seems to have little to
>> do with email subject of "Input validation observations".
>
> FYI, I'm watching this thread and intend to respond to this patch, which
> is one of a series of patches responding to the observations mentioned
> in the subject line. But yes, I see what you mean - the subject line no
> longer describes the content of these messages well and so it's not easy
> for others to follow.
>
>> In the
>> future, when sending a new patch to the list, I'd recommend starting a
>> new thread, complete with it's own "[PATCH] ..." subject, as
>> recommended in the patch submission guidelines. It's more likely to
>> get noticed than when it's tacked onto the end of a largely-unrelated
>> 40-message thread.
>
> Agreed. Or, in a case like this where the patch is a direct response to
> the original thread, I think replying within the thread and changing the
> Subject line would be fine. (That would be seen as a new thread by some
> mail clients and as part of the same thread by others, but has the
> required effect in either case and is simple to do.)
>
>> Anyway, thanks for the work, I just don't want it to get lost. :)
>
> Yup, thanks from me too.
I assumed that these were going to be reviewed by Julian. I will keep
all these points in mind next time.
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-12-13, Hyrum K. Wright wrote:
> Random community nit: The attached log message seems to have little to
> do with email subject of "Input validation observations".
FYI, I'm watching this thread and intend to respond to this patch, which
is one of a series of patches responding to the observations mentioned
in the subject line. But yes, I see what you mean - the subject line no
longer describes the content of these messages well and so it's not easy
for others to follow.
> In the
> future, when sending a new patch to the list, I'd recommend starting a
> new thread, complete with it's own "[PATCH] ..." subject, as
> recommended in the patch submission guidelines. It's more likely to
> get noticed than when it's tacked onto the end of a largely-unrelated
> 40-message thread.
Agreed. Or, in a case like this where the patch is a direct response to
the original thread, I think replying within the thread and changing the
Subject line would be fine. (That would be seen as a new thread by some
mail clients and as part of the same thread by others, but has the
required effect in either case and is simple to do.)
> Anyway, thanks for the work, I just don't want it to get lost. :)
Yup, thanks from me too.
- Julian
Re: Input validation observations
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Random community nit: The attached log message seems to have little to
do with email subject of "Input validation observations". In the
future, when sending a new patch to the list, I'd recommend starting a
new thread, complete with it's own "[PATCH] ..." subject, as
recommended in the patch submission guidelines. It's more likely to
get noticed than when it's tacked onto the end of a largely-unrelated
40-message thread.
Anyway, thanks for the work, I just don't want it to get lost. :)
-Hyrum
On Mon, Dec 13, 2010 at 9:05 AM, noorul Islam. Kamal Malmiyoda
<no...@collab.net> wrote:
> I am top posting since my mails are not getting through with my mail client.
> I am using web based client which lacks quoting feature.
>
> This is in response to
>
> * "svn mv ^/ ^/" -> "Cannot move path
> 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
> as if it's a local path.
>
> Attached is the patch which improves the error message. Also added missing
> test. All the tests pass with 'make check'.
>
> Log
>
> [[[
> Make 'svn move' display correct error message while moving any
> path/URL onto or into itself. Add missing test.
>
> * subversion/libsvn_client/copy.c
> (try_copy): Display error message based on source type,
> (repos_to_repos_copy): Remove redundant code.
>
> * subversion/tests/cmdline/copy_tests.py
> (move_wc_and_repo_dir_to_itself, test_list): New test
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
>
> Thanks and Regards
> Noorul
>
> -----Original Message-----
> From: Julian Foad [mailto:julian.foad@wandisco.com]
> Sent: Tue 11/30/2010 5:18 PM
> To: Subversion Development; noorul Islam. Kamal Malmiyoda
> Subject: Input validation observations
>
> I tried some potentially invalid inputs to "svn" a week or two ago and
> made notes on what I found. Just posting here in case anyone wants to
> do something about one or more of them.
>
> Noorul, I'm including you in the "To" addresses because you said you
> were looking for more small tasks to do, so feel free to pick one of
> these if you want to.
>
> Where I end with a question mark, it means I'm not sure if we want this
> change, it's just a suggestion.
>
> * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
> try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
> ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
> WC path".
>
> * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
> 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
> Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
>
> * "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
> in lib; should reject them at CLI level?
>
> * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> in revision REV"?
>
> * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> mix URL and local targets"?
>
> * "svn mkdir a ^/" -> "Can't create directory
> 'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
> a local path.
>
> * "svn mv ^/ ^/" -> "Cannot move path
> 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
> as if it's a local path.
>
> * "svn update ^/a" -> "Skipped
> 'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
> WC path"?
>
>
> - Julian
>
>
>
>
>
>
RE: Input validation observations
Posted by "noorul Islam. Kamal Malmiyoda" <no...@collab.net>.
I am top posting since my mails are not getting through with my mail client. I am using web based client which lacks quoting feature.
This is in response to
* "svn mv ^/ ^/" -> "Cannot move path
'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
as if it's a local path.
Attached is the patch which improves the error message. Also added missing test. All the tests pass with 'make check'.
Log
[[[
Make 'svn move' display correct error message while moving any
path/URL onto or into itself. Add missing test.
* subversion/libsvn_client/copy.c
(try_copy): Display error message based on source type,
(repos_to_repos_copy): Remove redundant code.
* subversion/tests/cmdline/copy_tests.py
(move_wc_and_repo_dir_to_itself, test_list): New test
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
-----Original Message-----
From: Julian Foad [mailto:julian.foad@wandisco.com]
Sent: Tue 11/30/2010 5:18 PM
To: Subversion Development; noorul Islam. Kamal Malmiyoda
Subject: Input validation observations
I tried some potentially invalid inputs to "svn" a week or two ago and
made notes on what I found. Just posting here in case anyone wants to
do something about one or more of them.
Noorul, I'm including you in the "To" addresses because you said you
were looking for more small tasks to do, so feel free to pick one of
these if you want to.
Where I end with a question mark, it means I'm not sure if we want this
change, it's just a suggestion.
* "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
WC path".
* "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
* "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
in lib; should reject them at CLI level?
* "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
in revision REV"?
* "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
mix URL and local targets"?
* "svn mkdir a ^/" -> "Can't create directory
'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
a local path.
* "svn mv ^/ ^/" -> "Cannot move path
'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
as if it's a local path.
* "svn update ^/a" -> "Skipped
'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
WC path"?
- Julian
Re: Input validation observations
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/03/2010 06:51 AM, Julian Foad wrote:
> Noorul Islam K M wrote:
>> Julian Foad <ju...@wandisco.com> writes:
[...]
>> Index: subversion/svn/info-cmd.c
>> ===================================================================
>> --- subversion/svn/info-cmd.c (revision 1041293)
>> +++ subversion/svn/info-cmd.c (working copy)
>> @@ -584,12 +584,8 @@
>> }
>> 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"), err->message));
>
> Unfortunately we cannot assume that err->message is a good user-friendly
> description of the problem.
I've tweaked the documentation for svn_error_t in r1041891 to hopefully
point folks away from directly accessing err->message (among other things).
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
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
Re: Input validation observations - svn info - changing "(Not a
valid URL)" to the specific error message
Posted by Julian Foad <ju...@wandisco.com>.
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
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-12-15, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > Noorul Islam K M wrote:
> >
> >> Julian Foad <ju...@wandisco.com> writes:
> >> > Thank you for the updated patch. Even though this is a very
> >> > simple-looking patch, I need a bit more information to help me review
> >> > it.
> >> >
> >> > Do you think both of the options I suggested are possible solutions?
> >> > What are the advantages of the option you chose? What disadvantages do
> >> > you know about? What are the risks with it - in what ways could it
> >> > possibly go wrong or make a user unhappy? For example, what other error
> >> > conditions might this code possibly handle? Which of them did you try?
> >> > Show us the results. Do you think they are user-friendly?
> >>
> >> 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.
> >>
> >> 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.
> >
> >
> > Hi Noorul.
> >
> > Thank you very much for this extra information. It makes my job as a
> > reviewer very much easier.
> >
> > I haven't reviewed it yet but I will get back to it soon.
> >
> >
>
> Just pinging to remind you since this is a huge thread.
Thanks for the ping.
If anybody else wants to take this, please do. Otherwise, I have it
marked for attention and will get to it some time.
Same for the 'move_to_self_error.diff' patch in this thread.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>> > Thank you for the updated patch. Even though this is a very
>> > simple-looking patch, I need a bit more information to help me review
>> > it.
>> >
>> > Do you think both of the options I suggested are possible solutions?
>> > What are the advantages of the option you chose? What disadvantages do
>> > you know about? What are the risks with it - in what ways could it
>> > possibly go wrong or make a user unhappy? For example, what other error
>> > conditions might this code possibly handle? Which of them did you try?
>> > Show us the results. Do you think they are user-friendly?
>>
>> 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.
>>
>> 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.
>
>
> Hi Noorul.
>
> Thank you very much for this extra information. It makes my job as a
> reviewer very much easier.
>
> I haven't reviewed it yet but I will get back to it soon.
>
>
Just pinging to remind you since this is a huge thread.
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > Thank you for the updated patch. Even though this is a very
> > simple-looking patch, I need a bit more information to help me review
> > it.
> >
> > Do you think both of the options I suggested are possible solutions?
> > What are the advantages of the option you chose? What disadvantages do
> > you know about? What are the risks with it - in what ways could it
> > possibly go wrong or make a user unhappy? For example, what other error
> > conditions might this code possibly handle? Which of them did you try?
> > Show us the results. Do you think they are user-friendly?
>
> 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.
>
> 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.
Hi Noorul.
Thank you very much for this extra information. It makes my job as a
reviewer very much easier.
I haven't reviewed it yet but I will get back to it soon.
> > Checking those sorts of things normally takes much more effort than
> > actually writing the few lines of source code. That's all part of
> > making a patch. Whenever you send a patch, it's helpful to say these
> > things. When the reviewer knows what's already been checked, he or she
> > can then concentrate on verifying the correctness of the reasoning and
> > of the code.
> >
> > Please take a little extra time to provide this help.
>
> I will keep this in mind.
Thank you very much. I really appreciate it.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Sat, 2010-12-04, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>> > Noorul Islam K M wrote:
>> >> Julian Foad <ju...@wandisco.com> writes:
>> >> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
>> >> > in revision REV"?
> [...]
>> >> - 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"), err->message));
>> >
>> > Unfortunately we cannot assume that err->message is a good user-friendly
>> > description of the problem. It appears that it *is* in the specific
>> > case we're looking at:
>> >
>> > $ svn info ^/b
>> > URL 'https://svn.apache.org/repos/asf/b' non-existent in revision
>> > 1041775
>> >
>> > That's lovely. But in other cases it's not:
>> >
>> > $ svn info hhh://aa.cc.dd/foo
>> > traced call
>> >
>> > See how err->message is just "traced call" in this case. We can't even
>> > assume it is not NULL, in fact.
>> >
>> > I can think of two options. We could try to use one of the
>> > svn_error_*() functions to print out a "nice" description of the actual
>> > returned error. Alternatively, we could ignore 'err' and print a simple
>> > message here, like the existing code is doing but saying something more
>> > helpful than "Not a valid URL". What do you think?
>> >
>> > Does the documentation for svn_client_info3() say under what conditions
>> > it returns the error code SVN_ERR_RA_ILLEGAL_URL? Does that help?
>>
>> Attached is the updated patch using svn_err_best_message()
>
> Hi Noorul.
>
> Thank you for the updated patch. Even though this is a very
> simple-looking patch, I need a bit more information to help me review
> it.
>
> Do you think both of the options I suggested are possible solutions?
> What are the advantages of the option you chose? What disadvantages do
> you know about? What are the risks with it - in what ways could it
> possibly go wrong or make a user unhappy? For example, what other error
> conditions might this code possibly handle? Which of them did you try?
> Show us the results. Do you think they are user-friendly?
>
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.
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.
> Checking those sorts of things normally takes much more effort than
> actually writing the few lines of source code. That's all part of
> making a patch. Whenever you send a patch, it's helpful to say these
> things. When the reviewer knows what's already been checked, he or she
> can then concentrate on verifying the correctness of the reasoning and
> of the code.
>
> Please take a little extra time to provide this help.
I will keep this in mind.
Please find attached the updated patch.
Log
[[[
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
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Sat, 2010-12-04, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > Noorul Islam K M wrote:
> >> Julian Foad <ju...@wandisco.com> writes:
> >> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> >> > in revision REV"?
[...]
> >> - 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"), err->message));
> >
> > Unfortunately we cannot assume that err->message is a good user-friendly
> > description of the problem. It appears that it *is* in the specific
> > case we're looking at:
> >
> > $ svn info ^/b
> > URL 'https://svn.apache.org/repos/asf/b' non-existent in revision
> > 1041775
> >
> > That's lovely. But in other cases it's not:
> >
> > $ svn info hhh://aa.cc.dd/foo
> > traced call
> >
> > See how err->message is just "traced call" in this case. We can't even
> > assume it is not NULL, in fact.
> >
> > I can think of two options. We could try to use one of the
> > svn_error_*() functions to print out a "nice" description of the actual
> > returned error. Alternatively, we could ignore 'err' and print a simple
> > message here, like the existing code is doing but saying something more
> > helpful than "Not a valid URL". What do you think?
> >
> > Does the documentation for svn_client_info3() say under what conditions
> > it returns the error code SVN_ERR_RA_ILLEGAL_URL? Does that help?
>
> Attached is the updated patch using svn_err_best_message()
Hi Noorul.
Thank you for the updated patch. Even though this is a very
simple-looking patch, I need a bit more information to help me review
it.
Do you think both of the options I suggested are possible solutions?
What are the advantages of the option you chose? What disadvantages do
you know about? What are the risks with it - in what ways could it
possibly go wrong or make a user unhappy? For example, what other error
conditions might this code possibly handle? Which of them did you try?
Show us the results. Do you think they are user-friendly?
Checking those sorts of things normally takes much more effort than
actually writing the few lines of source code. That's all part of
making a patch. Whenever you send a patch, it's helpful to say these
things. When the reviewer knows what's already been checked, he or she
can then concentrate on verifying the correctness of the reasoning and
of the code.
Please take a little extra time to provide this help.
Thank you in advance.
- Julian
> - 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"),
> + svn_err_best_message(err, errbuf, sizeof(errbuf))));
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
>> > in revision REV"?
>>
>> Patch attached.
>
> Hi Noorul.
>
>> 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.
> [...]
>> Index: subversion/svn/info-cmd.c
>> ===================================================================
>> --- subversion/svn/info-cmd.c (revision 1041293)
>> +++ subversion/svn/info-cmd.c (working copy)
>> @@ -584,12 +584,8 @@
>> }
>> 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"), err->message));
>
> Unfortunately we cannot assume that err->message is a good user-friendly
> description of the problem. It appears that it *is* in the specific
> case we're looking at:
>
> $ svn info ^/b
> URL 'https://svn.apache.org/repos/asf/b' non-existent in revision
> 1041775
>
> That's lovely. But in other cases it's not:
>
> $ svn info hhh://aa.cc.dd/foo
> traced call
>
> See how err->message is just "traced call" in this case. We can't even
> assume it is not NULL, in fact.
>
> I can think of two options. We could try to use one of the
> svn_error_*() functions to print out a "nice" description of the actual
> returned error. Alternatively, we could ignore 'err' and print a simple
> message here, like the existing code is doing but saying something more
> helpful than "Not a valid URL". What do you think?
>
> Does the documentation for svn_client_info3() say under what conditions
> it returns the error code SVN_ERR_RA_ILLEGAL_URL? Does that help?
>
Attached is the updated patch using svn_err_best_message()
Log
[[[
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.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> > in revision REV"?
>
> Patch attached.
Hi Noorul.
> 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.
[...]
> Index: subversion/svn/info-cmd.c
> ===================================================================
> --- subversion/svn/info-cmd.c (revision 1041293)
> +++ subversion/svn/info-cmd.c (working copy)
> @@ -584,12 +584,8 @@
> }
> 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"), err->message));
Unfortunately we cannot assume that err->message is a good user-friendly
description of the problem. It appears that it *is* in the specific
case we're looking at:
$ svn info ^/b
URL 'https://svn.apache.org/repos/asf/b' non-existent in revision
1041775
That's lovely. But in other cases it's not:
$ svn info hhh://aa.cc.dd/foo
traced call
See how err->message is just "traced call" in this case. We can't even
assume it is not NULL, in fact.
I can think of two options. We could try to use one of the
svn_error_*() functions to print out a "nice" description of the actual
returned error. Alternatively, we could ignore 'err' and print a simple
message here, like the existing code is doing but saying something more
helpful than "Not a valid URL". What do you think?
Does the documentation for svn_client_info3() say under what conditions
it returns the error code SVN_ERR_RA_ILLEGAL_URL? Does that help?
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> I tried some potentially invalid inputs to "svn" a week or two ago and
> made notes on what I found. Just posting here in case anyone wants to
> do something about one or more of them.
>
> Noorul, I'm including you in the "To" addresses because you said you
> were looking for more small tasks to do, so feel free to pick one of
> these if you want to.
>
> Where I end with a question mark, it means I'm not sure if we want this
> change, it's just a suggestion.
>
>
> * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> in revision REV"?
>
Patch attached.
Log
[[[
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.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-12-13, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> >
> > * "svn mv ^/ ^/" -> "Cannot move path
> > 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
> > as if it's a local path.
>
> Attached is the patch which improves error message displayed.
> [[[
> Make 'svn move' display correct error message while moving any
> path/URL onto or into itself. Add missing tests.
>
> * subversion/libsvn_client/copy.c
> (try_copy): Display error message based on source type,
> (repos_to_repos_copy): Remove redundant code.
>
> * subversion/tests/cmdline/copy_tests.py
> (move_wc_and_repo_dir_to_itself, test_list): New test
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
Committed revision 1051059. Thanks.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
>
> * "svn mv ^/ ^/" -> "Cannot move path
> 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
> as if it's a local path.
>
Attached is the patch which improves error message displayed.
Log
[[[
Make 'svn move' display correct error message while moving any
path/URL onto or into itself. Add missing tests.
* subversion/libsvn_client/copy.c
(try_copy): Display error message based on source type,
(repos_to_repos_copy): Remove redundant code.
* subversion/tests/cmdline/copy_tests.py
(move_wc_and_repo_dir_to_itself, test_list): New test
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2010-12-02 at 14:05 +0530, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > I tried some potentially invalid inputs to "svn" a week or two ago and
> > made notes on what I found. Just posting here in case anyone wants to
> > do something about one or more of them.
> >
> > Noorul, I'm including you in the "To" addresses because you said you
> > were looking for more small tasks to do, so feel free to pick one of
> > these if you want to.
> >
> > Where I end with a question mark, it means I'm not sure if we want this
> > change, it's just a suggestion.
> >
> > * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
> > try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
> > ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
> > WC path".
> >
> > * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
> > 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
> > Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
> >
>
> In this case code assumes that the user is trying to checkout from both
> source into current directory. i.e, If all source targets are URLs then
> the checkout target becomes "."
Oh yes, you're right - there's no problem there. I didn't notice that
"svn checkout URL URL" was a supported syntax. Thanks.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> I tried some potentially invalid inputs to "svn" a week or two ago and
> made notes on what I found. Just posting here in case anyone wants to
> do something about one or more of them.
>
> Noorul, I'm including you in the "To" addresses because you said you
> were looking for more small tasks to do, so feel free to pick one of
> these if you want to.
>
> Where I end with a question mark, it means I'm not sure if we want this
> change, it's just a suggestion.
>
> * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
> try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
> ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
> WC path".
>
> * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
> 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
> Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
>
In this case code assumes that the user is trying to checkout from both
source into current directory. i.e, If all source targets are URLs then
the checkout target becomes "."
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
Noorul Islam K M wrote:
> Ran test suite and found some failures and fixed them. All of them were
> passing URL to 'svn up' which is not at all required. Now all tests are
> passing with the below attached patch.
Thanks, Noorul. That's lovely. Committed revision 1043409.
- Julian
> Log
> [[[
>
> Make 'svn update' verify that URLs are not passed as targets.
>
> * subversion/svn/update-cmd.c,
> subversion/libsvn_client/update.c:
> (svn_cl__update, svn_client_update4): Raise an error if a URL was
> passed. Remove code that notifies 'Skipped' message for URL targets.
>
> * subversion/tests/cmdline/input_validation_tests.py
> (invalid_update_targets, test_list): New test
>
> * subversion/tests/cmdline/externals_tests.py,
> subversion/tests/cmdline/svntest/actions.py,
> (cannot_move_or_remove_file_externals,
> can_place_file_external_into_dir_external,
> external_into_path_with_spaces,
> inject_conflict_into_wc): Remove URL target from 'svn up' command.
>
> * subversion/tests/cmdline/basic_tests.py
> (basic_update): URLs are no longer accepted as targets. Remove test
> case for 'Skipped' message.
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Julian Foad <ju...@wandisco.com> writes:
>
>> On Sat, 2010-12-04, Noorul Islam K M wrote:
>>
>>> Julian Foad <ju...@wandisco.com> writes:
>>> > On Fri, 2010-12-03, Noorul Islam K M wrote:
>>> >> Julian Foad <ju...@wandisco.com> writes:
>>> >> > I think we should change this behaviour and make "svn update" throw an
>>> >> > error if any target is a URL.
>>> >>
>>> >> Attached is the patch for same.
>>> > [...]
>>> >> Make 'svn update' verify that URLs are not passed as targets.
>>> >>
>>> >> * subversion/svn/update-cmd.c,
>>> >> subversion/libsvn_client/update.c:
>>> >> (svn_cl__update, svn_client_update4): Raise an error if a URL was
>>> >> passed. Remove code that notifies 'Skipped' message for URL targets.
>>> > [...]
>>
>>> Please find attached the updated patch.
>>
>> Hi Noorul.
>>
>> Several tests fail with this patch.
>>
>> FAIL: basic_tests.py 4: basic update command
>> FAIL: commit_tests.py 56: 'svn commit --changelist=foo' above a
>> conflict
>> FAIL: externals_tests.py 15: should not be able to mv or rm a file
>> external
>> FAIL: externals_tests.py 16: place a file external into a directory
>> external
>> [...]
>>
>> Before you send a patch, always run the test suite ("make check" or
>> similar) and check that all tests pass. If you have done no testing or
>> different testing or you found failures but want to send the patch
>> anyway, then simply say so in the email, so we know. If you don't say
>> anything, we will assume you ran the test suite and all tests pass.
>>
>
> I will keep this in mind. I actually added a test case for this as part
> of the patch. My mistake to not run the entire test suite. I will send
> follow-up patch with modified test cases.
>
Ran test suite and found some failures and fixed them. All of them were
passing URL to 'svn up' which is not at all required. Now all tests are
passing with the below attached patch.
Log
[[[
Make 'svn update' verify that URLs are not passed as targets.
* subversion/svn/update-cmd.c,
subversion/libsvn_client/update.c:
(svn_cl__update, svn_client_update4): Raise an error if a URL was
passed. Remove code that notifies 'Skipped' message for URL targets.
* subversion/tests/cmdline/input_validation_tests.py
(invalid_update_targets, test_list): New test
* subversion/tests/cmdline/externals_tests.py,
subversion/tests/cmdline/svntest/actions.py,
(cannot_move_or_remove_file_externals,
can_place_file_external_into_dir_external,
external_into_path_with_spaces,
inject_conflict_into_wc): Remove URL target from 'svn up' command.
* subversion/tests/cmdline/basic_tests.py
(basic_update): URLs are no longer accepted as targets. Remove test
case for 'Skipped' message.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Sat, 2010-12-04, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>> > On Fri, 2010-12-03, Noorul Islam K M wrote:
>> >> Julian Foad <ju...@wandisco.com> writes:
>> >> > I think we should change this behaviour and make "svn update" throw an
>> >> > error if any target is a URL.
>> >>
>> >> Attached is the patch for same.
>> > [...]
>> >> Make 'svn update' verify that URLs are not passed as targets.
>> >>
>> >> * subversion/svn/update-cmd.c,
>> >> subversion/libsvn_client/update.c:
>> >> (svn_cl__update, svn_client_update4): Raise an error if a URL was
>> >> passed. Remove code that notifies 'Skipped' message for URL targets.
>> > [...]
>
>> Please find attached the updated patch.
>
> Hi Noorul.
>
> Several tests fail with this patch.
>
> FAIL: basic_tests.py 4: basic update command
> FAIL: commit_tests.py 56: 'svn commit --changelist=foo' above a
> conflict
> FAIL: externals_tests.py 15: should not be able to mv or rm a file
> external
> FAIL: externals_tests.py 16: place a file external into a directory
> external
> [...]
>
> Before you send a patch, always run the test suite ("make check" or
> similar) and check that all tests pass. If you have done no testing or
> different testing or you found failures but want to send the patch
> anyway, then simply say so in the email, so we know. If you don't say
> anything, we will assume you ran the test suite and all tests pass.
>
I will keep this in mind. I actually added a test case for this as part
of the patch. My mistake to not run the entire test suite. I will send
follow-up patch with modified test cases.
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Sat, 2010-12-04, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > On Fri, 2010-12-03, Noorul Islam K M wrote:
> >> Julian Foad <ju...@wandisco.com> writes:
> >> > I think we should change this behaviour and make "svn update" throw an
> >> > error if any target is a URL.
> >>
> >> Attached is the patch for same.
> > [...]
> >> Make 'svn update' verify that URLs are not passed as targets.
> >>
> >> * subversion/svn/update-cmd.c,
> >> subversion/libsvn_client/update.c:
> >> (svn_cl__update, svn_client_update4): Raise an error if a URL was
> >> passed. Remove code that notifies 'Skipped' message for URL targets.
> > [...]
> Please find attached the updated patch.
Hi Noorul.
Several tests fail with this patch.
FAIL: basic_tests.py 4: basic update command
FAIL: commit_tests.py 56: 'svn commit --changelist=foo' above a
conflict
FAIL: externals_tests.py 15: should not be able to mv or rm a file
external
FAIL: externals_tests.py 16: place a file external into a directory
external
[...]
Before you send a patch, always run the test suite ("make check" or
similar) and check that all tests pass. If you have done no testing or
different testing or you found failures but want to send the patch
anyway, then simply say so in the email, so we know. If you don't say
anything, we will assume you ran the test suite and all tests pass.
Thank you.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Fri, 2010-12-03, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>> > I think we should change this behaviour and make "svn update" throw an
>> > error if any target is a URL.
>>
>> Attached is the patch for same.
> [...]
>> Make 'svn update' verify that URLs are not passed as targets.
>>
>> * subversion/svn/update-cmd.c,
>> subversion/libsvn_client/update.c:
>> (svn_cl__update, svn_client_update4): Raise an error if a URL was
>> passed. Remove code that notifies 'Skipped' message for URL targets.
> [...]
>> Index: subversion/libsvn_client/update.c
>> ===================================================================
>> --- subversion/libsvn_client/update.c (revision 1041293)
>> +++ subversion/libsvn_client/update.c (working copy)
>> @@ -397,44 +397,45 @@
>>
>> for (i = 0; i < paths->nelts; ++i)
>> {
>> + path = APR_ARRAY_IDX(paths, i, const char *);
>> +
>> + if (svn_path_is_url(path))
>> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
>> + _("'%s' is not a local path"), path);
>> + }
>> +
>> + for (i = 0; i < paths->nelts; ++i)
>> + {
>> svn_boolean_t sleep;
>> svn_boolean_t skipped = FALSE;
>> svn_error_t *err = SVN_NO_ERROR;
>> svn_revnum_t result_rev;
>> + const char *local_abspath;
>> path = APR_ARRAY_IDX(paths, i, const char *);
>>
>> svn_pool_clear(subpool);
>>
>> if (ctx->cancel_func && (err = ctx->cancel_func(ctx->cancel_baton)))
>> break;
>> -
>> - if (svn_path_is_url(path))
>> +
>> + SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, subpool));
>> + err = svn_client__update_internal(&result_rev, local_abspath,
>> + revision, depth, depth_is_sticky,
>> + ignore_externals,
>> + allow_unver_obstructions,
>> + &sleep, FALSE, make_parents,
>> + ctx, subpool);
>> +
>> + if (err && err->apr_err != SVN_ERR_WC_NOT_WORKING_COPY)
>> {
>> - skipped = TRUE;
>
> Hi Noorul.
>
> Having removed this "skipped = TRUE" statement, the only remaining use
> of the "skipped" variable is ...
>
>> + return svn_error_return(err);
>> }
>> - else
>> +
>> + if (err)
>> {
>> - const char *local_abspath;
>> -
>> - SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, subpool));
>> - err = svn_client__update_internal(&result_rev, local_abspath,
>> - revision, depth, depth_is_sticky,
>> - ignore_externals,
>> - allow_unver_obstructions,
>> - &sleep, FALSE, make_parents,
>> - ctx, subpool);
>> -
>> - if (err && err->apr_err != SVN_ERR_WC_NOT_WORKING_COPY)
>> - {
>> - return svn_error_return(err);
>> - }
>> -
>> - if (err)
>> - {
>> - /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
>> - svn_error_clear(err);
>> - skipped = TRUE;
>> - }
>> + /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
>> + svn_error_clear(err);
>> + skipped = TRUE;
>
> ... here ...
>
>> }
>>
>> if (skipped)
>
> ... and here. So you can eliminate the variable and join the two blocks
> together. That would be simpler.
>
> The patch looks functionally correct.
>
> Thanks.
>
> - Julian
>
>
>> @@ -443,22 +444,9 @@
>> if (ctx->notify_func2)
>> {
>> svn_wc_notify_t *notify;
>> -
>> - if (svn_path_is_url(path))
>> - {
>> - /* For some historic reason this user error is supported,
>> - and must provide correct notifications. */
>> - notify = svn_wc_create_notify_url(path,
>> - svn_wc_notify_skip,
>> - subpool);
>> - }
>> - else
>> - {
>> - notify = svn_wc_create_notify(path,
>> - svn_wc_notify_skip,
>> - subpool);
>> - }
>> -
>> + notify = svn_wc_create_notify(path,
>> + svn_wc_notify_skip,
>> + subpool);
>> (*ctx->notify_func2)(ctx->notify_baton2, notify, subpool);
>> }
>> }
Please find attached the updated patch.
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-12-03, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > I think we should change this behaviour and make "svn update" throw an
> > error if any target is a URL.
>
> Attached is the patch for same.
[...]
> Make 'svn update' verify that URLs are not passed as targets.
>
> * subversion/svn/update-cmd.c,
> subversion/libsvn_client/update.c:
> (svn_cl__update, svn_client_update4): Raise an error if a URL was
> passed. Remove code that notifies 'Skipped' message for URL targets.
[...]
> Index: subversion/libsvn_client/update.c
> ===================================================================
> --- subversion/libsvn_client/update.c (revision 1041293)
> +++ subversion/libsvn_client/update.c (working copy)
> @@ -397,44 +397,45 @@
>
> for (i = 0; i < paths->nelts; ++i)
> {
> + path = APR_ARRAY_IDX(paths, i, const char *);
> +
> + if (svn_path_is_url(path))
> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("'%s' is not a local path"), path);
> + }
> +
> + for (i = 0; i < paths->nelts; ++i)
> + {
> svn_boolean_t sleep;
> svn_boolean_t skipped = FALSE;
> svn_error_t *err = SVN_NO_ERROR;
> svn_revnum_t result_rev;
> + const char *local_abspath;
> path = APR_ARRAY_IDX(paths, i, const char *);
>
> svn_pool_clear(subpool);
>
> if (ctx->cancel_func && (err = ctx->cancel_func(ctx->cancel_baton)))
> break;
> -
> - if (svn_path_is_url(path))
> +
> + SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, subpool));
> + err = svn_client__update_internal(&result_rev, local_abspath,
> + revision, depth, depth_is_sticky,
> + ignore_externals,
> + allow_unver_obstructions,
> + &sleep, FALSE, make_parents,
> + ctx, subpool);
> +
> + if (err && err->apr_err != SVN_ERR_WC_NOT_WORKING_COPY)
> {
> - skipped = TRUE;
Hi Noorul.
Having removed this "skipped = TRUE" statement, the only remaining use
of the "skipped" variable is ...
> + return svn_error_return(err);
> }
> - else
> +
> + if (err)
> {
> - const char *local_abspath;
> -
> - SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, subpool));
> - err = svn_client__update_internal(&result_rev, local_abspath,
> - revision, depth, depth_is_sticky,
> - ignore_externals,
> - allow_unver_obstructions,
> - &sleep, FALSE, make_parents,
> - ctx, subpool);
> -
> - if (err && err->apr_err != SVN_ERR_WC_NOT_WORKING_COPY)
> - {
> - return svn_error_return(err);
> - }
> -
> - if (err)
> - {
> - /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
> - svn_error_clear(err);
> - skipped = TRUE;
> - }
> + /* SVN_ERR_WC_NOT_WORKING_COPY: it's not versioned */
> + svn_error_clear(err);
> + skipped = TRUE;
... here ...
> }
>
> if (skipped)
... and here. So you can eliminate the variable and join the two blocks
together. That would be simpler.
The patch looks functionally correct.
Thanks.
- Julian
> @@ -443,22 +444,9 @@
> if (ctx->notify_func2)
> {
> svn_wc_notify_t *notify;
> -
> - if (svn_path_is_url(path))
> - {
> - /* For some historic reason this user error is supported,
> - and must provide correct notifications. */
> - notify = svn_wc_create_notify_url(path,
> - svn_wc_notify_skip,
> - subpool);
> - }
> - else
> - {
> - notify = svn_wc_create_notify(path,
> - svn_wc_notify_skip,
> - subpool);
> - }
> -
> + notify = svn_wc_create_notify(path,
> + svn_wc_notify_skip,
> + subpool);
> (*ctx->notify_func2)(ctx->notify_baton2, notify, subpool);
> }
> }
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Thu, 2010-12-02 at 13:58 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@btopenworld.com> writes:
>>
>> > On Tue, 2010-11-30 at 18:42 +0530, Noorul Islam K M wrote:
>> >
>> >> Julian Foad <ju...@wandisco.com> writes:
>> >>
>> >> > I tried some potentially invalid inputs to "svn" a week or two ago and
>> >> > made notes on what I found. Just posting here in case anyone wants to
>> >> > do something about one or more of them.
>> >> >
>> >> > Noorul, I'm including you in the "To" addresses because you said you
>> >> > were looking for more small tasks to do, so feel free to pick one of
>> >> > these if you want to.
>> >>
>> >> Thank you! I will start working on these one by one.
>> >
>> > Great!
>> >
>> >
>> >> > Where I end with a question mark, it means I'm not sure if we want this
>> >> > change, it's just a suggestion.
>> >> >
>> >> > * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
>> >> > try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
>> >> > ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
>> >> > WC path".
>> >> >
>> >> > * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
>> >> > 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
>> >> > Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
>> >> >
>> >> > * "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
>> >> > in lib; should reject them at CLI level?
>> >> >
>> >> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
>> >> > in revision REV"?
>> >> >
>> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>> >> > mix URL and local targets"?
>> >> >
>> >> > * "svn mkdir a ^/" -> "Can't create directory
>> >> > 'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
>> >> > a local path.
>> >> >
>> >> > * "svn mv ^/ ^/" -> "Cannot move path
>> >> > 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
>> >> > as if it's a local path.
>> >> >
>> >> > * "svn update ^/a" -> "Skipped
>> >> > 'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
>> >> > WC path"?
>> >> >
>> >>
>> >> svn help update says that the argument should be a PATH. I think it is
>> >> right to force the user to enter local PATH.
>> >
>> > Good. Thanks for finding that. I agree. That change will make some of
>> > my recent patch (r1040232) redundant: it will no longer need to remove
>> > URL targets from the array of targets, since it will just return an
>> > error if it finds any, like you already did in some of the other
>> > subcomands.
>> >
>>
>> I further looked into the code, since update accepts multiple targets,
>> the program skips URL targets assuming that there might one or more
>> local paths as part of targets list. The skipping part is intentional
>> and I am not sure whether we should change this behavior.
>
> I think we should change this behaviour and make "svn update" throw an
> error if any target is a URL.
>
Attached is the patch for same.
Log
[[[
Make 'svn update' verify that URLs are not passed as targets.
* subversion/svn/update-cmd.c,
subversion/libsvn_client/update.c:
(svn_cl__update, svn_client_update4): Raise an error if a URL was
passed. Remove code that notifies 'Skipped' message for URL targets.
* subversion/tests/cmdline/input_validation_tests.py
(invalid_update_targets, test_list): New test
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2010-12-02 at 13:58 +0530, Noorul Islam K M wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
> > On Tue, 2010-11-30 at 18:42 +0530, Noorul Islam K M wrote:
> >
> >> Julian Foad <ju...@wandisco.com> writes:
> >>
> >> > I tried some potentially invalid inputs to "svn" a week or two ago and
> >> > made notes on what I found. Just posting here in case anyone wants to
> >> > do something about one or more of them.
> >> >
> >> > Noorul, I'm including you in the "To" addresses because you said you
> >> > were looking for more small tasks to do, so feel free to pick one of
> >> > these if you want to.
> >>
> >> Thank you! I will start working on these one by one.
> >
> > Great!
> >
> >
> >> > Where I end with a question mark, it means I'm not sure if we want this
> >> > change, it's just a suggestion.
> >> >
> >> > * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
> >> > try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
> >> > ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
> >> > WC path".
> >> >
> >> > * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
> >> > 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
> >> > Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
> >> >
> >> > * "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
> >> > in lib; should reject them at CLI level?
> >> >
> >> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> >> > in revision REV"?
> >> >
> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> >> > mix URL and local targets"?
> >> >
> >> > * "svn mkdir a ^/" -> "Can't create directory
> >> > 'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
> >> > a local path.
> >> >
> >> > * "svn mv ^/ ^/" -> "Cannot move path
> >> > 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
> >> > as if it's a local path.
> >> >
> >> > * "svn update ^/a" -> "Skipped
> >> > 'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
> >> > WC path"?
> >> >
> >>
> >> svn help update says that the argument should be a PATH. I think it is
> >> right to force the user to enter local PATH.
> >
> > Good. Thanks for finding that. I agree. That change will make some of
> > my recent patch (r1040232) redundant: it will no longer need to remove
> > URL targets from the array of targets, since it will just return an
> > error if it finds any, like you already did in some of the other
> > subcomands.
> >
>
> I further looked into the code, since update accepts multiple targets,
> the program skips URL targets assuming that there might one or more
> local paths as part of targets list. The skipping part is intentional
> and I am not sure whether we should change this behavior.
I think we should change this behaviour and make "svn update" throw an
error if any target is a URL.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@btopenworld.com> writes:
> On Tue, 2010-11-30 at 18:42 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>>
>> > I tried some potentially invalid inputs to "svn" a week or two ago and
>> > made notes on what I found. Just posting here in case anyone wants to
>> > do something about one or more of them.
>> >
>> > Noorul, I'm including you in the "To" addresses because you said you
>> > were looking for more small tasks to do, so feel free to pick one of
>> > these if you want to.
>>
>> Thank you! I will start working on these one by one.
>
> Great!
>
>
>> > Where I end with a question mark, it means I'm not sure if we want this
>> > change, it's just a suggestion.
>> >
>> > * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
>> > try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
>> > ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
>> > WC path".
>> >
>> > * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
>> > 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
>> > Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
>> >
>> > * "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
>> > in lib; should reject them at CLI level?
>> >
>> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
>> > in revision REV"?
>> >
>> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>> > mix URL and local targets"?
>> >
>> > * "svn mkdir a ^/" -> "Can't create directory
>> > 'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
>> > a local path.
>> >
>> > * "svn mv ^/ ^/" -> "Cannot move path
>> > 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
>> > as if it's a local path.
>> >
>> > * "svn update ^/a" -> "Skipped
>> > 'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
>> > WC path"?
>> >
>>
>> svn help update says that the argument should be a PATH. I think it is
>> right to force the user to enter local PATH.
>
> Good. Thanks for finding that. I agree. That change will make some of
> my recent patch (r1040232) redundant: it will no longer need to remove
> URL targets from the array of targets, since it will just return an
> error if it finds any, like you already did in some of the other
> subcomands.
>
I further looked into the code, since update accepts multiple targets,
the program skips URL targets assuming that there might one or more
local paths as part of targets list. The skipping part is intentional
and I am not sure whether we should change this behavior.
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2010-11-30 at 18:42 +0530, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > I tried some potentially invalid inputs to "svn" a week or two ago and
> > made notes on what I found. Just posting here in case anyone wants to
> > do something about one or more of them.
> >
> > Noorul, I'm including you in the "To" addresses because you said you
> > were looking for more small tasks to do, so feel free to pick one of
> > these if you want to.
>
> Thank you! I will start working on these one by one.
Great!
> > Where I end with a question mark, it means I'm not sure if we want this
> > change, it's just a suggestion.
> >
> > * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
> > try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
> > ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
> > WC path".
> >
> > * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
> > 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
> > Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
> >
> > * "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
> > in lib; should reject them at CLI level?
> >
> > * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> > in revision REV"?
> >
> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> > mix URL and local targets"?
> >
> > * "svn mkdir a ^/" -> "Can't create directory
> > 'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
> > a local path.
> >
> > * "svn mv ^/ ^/" -> "Cannot move path
> > 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
> > as if it's a local path.
> >
> > * "svn update ^/a" -> "Skipped
> > 'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
> > WC path"?
> >
>
> svn help update says that the argument should be a PATH. I think it is
> right to force the user to enter local PATH.
Good. Thanks for finding that. I agree. That change will make some of
my recent patch (r1040232) redundant: it will no longer need to remove
URL targets from the array of targets, since it will just return an
error if it finds any, like you already did in some of the other
subcomands.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> I tried some potentially invalid inputs to "svn" a week or two ago and
> made notes on what I found. Just posting here in case anyone wants to
> do something about one or more of them.
>
> Noorul, I'm including you in the "To" addresses because you said you
> were looking for more small tasks to do, so feel free to pick one of
> these if you want to.
Thank you! I will start working on these one by one.
>
> Where I end with a question mark, it means I'm not sure if we want this
> change, it's just a suggestion.
>
> * "svn checkout ^/ ^/y" -> "A asf/cxf, A asf/cxf/utils, ...". (Don't
> try this without being ready on the Ctrl-C or Ctrl-\!) It seems to
> ignore "^/y" and create ./(basename(^/)); should fail: "'^/y' is not a
> WC path".
>
> * "svn checkout ^/subversion/trunk/build ^/y" -> "Checked out revision
> 1040465. URL 'https://svn.apache.org/repos/asf/y' doesn't exist".
> Bleach - that's just crazy. Should fail: "'^/y' is not a WC path".
>
> * "svn copy a ^/b c" doesn't detect the mixed source types in cl, only
> in lib; should reject them at CLI level?
>
> * "svn info ^/b" -> "Not a valid URL"; should be "path '/b' not found
> in revision REV"?
>
> * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> mix URL and local targets"?
>
> * "svn mkdir a ^/" -> "Can't create directory
> 'https:/svn.apache.org/repos/asf'"; should not print the URL as if it's
> a local path.
>
> * "svn mv ^/ ^/" -> "Cannot move path
> 'https:/svn.apache.org/repos/asf' into itself"; should not print the URL
> as if it's a local path.
>
> * "svn update ^/a" -> "Skipped
> 'https://svn.apache.org/repos/asf/a' ..."; should fail: "'^/a' is not a
> WC path"?
>
svn help update says that the argument should be a PATH. I think it is
right to force the user to enter local PATH.
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-12-10 at 18:09 +0530, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > Noorul Islam K M wrote:
> >
> >> Attached is the patch for svn/diff-cmd.c. All tests pass.
> >
> > Hi Noorul. Thanks for mentioning that all tests pass - that's good to
> > know.
> >
> >> + svn_cl__assert_homogeneous_target_type(targets);
> >> +
> >> /* Check to see if at least one of our paths is a working copy
> >> path. */
> >> for (i = 0; i < targets->nelts; ++i)
> >
> > After you have asserted that all the targets are of the same type, there
> > is no need for a loop that checks all of them in turn, just to find out
> > whether they are paths or URLs, is there?
> >
>
> If you see the code below, it is using the variable
> working_copy_present.
Yes, it is, but you don't need a loop. All the targets are the same
type, so after examining the first one there is no point in examining
the rest.
- Julian
> Also I attached the wrong patch. I am not sure how that
> happened. Somehow my first version of the patch got attached. Here is
> the correct one.
>
> Thanks and Regards
> Noorul
>
> plain text document attachment (diff-cmd-new-mixed-func.txt)
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 1044208)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -260,7 +260,7 @@
> }
> else
> {
> - svn_boolean_t working_copy_present = FALSE, url_present = FALSE;
> + svn_boolean_t working_copy_present = FALSE;
>
> /* The 'svn diff [-r N[:M]] [TARGET[@REV]...]' case matches. */
>
> @@ -272,22 +272,20 @@
> old_target = "";
> new_target = "";
>
> + SVN_ERR(svn_cl__assert_homogeneous_target_type(targets));
> +
> /* Check to see if at least one of our paths is a working copy
> path. */
> for (i = 0; i < targets->nelts; ++i)
> {
> const char *path = APR_ARRAY_IDX(targets, i, const char *);
> if (! svn_path_is_url(path))
> - working_copy_present = TRUE;
> - else
> - url_present = TRUE;
> + {
> + working_copy_present = TRUE;
> + break;
> + }
> }
>
> - if (url_present && working_copy_present)
> - return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> - _("Cannot mix repository and working copy "
> - "targets"));
> -
> if (opt_state->start_revision.kind == svn_opt_revision_unspecified
> && working_copy_present)
> opt_state->start_revision.kind = svn_opt_revision_base;
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Noorul Islam K M wrote:
>
>> Attached is the patch for svn/diff-cmd.c. All tests pass.
>
> Hi Noorul. Thanks for mentioning that all tests pass - that's good to
> know.
>
>> + svn_cl__assert_homogeneous_target_type(targets);
>> +
>> /* Check to see if at least one of our paths is a working copy
>> path. */
>> for (i = 0; i < targets->nelts; ++i)
>
> After you have asserted that all the targets are of the same type, there
> is no need for a loop that checks all of them in turn, just to find out
> whether they are paths or URLs, is there?
>
If you see the code below, it is using the variable
working_copy_present.
Also I attached the wrong patch. I am not sure how that
happened. Somehow my first version of the patch got attached. Here is
the correct one.
Thanks and Regards
Noorul
Re: Input validation observations
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/10/2010 07:00 AM, Julian Foad wrote:
> Noorul Islam K M wrote:
>> Attached is the patch for svn/diff-cmd.c. All tests pass.
>
> Hi Noorul. Thanks for mentioning that all tests pass - that's good to
> know.
>
>> + svn_cl__assert_homogeneous_target_type(targets);
>> +
>> /* Check to see if at least one of our paths is a working copy
>> path. */
>> for (i = 0; i < targets->nelts; ++i)
>
> After you have asserted that all the targets are of the same type, there
> is no need for a loop that checks all of them in turn, just to find out
> whether they are paths or URLs, is there?
Maybe svn_cl__assert_homogeneous_target_type() should optionally set an
is_url boolean return value so all callers can benefit?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-12-10, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > After you have asserted that all the targets are of the same type, there
> > is no need for a loop that checks all of them in turn, just to find out
> > whether they are paths or URLs, is there?
>
> You are absolutely right. Thanks for this pointer. Please find updated
> patch attached. All tests pass.
Thanks, Noorul.
Committed revision 1044448.
I hope you don't mind, I took the liberty of tweaking it a bit further:
I changed it from
svn_boolean_t working_copy_present = FALSE;
...
if (! XXX)
working_copy_present = TRUE;
to
svn_boolean_t working_copy_present;
...
working_copy_present = ! XXX;
just so that the initialization happens in a single place.
Thanks.
- Julian
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 1044373)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -260,7 +260,7 @@
> }
> else
> {
> - svn_boolean_t working_copy_present = FALSE, url_present = FALSE;
> + svn_boolean_t working_copy_present = FALSE;
>
> /* The 'svn diff [-r N[:M]] [TARGET[@REV]...]' case matches. */
>
> @@ -272,21 +272,10 @@
> old_target = "";
> new_target = "";
>
> - /* Check to see if at least one of our paths is a working copy
> - path. */
> - for (i = 0; i < targets->nelts; ++i)
> - {
> - const char *path = APR_ARRAY_IDX(targets, i, const char *);
> - if (! svn_path_is_url(path))
> - working_copy_present = TRUE;
> - else
> - url_present = TRUE;
> - }
> + SVN_ERR(svn_cl__assert_homogeneous_target_type(targets));
>
> - if (url_present && working_copy_present)
> - return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> - _("Cannot mix repository and working copy "
> - "targets"));
> + if (! svn_path_is_url(APR_ARRAY_IDX(targets, 0, const char *)))
> + working_copy_present = TRUE;
>
> if (opt_state->start_revision.kind == svn_opt_revision_unspecified
> && working_copy_present)
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Noorul Islam K M wrote:
>
>> Attached is the patch for svn/diff-cmd.c. All tests pass.
>
> Hi Noorul. Thanks for mentioning that all tests pass - that's good to
> know.
>
>> + svn_cl__assert_homogeneous_target_type(targets);
>> +
>> /* Check to see if at least one of our paths is a working copy
>> path. */
>> for (i = 0; i < targets->nelts; ++i)
>
> After you have asserted that all the targets are of the same type, there
> is no need for a loop that checks all of them in turn, just to find out
> whether they are paths or URLs, is there?
>
You are absolutely right. Thanks for this pointer. Please find updated
patch attached. All tests pass.
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
Noorul Islam K M wrote:
> Attached is the patch for svn/diff-cmd.c. All tests pass.
Hi Noorul. Thanks for mentioning that all tests pass - that's good to
know.
> + svn_cl__assert_homogeneous_target_type(targets);
> +
> /* Check to see if at least one of our paths is a working copy
> path. */
> for (i = 0; i < targets->nelts; ++i)
After you have asserted that all the targets are of the same type, there
is no need for a loop that checks all of them in turn, just to find out
whether they are paths or URLs, is there?
- Julian
> {
> const char *path = APR_ARRAY_IDX(targets, i, const char *);
> if (! svn_path_is_url(path))
> - working_copy_present = TRUE;
> - else
> - url_present = TRUE;
> + {
> + working_copy_present = TRUE;
> + break;
> + }
> }
[...]
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Noorul Islam K M <no...@collab.net> writes:
> Julian Foad <ju...@wandisco.com> writes:
>
>> On Sat, 2010-12-04 at 13:01 +0530, Noorul Islam K M wrote:
>>
>>> Julian Foad <ju...@wandisco.com> writes:
>>>
>>> > Noorul Islam K M wrote:
>>> >
>>> >> Julian Foad <ju...@wandisco.com> writes:
>>> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>>> >> > mix URL and local targets"?
>>> > [...]
>>> >> Make 'svn mkdir' verify that both working copy paths and URLs are
>>> >> not passed.
>>> >>
>>> >> * subversion/svn/mkdir-cmd.c,
>>> >> subversion/libsvn_client/add.c
>>> >> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
>>> >> copy paths and URLs are passed.
>>> >>
>>> >> * subversion/tests/cmdline/input_validation_tests.py
>>> >> (invalid_mkdir_targets, test_list): New test
>>> > [...]
>>> >> Index: subversion/svn/mkdir-cmd.c
>>> >> ===================================================================
>>> >> --- subversion/svn/mkdir-cmd.c (revision 1041293)
>>> >> +++ subversion/svn/mkdir-cmd.c (working copy)
>>> >> @@ -48,6 +48,8 @@
>>> >> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
>>> >> + int i;
>>> >> @@ -56,6 +58,22 @@
>>> >> + /* Check to see if at least one of our paths is a working copy
>>> >> + path or a repository url. */
>>> >> + for (i = 0; i < targets->nelts; ++i)
>>> >> + {
>>> >> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>>> >> + if (! svn_path_is_url(target))
>>> >> + wc_present = TRUE;
>>> >> + else
>>> >> + url_present = TRUE;
>>> >> + }
>>> >> +
>>> >> + if (url_present && wc_present)
>>> >> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>>> >> + _("Cannot mix repository and working copy "
>>> >> + "targets"));
>>> >
>>> > This is fine.
>>> >
>>> > The same code already exists in three other files and equivalent but
>>> > different code also exists in at least delete-cmd.c and probably other
>>> > files. I think it is time to factor it out. We can do that in a
>>> > subsequent patch.
>>>
>>> Do you mean we need to come up with new function that will do this check
>>> and return the error message?
>>
>> Yes please.
>
> Attached is the patch which has the new functions to replace the
> existing blocks. All tests pass when tested with 'make check'. No need
> for test cases as they are already available. Also I have not modified
> libsvn_client/copy.c and svn/diff-cmd.c because they are not straight
> forward. I will go through them again and will come up with follow-up
> patch.
>
Attached is the patch for svn/diff-cmd.c. All tests pass.
Log
[[[
Follow-up to r1044028. Make use of new function.
* subversion/svn/diff-cmd.c
(svn_cl__diff): Make use of new function
svn_cl__assert_homogeneous_target_type(). Tweak existing logic which
checks to see whether working copy path is present in targets.
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-12-08, Noorul Islam K M wrote:
> Attached is the patch which has the new functions to replace the
> existing blocks. All tests pass when tested with 'make check'. No need
> for test cases as they are already available.
Noorul, thank you. This is great.
> Also I have not modified
> libsvn_client/copy.c and svn/diff-cmd.c because they are not straight
> forward. I will go through them again and will come up with follow-up
> patch.
OK, that's fine.
The log message is good. I'll just rearrange the introductory sentence
and start with the words "Factor out ...", as I think that provides a
quick introduction to the change.
> [[[
> Two new functions one each for command line and client API which checks
> whether the target types are homogeneous.
>
> * subversion/svn/cl.h,
> subversion/svn/util.c
> (svn_cl__assert_homogeneous_target_type): New function
>
> * subversion/libsvn_client/client.h
> subversion/libsvn_client/util.c
> (svn_client__assert_homogeneous_target_type): New function
>
> * subversion/svn/mkdir-cmd.c,
> subversion/svn/delete-cmd.c,
> subversion/svn/lock-cmd.c,
> subversion/svn/unlock-cmd.c
> (svn_cl__mkdir, svn_cl__delete, svn_cl__lock, svn_cl__unlock):
> Replace existing logic with call to new function
> svn_cl__assert_homogeneous_target_type
>
> * subversion/libsvn_client/delete.c,
> subversion/libsvn_client/locking_commands.c,
> subversion/libsvn_client/add.c
> (svn_client_delete4, organize_lock_targets, svn_client_mkdir4):
> Replace existing logic with call to new function
> svn_client__assert_homogeneous_target_type
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
The patch is good. The only things I want to change are very minor
details. There is no need for you to resubmit the patch; I will just
change them before I commit it.
> Index: subversion/svn/cl.h
> ===================================================================
> +/* This function checks to see if targets contain mixture of URLs
> + * and paths, returning an error if it finds a mixture of both. */
> +svn_error_t *
> +svn_cl__assert_homogeneous_target_type(apr_array_header_t *targets);
> Index: subversion/libsvn_client/client.h
> ===================================================================
> +/* This function checks to see if targets contain mixture of URLs
> + * and paths, returning an error if it finds mixture of both. */
> +svn_error_t *
> +svn_client__assert_homogeneous_target_type(const apr_array_header_t *targets);
I re-wrote the doc string as:
/* Return an error if TARGETS contains a mixture of URLs and paths;
* otherwise return SVN_NO_ERROR. */
and I added "const" to the parameter of the svn_cl__ function, because
that's logical and consistent with the other function.
> Index: subversion/svn/util.c
> ===================================================================
> +svn_error_t *
> +svn_cl__assert_homogeneous_target_type(apr_array_header_t *targets)
> +{
> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
> + int i;
> +
> + for (i = 0; i < targets->nelts; ++i)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> + if (! svn_path_is_url(target))
> + wc_present = TRUE;
> + else
> + url_present = TRUE;
> + }
> +
> + if (url_present && wc_present)
> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Cannot mix repository and working copy "
> + "targets"));
I adjusted the indentation of these two lines so "_(" is below "SVN_".
> +
> + return SVN_NO_ERROR;
> +}
Committed revision 1044028.
Thanks again.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Sat, 2010-12-04 at 13:01 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>>
>> > Noorul Islam K M wrote:
>> >
>> >> Julian Foad <ju...@wandisco.com> writes:
>> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>> >> > mix URL and local targets"?
>> > [...]
>> >> Make 'svn mkdir' verify that both working copy paths and URLs are
>> >> not passed.
>> >>
>> >> * subversion/svn/mkdir-cmd.c,
>> >> subversion/libsvn_client/add.c
>> >> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
>> >> copy paths and URLs are passed.
>> >>
>> >> * subversion/tests/cmdline/input_validation_tests.py
>> >> (invalid_mkdir_targets, test_list): New test
>> > [...]
>> >> Index: subversion/svn/mkdir-cmd.c
>> >> ===================================================================
>> >> --- subversion/svn/mkdir-cmd.c (revision 1041293)
>> >> +++ subversion/svn/mkdir-cmd.c (working copy)
>> >> @@ -48,6 +48,8 @@
>> >> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
>> >> + int i;
>> >> @@ -56,6 +58,22 @@
>> >> + /* Check to see if at least one of our paths is a working copy
>> >> + path or a repository url. */
>> >> + for (i = 0; i < targets->nelts; ++i)
>> >> + {
>> >> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>> >> + if (! svn_path_is_url(target))
>> >> + wc_present = TRUE;
>> >> + else
>> >> + url_present = TRUE;
>> >> + }
>> >> +
>> >> + if (url_present && wc_present)
>> >> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> >> + _("Cannot mix repository and working copy "
>> >> + "targets"));
>> >
>> > This is fine.
>> >
>> > The same code already exists in three other files and equivalent but
>> > different code also exists in at least delete-cmd.c and probably other
>> > files. I think it is time to factor it out. We can do that in a
>> > subsequent patch.
>>
>> Do you mean we need to come up with new function that will do this check
>> and return the error message?
>
> Yes please.
Attached is the patch which has the new functions to replace the
existing blocks. All tests pass when tested with 'make check'. No need
for test cases as they are already available. Also I have not modified
libsvn_client/copy.c and svn/diff-cmd.c because they are not straight
forward. I will go through them again and will come up with follow-up
patch.
Log
[[[
Two new functions one each for command line and client API which checks
whether the target types are homogeneous.
* subversion/svn/cl.h,
subversion/svn/util.c
(svn_cl__assert_homogeneous_target_type): New function
* subversion/libsvn_client/client.h
subversion/libsvn_client/util.c
(svn_client__assert_homogeneous_target_type): New function
* subversion/svn/mkdir-cmd.c,
subversion/svn/delete-cmd.c,
subversion/svn/lock-cmd.c,
subversion/svn/unlock-cmd.c
(svn_cl__mkdir, svn_cl__delete, svn_cl__lock, svn_cl__unlock):
Replace existing logic with call to new function
svn_cl__assert_homogeneous_target_type
* subversion/libsvn_client/delete.c,
subversion/libsvn_client/locking_commands.c,
subversion/libsvn_client/add.c
(svn_client_delete4, organize_lock_targets, svn_client_mkdir4):
Replace existing logic with call to new function
svn_client__assert_homogeneous_target_type
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> On Sat, 2010-12-04 at 13:01 +0530, Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>>
>> > Noorul Islam K M wrote:
>> >
>> >> Julian Foad <ju...@wandisco.com> writes:
>> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>> >> > mix URL and local targets"?
>> > [...]
>> >> Make 'svn mkdir' verify that both working copy paths and URLs are
>> >> not passed.
>> >>
>> >> * subversion/svn/mkdir-cmd.c,
>> >> subversion/libsvn_client/add.c
>> >> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
>> >> copy paths and URLs are passed.
>> >>
>> >> * subversion/tests/cmdline/input_validation_tests.py
>> >> (invalid_mkdir_targets, test_list): New test
>> > [...]
>> >> Index: subversion/svn/mkdir-cmd.c
>> >> ===================================================================
>> >> --- subversion/svn/mkdir-cmd.c (revision 1041293)
>> >> +++ subversion/svn/mkdir-cmd.c (working copy)
>> >> @@ -48,6 +48,8 @@
>> >> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
>> >> + int i;
>> >> @@ -56,6 +58,22 @@
>> >> + /* Check to see if at least one of our paths is a working copy
>> >> + path or a repository url. */
>> >> + for (i = 0; i < targets->nelts; ++i)
>> >> + {
>> >> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>> >> + if (! svn_path_is_url(target))
>> >> + wc_present = TRUE;
>> >> + else
>> >> + url_present = TRUE;
>> >> + }
>> >> +
>> >> + if (url_present && wc_present)
>> >> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> >> + _("Cannot mix repository and working copy "
>> >> + "targets"));
>> >
>> > This is fine.
>> >
>> > The same code already exists in three other files and equivalent but
>> > different code also exists in at least delete-cmd.c and probably other
>> > files. I think it is time to factor it out. We can do that in a
>> > subsequent patch.
>>
>> Do you mean we need to come up with new function that will do this check
>> and return the error message?
>
> Yes please.
Attached is the patch which has the new functions to replace the
existing blocks. All tests pass when tested with 'make check'. No need
for test cases as they are already available. Also I have not modified
libsvn_client/copy.c and svn/diff-cmd.c because they are not straight
forward. I will go through them again and will come up with follow-up
patch.
Log
[[[
Two new functions one each for command line and client API which checks
whether the target types are homogeneous.
* subversion/svn/cl.h,
subversion/svn/util.c
(svn_cl__assert_homogeneous_target_type): New function
* subversion/libsvn_client/client.h
subversion/libsvn_client/util.c
(svn_client__assert_homogeneous_target_type): New function
* subversion/svn/mkdir-cmd.c,
subversion/svn/delete-cmd.c,
subversion/svn/lock-cmd.c,
subversion/svn/unlock-cmd.c
(svn_cl__mkdir, svn_cl__delete, svn_cl__lock, svn_cl__unlock):
Replace existing logic with call to new function
svn_cl__assert_homogeneous_target_type
* subversion/libsvn_client/delete.c,
subversion/libsvn_client/locking_commands.c,
subversion/libsvn_client/add.c
(svn_client_delete4, organize_lock_targets, svn_client_mkdir4):
Replace existing logic with call to new function
svn_client__assert_homogeneous_target_type
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Sat, 2010-12-04 at 13:01 +0530, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
>
> > Noorul Islam K M wrote:
> >
> >> Julian Foad <ju...@wandisco.com> writes:
> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> >> > mix URL and local targets"?
> > [...]
> >> Make 'svn mkdir' verify that both working copy paths and URLs are
> >> not passed.
> >>
> >> * subversion/svn/mkdir-cmd.c,
> >> subversion/libsvn_client/add.c
> >> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
> >> copy paths and URLs are passed.
> >>
> >> * subversion/tests/cmdline/input_validation_tests.py
> >> (invalid_mkdir_targets, test_list): New test
> > [...]
> >> Index: subversion/svn/mkdir-cmd.c
> >> ===================================================================
> >> --- subversion/svn/mkdir-cmd.c (revision 1041293)
> >> +++ subversion/svn/mkdir-cmd.c (working copy)
> >> @@ -48,6 +48,8 @@
> >> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
> >> + int i;
> >> @@ -56,6 +58,22 @@
> >> + /* Check to see if at least one of our paths is a working copy
> >> + path or a repository url. */
> >> + for (i = 0; i < targets->nelts; ++i)
> >> + {
> >> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> >> + if (! svn_path_is_url(target))
> >> + wc_present = TRUE;
> >> + else
> >> + url_present = TRUE;
> >> + }
> >> +
> >> + if (url_present && wc_present)
> >> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> >> + _("Cannot mix repository and working copy "
> >> + "targets"));
> >
> > This is fine.
> >
> > The same code already exists in three other files and equivalent but
> > different code also exists in at least delete-cmd.c and probably other
> > files. I think it is time to factor it out. We can do that in a
> > subsequent patch.
>
> Do you mean we need to come up with new function that will do this check
> and return the error message?
Yes please.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>> > mix URL and local targets"?
> [...]
>> Make 'svn mkdir' verify that both working copy paths and URLs are
>> not passed.
>>
>> * subversion/svn/mkdir-cmd.c,
>> subversion/libsvn_client/add.c
>> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
>> copy paths and URLs are passed.
>>
>> * subversion/tests/cmdline/input_validation_tests.py
>> (invalid_mkdir_targets, test_list): New test
> [...]
>> Index: subversion/svn/mkdir-cmd.c
>> ===================================================================
>> --- subversion/svn/mkdir-cmd.c (revision 1041293)
>> +++ subversion/svn/mkdir-cmd.c (working copy)
>> @@ -48,6 +48,8 @@
>> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
>> + int i;
>> @@ -56,6 +58,22 @@
>> + /* Check to see if at least one of our paths is a working copy
>> + path or a repository url. */
>> + for (i = 0; i < targets->nelts; ++i)
>> + {
>> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>> + if (! svn_path_is_url(target))
>> + wc_present = TRUE;
>> + else
>> + url_present = TRUE;
>> + }
>> +
>> + if (url_present && wc_present)
>> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> + _("Cannot mix repository and working copy "
>> + "targets"));
>
> This is fine.
>
> The same code already exists in three other files and equivalent but
> different code also exists in at least delete-cmd.c and probably other
> files. I think it is time to factor it out. We can do that in a
> subsequent patch.
Do you mean we need to come up with new function that will do this check
and return the error message?
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-12-03 at 16:53 +0000, Julian Foad wrote:
> On Fri, 2010-12-03, Julian Foad wrote:
> > On Fri, 2010-12-03 at 19:15 +0530, Noorul Islam K M wrote:
> > > Julian Foad <ju...@wandisco.com> writes:
> > > > Noorul Islam K M wrote:
> > > >> Julian Foad <ju...@wandisco.com> writes:
> > > >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> > > >> > mix URL and local targets"?
> > > > [...]
> > > > Does this validation make the check at the beginning of mkdir_urls()
> > > > redundant? If so, we should get rid of it.
> > >
> > > Attached is the modified patch.
> >
> > Committed in r1041900.
> >
> > Thanks!
>
> EXPECTED STDERR (regexp):
> .*Illegal repository URL 'subdir'
> ACTUAL STDERR:
> svn: Try 'svn help' for more info
> svn: Cannot mix repository and working copy targets
> FAIL: basic_tests.py 49: mkdir mix url and local path should error
>
> I'll fix it.
r1041920. And I updated issue #2586
<http://subversion.tigris.org/issues/show_bug.cgi?id=2586>.
- Julian
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-12-03, Julian Foad wrote:
> On Fri, 2010-12-03 at 19:15 +0530, Noorul Islam K M wrote:
> > Julian Foad <ju...@wandisco.com> writes:
> > > Noorul Islam K M wrote:
> > >> Julian Foad <ju...@wandisco.com> writes:
> > >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> > >> > mix URL and local targets"?
> > > [...]
> > > Does this validation make the check at the beginning of mkdir_urls()
> > > redundant? If so, we should get rid of it.
> >
> > Attached is the modified patch.
>
> Committed in r1041900.
>
> Thanks!
EXPECTED STDERR (regexp):
.*Illegal repository URL 'subdir'
ACTUAL STDERR:
svn: Try 'svn help' for more info
svn: Cannot mix repository and working copy targets
FAIL: basic_tests.py 49: mkdir mix url and local path should error
I'll fix it.
- Julian
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-12-03 at 19:15 +0530, Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > Noorul Islam K M wrote:
> >> Julian Foad <ju...@wandisco.com> writes:
> >> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> >> > mix URL and local targets"?
> > [...]
> > Does this validation make the check at the beginning of mkdir_urls()
> > redundant? If so, we should get rid of it.
>
> Attached is the modified patch.
Committed in r1041900.
Thanks!
- Julian
> Log
>
> [[[
> Make 'svn mkdir' verify that both working copy paths and URLs are
> not passed.
>
> * subversion/svn/mkdir-cmd.c,
> subversion/libsvn_client/add.c
> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
> copy paths and URLs are passed.
> (mkdir_urls): Remove redundant code.
>
> * subversion/tests/cmdline/input_validation_tests.py
> (invalid_mkdir_targets, test_list): New test
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> Noorul Islam K M wrote:
>
>> Julian Foad <ju...@wandisco.com> writes:
>> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
>> > mix URL and local targets"?
> [...]
>> Make 'svn mkdir' verify that both working copy paths and URLs are
>> not passed.
>>
>> * subversion/svn/mkdir-cmd.c,
>> subversion/libsvn_client/add.c
>> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
>> copy paths and URLs are passed.
>>
>> * subversion/tests/cmdline/input_validation_tests.py
>> (invalid_mkdir_targets, test_list): New test
> [...]
>> Index: subversion/svn/mkdir-cmd.c
>> ===================================================================
>> --- subversion/svn/mkdir-cmd.c (revision 1041293)
>> +++ subversion/svn/mkdir-cmd.c (working copy)
>> @@ -48,6 +48,8 @@
>> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
>> + int i;
>> @@ -56,6 +58,22 @@
>> + /* Check to see if at least one of our paths is a working copy
>> + path or a repository url. */
>> + for (i = 0; i < targets->nelts; ++i)
>> + {
>> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
>> + if (! svn_path_is_url(target))
>> + wc_present = TRUE;
>> + else
>> + url_present = TRUE;
>> + }
>> +
>> + if (url_present && wc_present)
>> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
>> + _("Cannot mix repository and working copy "
>> + "targets"));
>
> This is fine.
>
> The same code already exists in three other files and equivalent but
> different code also exists in at least delete-cmd.c and probably other
> files. I think it is time to factor it out. We can do that in a
> subsequent patch.
>
>> Index: subversion/libsvn_client/add.c
>> ===================================================================
>> --- subversion/libsvn_client/add.c (revision 1041293)
>> +++ subversion/libsvn_client/add.c (working copy)
>> @@ -875,9 +875,28 @@
>> svn_client_ctx_t *ctx,
>> apr_pool_t *pool)
>> {
>> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
>> + int i;
>> +
>> if (! paths->nelts)
>> return SVN_NO_ERROR;
>>
>> + /* Check to see if at least one of our paths is a working copy
>> + path or a repository url. */
>> + for (i = 0; i < paths->nelts; ++i)
>> + {
>> + const char *path = APR_ARRAY_IDX(paths, i, const char *);
>> + if (! svn_path_is_url(path))
>> + wc_present = TRUE;
>> + else
>> + url_present = TRUE;
>> + }
>> +
>> + if (url_present && wc_present)
>> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
>> + _("Cannot mix repository and working copy "
>> + "targets"));
>
> Does this validation make the check at the beginning of mkdir_urls()
> redundant? If so, we should get rid of it.
>
Attached is the modified patch.
Log
[[[
Make 'svn mkdir' verify that both working copy paths and URLs are
not passed.
* subversion/svn/mkdir-cmd.c,
subversion/libsvn_client/add.c
(svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
copy paths and URLs are passed.
(mkdir_urls): Remove redundant code.
* subversion/tests/cmdline/input_validation_tests.py
(invalid_mkdir_targets, test_list): New test
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]
Thanks and Regards
Noorul
Re: Input validation observations
Posted by Julian Foad <ju...@wandisco.com>.
Noorul Islam K M wrote:
> Julian Foad <ju...@wandisco.com> writes:
> > * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> > mix URL and local targets"?
[...]
> Make 'svn mkdir' verify that both working copy paths and URLs are
> not passed.
>
> * subversion/svn/mkdir-cmd.c,
> subversion/libsvn_client/add.c
> (svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
> copy paths and URLs are passed.
>
> * subversion/tests/cmdline/input_validation_tests.py
> (invalid_mkdir_targets, test_list): New test
[...]
> Index: subversion/svn/mkdir-cmd.c
> ===================================================================
> --- subversion/svn/mkdir-cmd.c (revision 1041293)
> +++ subversion/svn/mkdir-cmd.c (working copy)
> @@ -48,6 +48,8 @@
> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
> + int i;
> @@ -56,6 +58,22 @@
> + /* Check to see if at least one of our paths is a working copy
> + path or a repository url. */
> + for (i = 0; i < targets->nelts; ++i)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> + if (! svn_path_is_url(target))
> + wc_present = TRUE;
> + else
> + url_present = TRUE;
> + }
> +
> + if (url_present && wc_present)
> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Cannot mix repository and working copy "
> + "targets"));
This is fine.
The same code already exists in three other files and equivalent but
different code also exists in at least delete-cmd.c and probably other
files. I think it is time to factor it out. We can do that in a
subsequent patch.
> Index: subversion/libsvn_client/add.c
> ===================================================================
> --- subversion/libsvn_client/add.c (revision 1041293)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -875,9 +875,28 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
> + int i;
> +
> if (! paths->nelts)
> return SVN_NO_ERROR;
>
> + /* Check to see if at least one of our paths is a working copy
> + path or a repository url. */
> + for (i = 0; i < paths->nelts; ++i)
> + {
> + const char *path = APR_ARRAY_IDX(paths, i, const char *);
> + if (! svn_path_is_url(path))
> + wc_present = TRUE;
> + else
> + url_present = TRUE;
> + }
> +
> + if (url_present && wc_present)
> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("Cannot mix repository and working copy "
> + "targets"));
Does this validation make the check at the beginning of mkdir_urls()
redundant? If so, we should get rid of it.
- Julian
Re: Input validation observations
Posted by Noorul Islam K M <no...@collab.net>.
Julian Foad <ju...@wandisco.com> writes:
> I tried some potentially invalid inputs to "svn" a week or two ago and
> made notes on what I found. Just posting here in case anyone wants to
> do something about one or more of them.
>
> Noorul, I'm including you in the "To" addresses because you said you
> were looking for more small tasks to do, so feel free to pick one of
> these if you want to.
>
> Where I end with a question mark, it means I'm not sure if we want this
> change, it's just a suggestion.
>
> * "svn mkdir ^/ a" -> "Illegal repository URL 'a'"; should say "can't
> mix URL and local targets"?
>
Patch attached
Log
[[[
Make 'svn mkdir' verify that both working copy paths and URLs are
not passed.
* subversion/svn/mkdir-cmd.c,
subversion/libsvn_client/add.c
(svn_cl__mkdir, svn_client_mkdir4): Raise an error if both working
copy paths and URLs are passed.
* subversion/tests/cmdline/input_validation_tests.py
(invalid_mkdir_targets, test_list): New test
Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
]]]