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.
> 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
Received on 2010-12-20 12:46:35 CET