Julian Foad <julian.f...@wandisco.com> writes:
> On Thu, 2011-02-10, Noorul Islam K M wrote:
>
>> It looks like for file:// and http:// protocols cat prints different
>> warning messages for non-existing target.
>>
>> For http://
>>
>> svn: warning: W160013:
>> '/svn-test-work/repositories/cat_tests-9/!svn/bc/1/non-existing' path
>> not found
>>
>> For file://
>>
>> svn: warning: W160013: File not found: revision 1, path '/non-existing'
>
> OK. Ideally, I think, we should aim to enhance Subversion's error
> reporting so that it reports the same high-level error message for any
> protocol, as a wrapper around the protocol-specific lower-level
> messages. But first we can just work with what we have.
>
> I see that your patch is modifying a test's expectations to match both
> error messages. Is the test currently failing, or wrongly passing, due
> to this difference? In what way does the patch change the test results?
>
This is actually committed now in r1069330. The test was failing for
http:// and svn://. The patch looks for the error code alone.
> You didn't mention the svn:// protocol. Do you need to adjust for that
> too?
>
No need to adjust for that.
>
>> Log
>>
>> [[[
>> For file:// and http:// protocols cat prints different warning messages
>> for non-existing target.
>
> That statement supplies some relevant information, but a better log
> message would begin with a high level summary of the *change* being
> made. For this patch, the simplest top-level summary could be:
>
> Fix an error in a test.
>
> The ideal level of detail for the first sentence is just enough for any
> other developer to decide whether they need to read further and learn
> the details of this change. Something like this in style:
>
> Correct the expectations of an XFail test that has been failing for
> the wrong reason (when using http:// protocol) since rXXXXXXX.
>
> ... although I don't think that is a correct description of this
> particular patch.
>
I will keep all these points in mind. Thank you for taking your time
reviewing this.
Thanks and Regards
Noorul
>
>
>> * subversion/tests/cmdline/cat_tests.py
>> (cat_non_existing_remote_file): Modify regular expression to handle
>> both http:// and file:// targets.
>> ]]]
>
>> Index: subversion/tests/cmdline/cat_tests.py
>> ===================================================================
>> --- subversion/tests/cmdline/cat_tests.py (revision 1069209)
>> +++ subversion/tests/cmdline/cat_tests.py (working copy)
>> @@ -238,8 +238,7 @@
>> sbox.build(create_wc = False)
>> non_existing_path = sbox.repo_url + '/non-existing'
>>
>> - expected_err = "svn: warning: W160013: File not found.*" + \
>> - non_existing_path.split('/')[1]
>> + expected_err = "svn: warning: W160013: .*not found.*"
>>
>> # cat operation on non-existing remote path should return 1
>> svntest.actions.run_and_verify_svn2(None, None, expected_err, 1,
