Forwarding one of the threads to dev@ with [PATCH] tag so that this gets
attention of more people. Julian said on IRC,
<julianf> noorul: Now we're into the realm of changing behaviour. If the error
code returned from libsvn_client is different in 1.7 from what it
was in 1.6, maybe that indicates a backward-compatibility problem
with our APIs. I think this needs more review. It is not so simple
as just changing an error message.
<julianf> noorul: Please could you post this again as a new thread, with
subject line starting with "[PATCH]"? I'd like other people to
review this. Thanks.
Pasting here the log once more so that there is some clarity about the
patch attached.
[[[
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
attached mail follows:
Noorul Islam K M <noorul_at_collab.net> writes:
> Julian Foad <julian.foad_at_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_at_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_at_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_at_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_at_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.
Index: subversion/tests/cmdline/basic_tests.py
===================================================================
--- subversion/tests/cmdline/basic_tests.py (revision 1051915)
+++ subversion/tests/cmdline/basic_tests.py (working copy)
@@ -2260,7 +2260,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 1051915)
+++ subversion/svn/info-cmd.c (working copy)
@@ -566,25 +566,15 @@
{
/* If one of the targets is a non-existent URL or wc-entry,
don't bail out. Just warn and move on to the next target. */
- if (err->apr_err == SVN_ERR_UNVERSIONED_RESOURCE
- || err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
+ if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND ||
+ err->apr_err == SVN_ERR_RA_ILLEGAL_URL)
{
- SVN_ERR(svn_cmdline_fprintf
- (stderr, subpool,
- _("%s: (Not a versioned resource)\n\n"),
- svn_path_is_url(truepath)
- ? truepath
- : svn_dirent_local_style(truepath, pool)));
+ char errbuf[256];
+
+ SVN_ERR(svn_cmdline_fprintf(
+ stderr, subpool, "svn: %s\n\n",
+ svn_err_best_message(err, errbuf, sizeof(errbuf))));
}
- 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)));
- }
else
{
return svn_error_return(err);
Received on 2010-12-23 11:37:32 CET