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 and Regards Noorul
