You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ka...@apache.org on 2011/04/04 14:55:38 UTC
svn commit: r1088602 - in /subversion/trunk/subversion: mod_dav_svn/mirror.c
tests/cmdline/dav-mirror-autocheck.sh
Author: kameshj
Date: Mon Apr 4 12:55:38 2011
New Revision: 1088602
URL: http://svn.apache.org/viewvc?rev=1088602&view=rev
Log:
Fix the regression issue triggered by r917523.
The revision r917523 do some url encodings to the paths and uris which are
not url-encoded. But there is one additional url-encoding of an uri which is
already encoded. With this extra encoding, committing a path to slave which has
space in it fails.
* subversion/tests/cmdline/dav-mirror-autocheck.sh
Add a testcase for a regression issue triggered by r917523.
* subversion/mod_dav_svn/mirror.c
(dav_svn__location_header_filter): Remove redundant url-encoding of new_uri.
Patch by: Vijayaguru G <vijay{_AT_}collab.net>
Modified:
subversion/trunk/subversion/mod_dav_svn/mirror.c
subversion/trunk/subversion/tests/cmdline/dav-mirror-autocheck.sh
Modified: subversion/trunk/subversion/mod_dav_svn/mirror.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mirror.c?rev=1088602&r1=1088601&r2=1088602&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mirror.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mirror.c Mon Apr 4 12:55:38 2011
@@ -241,7 +241,6 @@ apr_status_t dav_svn__location_header_fi
dav_svn__get_root_dir(r), "/",
start_foo, (char *)NULL),
r);
- new_uri = svn_path_uri_encode(new_uri, r->pool);
apr_table_set(r->headers_out, "Location", new_uri);
}
return ap_pass_brigade(f->next, bb);
Modified: subversion/trunk/subversion/tests/cmdline/dav-mirror-autocheck.sh
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/dav-mirror-autocheck.sh?rev=1088602&r1=1088601&r2=1088602&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/dav-mirror-autocheck.sh (original)
+++ subversion/trunk/subversion/tests/cmdline/dav-mirror-autocheck.sh Mon Apr 4 12:55:38 2011
@@ -416,10 +416,6 @@ $svnmucc rm "$BASE_URL/branch" cp 2 "$BA
say "svn log on $BASE_URL : "
$SVN --username jrandom --password rayjandom log -vq "$BASE_URL"
-# shut it down
-echo -n "${SCRIPT}: stopping httpd: "
-$HTTPD -f $HTTPD_CONFIG -k stop
-echo "."
# verify result: should be at rev 4 in both repos
# FIXME: do more rigorous verification here
@@ -438,5 +434,29 @@ fi
say "PASS: master, slave are both at r4, as expected"
-exit 0
+# The following test case is for the regression issue triggered by r917523.
+# The revision r917523 do some url encodings to the paths and uris which are
+# not url-encoded. But there is one additional url-encoding of an uri which is
+# already encoded. With this extra encoding, committing a path to slave which
+# has space in it fails. Please see this thread
+# http://svn.haxx.se/dev/archive-2011-03/0641.shtml for more info.
+
+say "Test case for regression issue triggered by r917523"
+
+$svnmucc cp 2 "$BASE_URL/trunk" "$BASE_URL/branch new"
+$svnmucc put /dev/null "$BASE_URL/branch new/file" \
+--config-option servers:global:http-library=neon
+RETVAL=$?
+
+if [ $RETVAL -eq 0 ] ; then
+ say "PASS: committing a path which has space in it passes"
+else
+ say "FAIL: committing a path which has space in it fails as there are extra
+ url-encodings happening in server side"
+fi
+# shut it down
+echo -n "${SCRIPT}: stopping httpd: "
+$HTTPD -f $HTTPD_CONFIG -k stop
+echo "."
+exit 0
Re: svn commit: r1088602 - in /subversion/trunk/subversion:
mod_dav_svn/mirror.c tests/cmdline/dav-mirror-autocheck.sh
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Tue, 05 Apr 2011 12:36 +0530, "vijay" <vi...@collab.net> wrote:
> The testcase is only for the regression issue triggered by r917523. If
> we have something like "davmirrorautocheck" like davautocheck which
> exercise all the tests in our test suite, then we can broaden the test
> cases for proxy setup.
In other words, the test is only for neon because the
davmirrorautocheck.sh script (the one you patched) is currently just a
skeletal/rudimentary framework and is yet to be more properly
incorporated into the rest of the cmdline tests? (which will have the
side-effect of solving the issue I mentioned)
That works for me, thanks for the explanation.
(And yes, 'make davmirrorautocheck' sounds interesting!)
Re: svn commit: r1088602 - in /subversion/trunk/subversion: mod_dav_svn/mirror.c
tests/cmdline/dav-mirror-autocheck.sh
Posted by vijay <vi...@collab.net>.
On Tuesday 05 April 2011 03:12 AM, Daniel Shahaf wrote:
> kameshj@apache.org wrote on Mon, Apr 04, 2011 at 12:55:38 -0000:
>> @@ -438,5 +434,29 @@ fi
>>
>> say "PASS: master, slave are both at r4, as expected"
>>
>> -exit 0
>> +# The following test case is for the regression issue triggered by r917523.
>> +# The revision r917523 do some url encodings to the paths and uris which are
>> +# not url-encoded. But there is one additional url-encoding of an uri which is
>> +# already encoded. With this extra encoding, committing a path to slave which
>> +# has space in it fails. Please see this thread
>> +# http://svn.haxx.se/dev/archive-2011-03/0641.shtml for more info.
>> +
>> +say "Test case for regression issue triggered by r917523"
>> +
>> +$svnmucc cp 2 "$BASE_URL/trunk" "$BASE_URL/branch new"
>> +$svnmucc put /dev/null "$BASE_URL/branch new/file" \
>> +--config-option servers:global:http-library=neon
>> +RETVAL=$?
>> +
> This looks bogus, why are you testing neon xor serf rather than both?
>
The reason why the particular commit operation uses neon is here
http://svn.haxx.se/dev/archive-2011-04/0021.shtml
snippet from the thread.
<snip>
The testcase exhibits the following behaviour when tested with neon and
serf.
------------------------------------------------
RA_LAYER HTTP V2 pre-HTTP V2
------------------------------------------------
Neon Fail Fail
------------------------------------------------
Serf Pass Fail
------------------------------------------------
There are two options to make the testcase fail for both neon and serf.
1.Disabling "SVNAdvertiseV2Protocol" by making it explicitly off in the
slave server's Location directive in Apache configuration.
2. Passing the command line option "--config-option
servers:global:http-library=neon" to svnmucc for the particular commit
operation as it fails in neon consistently.
I prefer to the second option(using neon for the particular commit)
instead of disabling HTTP V2 in server side which may affect other
testcases.
> Meanwhile analysing why it succeeds in serf would teach something
> interesting.
>
First, let me tell why it fails in neon.
As in the test case, the slave repo is hosted in 127.0.0.1 and master
repo is hosted in 127.0.0.2
1. For the particular commit which has space in its path, the CHECKOUT
request from client reaches the slave repo.
CHECKOUT /repo/!svn/ver/5/branch%20new HTTP/1.1
User-Agent: SVN/1.7.0-dev (under development) neon/0.29.3
2. The request from the client is proxied by the slave to the master and
the Master sends the response with Location header as follows.
Header Name: [location], Value:
[http://127.0.0.2:30679/repo//!svn/wrk/69179a87-38cb-43c4-945c-de1e0b297aad/branch%20new]
3. The slave encodes the location header again(r917523) and forward it
to the client.
Header Name: [location], Value:
[http://127.0.0.1:26248/repo//!svn/wrk/5b604aca-a4b1-41fc-87ae-51299bf565b2/branch%2520new]
The above location header is used in subsequent dav request which fails
as it has double-encoding of the space.
Here, PUT request fails as follows.
PUT
/repo/!svn/wrk/5b604aca-a4b1-41fc-87ae-51299bf565b2/branch%2520new/file
HTTP/1.1
svnmucc: E160013: File not found: transaction '5-5', path
'/branch%20new/file'
But why didn't it fail in serf?
Serf has the implementations of HTTP V2 stuffs which does not use
CHECKOUT request during commit.
It processes as follows.
OPTIONS -> POST -> PROPFIND -> PROPPATCH -> HEAD -> PUT -> MERGE -> DELETE
It gets the transaction id from POST request and directly put the
contents there.
But I could see the same failure while committing with serf when I
disable "SVNAdvertiseV2Protocol" , because it uses the CHECKOUT request
there.
I think we can use neon for the particular commit operation instead of
disabling HTTP V2 wholly in server side.
Please correct me if I am wrong.
Anyway we can enhance these tests further once we started to implement
"make davmirrorautocheck" which I am going to take as my next activity.
</snip>
The testcase is only for the regression issue triggered by r917523. If
we have something like "davmirrorautocheck" like davautocheck which
exercise all the tests in our test suite, then we can broaden the test
cases for proxy setup.
Thanks & Regards,
Vijayaguru
>> +if [ $RETVAL -eq 0 ] ; then
>> + say "PASS: committing a path which has space in it passes"
>> +else
>> + say "FAIL: committing a path which has space in it fails as there are extra
>> + url-encodings happening in server side"
>> +fi
Re: svn commit: r1088602 - in /subversion/trunk/subversion:
mod_dav_svn/mirror.c tests/cmdline/dav-mirror-autocheck.sh
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
kameshj@apache.org wrote on Mon, Apr 04, 2011 at 12:55:38 -0000:
> @@ -438,5 +434,29 @@ fi
>
> say "PASS: master, slave are both at r4, as expected"
>
> -exit 0
> +# The following test case is for the regression issue triggered by r917523.
> +# The revision r917523 do some url encodings to the paths and uris which are
> +# not url-encoded. But there is one additional url-encoding of an uri which is
> +# already encoded. With this extra encoding, committing a path to slave which
> +# has space in it fails. Please see this thread
> +# http://svn.haxx.se/dev/archive-2011-03/0641.shtml for more info.
> +
> +say "Test case for regression issue triggered by r917523"
> +
> +$svnmucc cp 2 "$BASE_URL/trunk" "$BASE_URL/branch new"
> +$svnmucc put /dev/null "$BASE_URL/branch new/file" \
> +--config-option servers:global:http-library=neon
> +RETVAL=$?
> +
This looks bogus, why are you testing neon xor serf rather than both?
> +if [ $RETVAL -eq 0 ] ; then
> + say "PASS: committing a path which has space in it passes"
> +else
> + say "FAIL: committing a path which has space in it fails as there are extra
> + url-encodings happening in server side"
> +fi