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. > > 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
