Julian Foad <julian.f...@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
>>
>> noo...@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
>>
>> noo...@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
>>
>> noo...@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
>>
>> noo...@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
