subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Noorul Islam K M <noo...@collab.net>
Subject Re: Input validation observations
Date Wed, 08 Dec 2010 09:26:29 GMT
Julian Foad <julian.foad@wandisco.com> writes:

> On Sat, 2010-12-04, Noorul Islam K M wrote:
>
>> Julian Foad <julian.foad@wandisco.com> writes:
>> > Noorul Islam K M wrote:
>> >> Julian Foad <julian.foad@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

noorul@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@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@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@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


Mime
View raw message