Paul Marculescu wrote:
>Branko ?ibej wrote:
>
>
>>As I've said before:
>>
>> * First, we have to replace our URL parsing code with the utilities
>> from apr-util
>> * Then fix apr-util to do the right thing on Windows.
>>
>>
>>
>
>I made a patch for apr_uri.c and send it on dev@apr.apache.org.
>I also made a little patch for libsvn_ra_local/split_url.c to use
>apr_uri_parse().
>
Please take a look at the HACKING file, and do include a log message
with patches, thanks.
>Index: split_url.c
>===================================================================
>RCS file: e:/cvsroot/cvstest/split_url.c,v
>retrieving revision 1.1.1.1
>diff -u -r1.1.1.1 split_url.c
>
Hmmm... I wonder if something is wrong with "svn diff"? people seem to
be using local CVS repos instead :-)
>--- split_url.c 2002/06/03 02:14:45 1.1.1.1
>+++ split_url.c 2002/06/03 02:21:31
>@@ -20,7 +20,7 @@
> #include <assert.h>
> #include <string.h>
> #include "svn_pools.h"
>-
>+#include <apr_uri.h>
>
Very minor nit: don't remove the empty line. (And possibly include
<apr_uri.h> before "svn_pools.h", but that's just my preferred bikeshed
colour ...)
> svn_error_t *
> svn_ra_local__split_URL (const svn_string_t **repos_path,
> const svn_string_t **fs_path,
>@@ -28,16 +28,17 @@
> apr_pool_t *pool)
> {
> svn_error_t *err;
>- svn_stringbuf_t *url;
>- char *hostname, *url_data, *path;
>+ svn_stringbuf_t *url;
>
Looks like you added a couple of spaces to the end of this line. This
happened several times below, too. Try to avoid that, as it makes the
patch larger and harder to read, without adding any semantic content.
> apr_pool_t *subpool = svn_pool_create (pool);
> svn_repos_t *repos;
>+ apr_uri_t uri;
>+
>+ apr_uri_parse( pool, URL->data, &uri );
>
Oops. We do all sorts of strange things with spaces around parens, but
not that. :-)
Try to emulate the style of the surrounding code. In this case, it should be
apr_uri_parse (pool, URL->data, &uri);
>- /* Verify that the URL is well-formed (loosely) */
>- url_data = URL->data;
>+ /* Verify that the URL is well-formed (loosely) */
>
Trailing spaces again ...
> /* First, check for the "file://" prefix. */
>- if (memcmp ("file://", url_data, 7))
>+ if (!uri.scheme || memcmp ("file", uri.scheme, 4))
> return svn_error_create
> (SVN_ERR_RA_ILLEGAL_URL, 0, NULL, pool,
> ("svn_ra_local__split_URL: URL does not contain `file://'
>prefix"));
>@@ -45,23 +46,21 @@
> /* Then, skip what's between the "file://" prefix and the next
> occurance of '/' -- this is the hostname, and we are considering
> everything from that '/' until the end of the URL to be the
>- absolute path portion of the URL. */
>- hostname = url_data + 7;
>- path = strchr (hostname, '/');
>- if (! path)
>+ absolute path portion of the URL. */
>+ if (!uri.path)
> return svn_error_create
> (SVN_ERR_RA_ILLEGAL_URL, 0, NULL, pool,
> ("svn_ra_local__split_URL: URL contains only a hostname, no
>path"));
>
Your mailer wraps long lines. That's not good for patches, because they
don't apply cleanly.
> /* Currently, the only hostnames we are allowing are the empty
> string and 'localhost' */
>- if ((hostname != path) && (memcmp (hostname, "localhost", 9)))
>+ if (uri.hostname[0] != 0 && memcmp (uri.hostname, "localhost", 9))
> return svn_error_create
> (SVN_ERR_RA_ILLEGAL_URL, 0, NULL, pool,
> ("svn_ra_local__split_URL: URL contains unsupported hostname"));
>
> /* Duplicate the URL, starting at the top of the path */
>- url = svn_stringbuf_create ((const char *)path, subpool);
>+ url = svn_stringbuf_create ((const char *)uri.path, subpool);
>
Where did this come from? My copy of split_url.c has the following line
here:
candidate_url = apr_pstrdup (subpool, path);
It looks like you're not comparing against the latest version of that
file. One more reason to use svn instead of copying stuff into a local
CVS repo.
> /* Loop, trying to open a repository at URL. If this fails, remove
> the last component from the URL, then try again. */
>@@ -101,7 +100,7 @@
> REPOS_PATH. FS_PATH is what we've hacked off in the process. We
> need to make sure these are allocated in the -original- pool. */
> *repos_path = svn_string_create_from_buf (url, pool);
>- *fs_path = svn_string_create (path + url->len, pool);
>+ *fs_path = svn_string_create (uri.path + url->len, pool);
>
Same here. The code in split_url.c is quite different.This makes me
wonder if there arent other discrepancies in the patch.
> /* Destroy our temporary memory pool. */
> svn_pool_destroy (subpool);
>
>
--
Brane Čibej <brane_at_xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jun 3 20:41:47 2002