Re: r916286

[Kamesh Jayachandran]

On 05/18/2011 02:04 AM, Paul Burba wrote:
Author: kameshj
Date: Thu Feb 25 13:40:22 2010
New Revision: 916286
URL: http://svn.apache.org/viewvc?rev=916286&view=rev
Log:
With the below apache configuration(See the trailing slash at the end
of '/svn/').

<Location /svn/>
  DAV svn
  SVNParentPath /repositories
#See the trailing slash on the master URI also can cause the confusion.
  SVNMasterURI http://master/svn/
  SVNAdvertiseV2Protocol Off
</Location>

We get the following error on the client side.

svn: Commit failed (details follow):
svn: MKACTIVITY of
'/svn/demujin/!svn/act/4b6d547c-018d-4e02-9d3f-2b283076cc06': Could
not
read status line: connection was closed by server (http://localhost)
Hi Kamesh,


Sorry for the late response Paul.
Are there any threads or IRC logs in which this is discussed
(particularly a more detailed replication)?

No. Paul this error caught my eyes while testing something locally.

While reviewing r916286
and r917512 I tried to replicate this problem by adding a trailing '/'
to the location and SVNMasterURI.  I *did* get an error, just a
different one:

C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slave>svn
st
M       file.txt

C:\SVN\src-branch-1.6.x\Debug\subversion\tests\cmdline\svn-test-work\working_copies\slave>svn
ci -m "commit to slave"
svn: Commit failed (details follow):
svn: OPTIONS of 'http://localhost/svn-test-work/slave': 200 OK
(http://localhost)



What's worse is that I get this error with a server at the latest
trunk (r1104523), trunk right after you fixed this bug (r917512), and
my own local attempt to backport this branch to 1.6.x (addressing the
conflicts and the use of 1.7 APIs).  They all fail the same way.


I could not see this error(I mean everything works as expected with 
<Location /svn/> and SVNMasterURI witj trailing '/') with against 
trunk@1126459

I built trunk@916285(prior to my fix) and saw this error and with 
trunk@916286 this error has gone away.


May be something to do with win32 build.

Can you attach your patch against r916285, I will build it/test it and 
let you know what is wrong?

With regards
Kamesh Jayachandran
Any insight is appreciated.

Paul

On the server(proxy) it is an assertion error on the following code
block from subversion/mod_dav_svn/mirror.c:proxy_request_fixup

   assert((uri_segment[0] == '\0')
           || (uri_segment[0] == '/'));

For the above configuration we get the uri_segment with the value
'reponame/some/path/inside/the/repo'.

We fix this by canonicalizing the 'root_dir'(The one in Location) and
'uri.path'(Path portion of Master URI).
* subversion/mod_dav_svn/dav_svn.h
(dav_svn__get_root_dir): Document that root_dir is in canonicalized form.
* subversion/mod_dav_svn/mod_dav_svn.c
(create_dir_config): Canonicalize the root_dir.
* subversion/mod_dav_svn/mirror.c
(dav_svn__location_in_filter, dav_svn__location_body_filter):
As root_dir is in canonical form canonicalize the uri.path too to avoid
spurious errors.
(dav_svn__location_header_filter): As root_dir is canonical we need to
  explicitly introduce the path seperator.

Suggested by: julianfoad

Modified:
    subversion/trunk/subversion/mod_dav_svn/dav_svn.h
    subversion/trunk/subversion/mod_dav_svn/mirror.c
    subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c

Modified: subversion/trunk/subversion/mod_dav_svn/dav_svn.h
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/dav_svn.h?rev=916286&r1=916285&r2=916286&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/dav_svn.h (original)
+++ subversion/trunk/subversion/mod_dav_svn/dav_svn.h Thu Feb 25 13:40:22 2010
@@ -352,7 +352,7 @@
  /* Return the activities db */
  const char * dav_svn__get_activities_db(request_rec *r);

-/* Return the root directory */
+/* Return the root directory in canonicalized form */
  const char * dav_svn__get_root_dir(request_rec *r);



Modified: subversion/trunk/subversion/mod_dav_svn/mirror.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mirror.c?rev=916286&r1=916285&r2=916286&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mirror.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mirror.c Thu Feb 25 13:40:22 2010
@@ -128,7 +128,7 @@
     locate_ctx_t *ctx = f->ctx;
     apr_status_t rv;
     apr_bucket *bkt;
-    const char *master_uri, *root_dir;
+    const char *master_uri, *root_dir, *canonicalized_uri;
     apr_uri_t uri;

     /* Don't filter if we're in a subrequest or we aren't setup to
@@ -143,7 +143,11 @@
        (that is, if our root path matches that of the master server). */
     apr_uri_parse(r->pool, master_uri,&uri);
     root_dir = dav_svn__get_root_dir(r);
-    if (strcmp(uri.path, root_dir) == 0) {
+    if (uri.path)
+        canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
+    else
+        canonicalized_uri = uri.path;
+    if (strcmp(canonicalized_uri, root_dir) == 0) {
         ap_remove_input_filter(f);
         return ap_get_brigade(f->next, bb, mode, block, readbytes);
     }
@@ -156,7 +160,7 @@

     if (!f->ctx) {
         ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
-        ctx->remotepath = uri.path;
+        ctx->remotepath = canonicalized_uri;
         ctx->remotepath_len = strlen(ctx->remotepath);
         ctx->localpath = root_dir;
         ctx->localpath_len = strlen(ctx->localpath);
@@ -226,7 +230,7 @@
         start_foo += strlen(master_uri);
         new_uri = ap_construct_url(r->pool,
                                    apr_pstrcat(r->pool,
-                                               dav_svn__get_root_dir(r),
+                                               dav_svn__get_root_dir(r), "/",
                                                start_foo, NULL),
                                    r);
         apr_table_set(r->headers_out, "Location", new_uri);
@@ -240,7 +244,7 @@
     request_rec *r = f->r;
     locate_ctx_t *ctx = f->ctx;
     apr_bucket *bkt;
-    const char *master_uri, *root_dir;
+    const char *master_uri, *root_dir, *canonicalized_uri;
     apr_uri_t uri;

     /* Don't filter if we're in a subrequest or we aren't setup to
@@ -255,7 +259,11 @@
        (that is, if our root path matches that of the master server). */
     apr_uri_parse(r->pool, master_uri,&uri);
     root_dir = dav_svn__get_root_dir(r);
-    if (strcmp(uri.path, root_dir) == 0) {
+    if (uri.path)
+        canonicalized_uri = svn_dirent_canonicalize(uri.path, r->pool);
+    else
+        canonicalized_uri = uri.path;
+    if (strcmp(canonicalized_uri, root_dir) == 0) {
         ap_remove_output_filter(f);
         return ap_pass_brigade(f->next, bb);
     }
@@ -268,7 +276,7 @@

     if (!f->ctx) {
         ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
-        ctx->remotepath = uri.path;
+        ctx->remotepath = canonicalized_uri;
         ctx->remotepath_len = strlen(ctx->remotepath);
         ctx->localpath = root_dir;
         ctx->localpath_len = strlen(ctx->localpath);

Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=916286&r1=916285&r2=916286&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Thu Feb 25
13:40:22 2010
@@ -169,7 +169,8 @@
   /* NOTE: dir==NULL creates the default per-dir config */
   dir_conf_t *conf = apr_pcalloc(p, sizeof(*conf));

-  conf->root_dir = dir;
+  if (dir)
+    conf->root_dir = svn_dirent_canonicalize(dir, p);
   conf->bulk_updates = CONF_FLAG_ON;
   conf->v2_protocol = CONF_FLAG_ON;