On Wed, 2010-12-15, Noorul Islam K M wrote: > Julian Foad <julian.f...@wandisco.com> writes: > > > Noorul Islam K M wrote: > > > >> Julian Foad <julian.f...@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 > >> > >> noo...@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 > >> > >> noo...@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 > >> > >> noo...@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 > >> > >> noo...@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
