>From IRC:
<Bert> julianf: We changed error codes in libsvn_wc all over the place.
I don't think we see strict error codes as part of the documented
behavior, unless it is in the function documentation
<julianf> Bert: OK, but I wonder if we really should accept changes to a
libsvn_client response (which this is, I assume). Even if not
documented, I wanted somebody else's opinion. Would you mind responding
to the thread?
(I'm thinking: if this is something that "svn" checks for explicitly,
then it's likely that other clients also check for it explicitly.)
- Julian
On Thu, 2010-12-23 at 17:32 +0530, Noorul Islam K M wrote:
> Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:
>
> > Noorul Islam K M wrote on Thu, Dec 23, 2010 at 15:53:58 +0530:
> >
> >>
> >> 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.
> >>
> >
> > So, to ask the obvious:
> >
> > What error codes (err->apr_err) were returned in 1.6.x, and what
> > error codes are returned now?
> >
> > Noorul: you can use tools/dev/which-error.py to convert numeric error
> > codes into symbolic ones.
>
> In 1.6 for a non-existent wc-entry the API returns
>
> 00150000 SVN_ERR_ENTRY_NOT_FOUND
>
> With 1.7
>
> 00155010 SVN_ERR_WC_PATH_NOT_FOUND
>
> >
> > (more below)
> >
> >> 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
> >>
> >
> >> 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))));
> >
> > Why do you have to place the "svn: " prefix here explicitly? Normally
> > svn_handle_error2() would do that for you. This is a red flag ("is
> > a wheel being reinvented here?") for me.
> >
>
> We are actually consuming the error here to print it and proceed with
> the other targets.
>
> Thanks and Regards
> Noorul
>
> >> }
> >> - 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 13:27:49 CET