On Wed, 2010-12-15, 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:
> >> > 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.
> >
> >
>
> Just pinging to remind you since this is a huge thread.
Thanks for the ping.
If anybody else wants to take this, please do. Otherwise, I have it
marked for attention and will get to it some time.
Same for the 'move_to_self_error.diff' patch in this thread.
- Julian
Received on 2010-12-15 18:06:09 CET