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 and Regards
Noorul
Received on 2010-12-15 08:21:21 CET