Noorul Islam K M wrote:
> Julian Foad <julian.foad_at_wandisco.com> writes:
> > 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_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.
>
> 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.
Hi Noorul.
Thank you very much for this extra information. It makes my job as a
reviewer very much easier.
I haven't reviewed it yet but I will get back to it soon.
> > 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.
Thank you very much. I really appreciate it.
- Julian
Received on 2010-12-08 16:38:06 CET