On Sat, 2010-12-04, Noorul Islam K M wrote:
> Julian Foad <julian.foad_at_wandisco.com> writes:
> > Noorul Islam K M wrote:
> >> Julian Foad <julian.foad_at_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?
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.
Thank you in advance.
- Julian
> - 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))));
Received on 2010-12-06 18:21:43 CET