Julian Foad <julian.f...@wandisco.com> writes:
> On Sat, 2010-12-04, Noorul Islam K M wrote:
>
>> Julian Foad <julian.f...@wandisco.com> writes:
>> > Noorul Islam K M wrote:
>> >> Julian Foad <julian.f...@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
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.
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
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];
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"),
+ svn_err_best_message(err, errbuf, sizeof(errbuf))));
}
else
{
