Hi Greg, and thanks a lot for reviewing the patch!
Greg Hudson wrote:
> I'm not sure whether I like the approach of having one bit of code to
> test whether a directory looks like a repository root, and another bit
> to open it. But it does make certain things simpler. (I tried fixing
> the problem without taking that approach, and ran into trouble,
> certainly.)
I think it does make the overall code cleaner. The open_repos loops were
not pretty.
> You really shouldn't leave behind variables named "candidate" in the two
> functions you gutted. "Candidate" suggests it's in competition with
> other possible candidates, but that's not the case any more; it's the
> only path we're going to try.
Your are right. I've changed it to repos_root instead. Fixed.
> If we're not going to look at repositories which don't have format
> files, then check_repos_version() shouldn't try to handle that case. (I
> don't think that code ever had any effect, actually; I think by the time
> it was introduced, SVN_REPOS__VERSION was already non-zero.)
I saw that code, but I did not want to make the patch larger and harder
to review by changing that too, but I guess I should have. :-) Fixed!
> Your diff against serve.c looks like it's against a slightly old version
> of that file.
That's odd. The base revision is from trunk revision 6838. I did an
update from trunk minutes before I sent the email. Are you sure I'm the
one with the slightly old version? ;-)
> + err = svn_io_check_path (svn_path_join (path, SVN_REPOS__FORMAT, pool),
> + &kind, pool);
>
> The second line should line up with the svn_path_join. (There are other
> examples of this problem.)
What do you mean? It looks lined up to me, and it looks just like other
functions in that file. If you show me how you want it to look I'm sure
I'll understand.
> + if (err)
> + {
> + return svn_error_createf(SVN_ERR_RA_SVN_REPOS_NOT_FOUND, err,
> + "No repository found in '%s'", url);
> + }
>
> Some source files choose to wrap single-statement, multi-line bodies in
> braces. serve.c chooses not to. Please do not change that in a
> piece-by-piece manner.
My apologies. I've fixed that one, and ones in repos.c and split_url.c
as well.
Thanks again for reviewing the patch! I've attached the updated patch.
The md5sum is a2175e22d8e91eba0fedfdb9e6980bad, and I sure hope that
it's a coincident that the sum end with "bad". :-)
/Tobias
------------------ log message ----------------------
The methods used by ra_local and svnserve to find the repository of an
URL were different. This was causing different behaviour in case of
errors. Fix the error handling to make sure that the error codes are
propagated to the user.
* subversion/include/svn_repos.h
(svn_repos_find_root_path): New
* libsvn_repos/repos.c
(check_repos_path): New
(check_repos_version): Require an existing format file.
(svn_repos_find_root_path): New
* libsvn_ra_local/split_url.c
(svn_ra_local__split_URL): Use the new svn_repos_find_root_path
function instead of looping and calling svn_repos_open. Renamed
candidate_url to candidate since it is not an URL. Propagate the
error from svn_repos_open if it fails so the user can get a clue.
* svnserve/serve.c
(find_repos): Use the new svn_repos_find_root_path function instead
of looping and calling svn_repos_open. Propagate the error from
svn_repos_open if it fails so the user can get a clue.
Index: subversion/include/svn_repos.h
===================================================================
--- subversion/include/svn_repos.h (revision 6839)
+++ subversion/include/svn_repos.h (working copy)
@@ -44,6 +44,16 @@
/* Opening and creating repositories. */
+/** Find the root path of the repository that contains @a path by
+ * removing a component at a time from the end of @a path until a
+ * likely repository root path is found.
+ *
+ * If a repository was found, the path to the root of the repository
+ * is returned, else @c NULL.
+ */
+char *svn_repos_find_root_path (const char *path,
+ apr_pool_t *pool);
+
/** Set @a *repos_p to a repository object for the repository at @a path.
*
* Allocate @a *repos_p in @a pool.
Index: subversion/libsvn_ra_local/split_url.c
===================================================================
--- subversion/libsvn_ra_local/split_url.c (revision 6839)
+++ subversion/libsvn_ra_local/split_url.c (working copy)
@@ -30,7 +30,7 @@
apr_pool_t *pool)
{
svn_error_t *err = SVN_NO_ERROR;
- const char *candidate_url;
+ const char *repos_root;
const char *hostname, *path;
/* Decode the URL, as we only use its parts as filesystem paths
@@ -69,7 +69,7 @@
/* Duplicate the URL, starting at the top of the path */
#ifndef SVN_WIN32
- candidate_url = apr_pstrdup (pool, path);
+ repos_root = apr_pstrdup (pool, path);
#else /* SVN_WIN32 */
/* On Windows, we'll typically have to skip the leading / if the
path starts with a drive letter. Like most Web browsers, We
@@ -92,72 +92,30 @@
char *const dup_path = apr_pstrdup (pool, ++path);
if (dup_path[1] == '|')
dup_path[1] = ':';
- candidate_url = dup_path;
+ repos_root = dup_path;
}
else
- candidate_url = apr_pstrdup (pool, path);
+ repos_root = apr_pstrdup (pool, path);
}
#endif /* SVN_WIN32 */
- /* Loop, trying to open a repository at URL. If this fails, remove
- the last component from the URL, then try again. */
- while (1)
- {
- /* Attempt to open a repository at URL. */
- err = svn_repos_open (repos, candidate_url, pool);
+ /* Search for a repository in the full path. */
+ repos_root = svn_repos_find_root_path(repos_root, pool);
+ if (!repos_root)
+ return svn_error_createf
+ (SVN_ERR_RA_LOCAL_REPOS_OPEN_FAILED, NULL,
+ "Unable to open repository '%s'", URL);
- /* Hey, cool, we were successful. Stop looping. */
- if (err == SVN_NO_ERROR)
- break;
-
- /* If we get an error -other- than the path or 'format' file not
- existing, then throw the error immediately. For example, we
- want permissions errors to be seen right away. */
- if ((! APR_STATUS_IS_ENOENT(err->apr_err))
- && (err->apr_err != SVN_ERR_REPOS_UNSUPPORTED_VERSION))
- return svn_error_createf
- (SVN_ERR_RA_LOCAL_REPOS_OPEN_FAILED, err,
- "Unable to open repository '%s'", URL);
-
- /* It would be strange indeed if "/" were a repository, but hey,
- people do strange things sometimes. Anyway, if "/" failed
- the test above, then reduce it to the empty string.
-
- ### I'm not sure whether SVN_EMPTY_PATH and SVN_PATH_IS_EMPTY
- in libsvn_subr/path.c is are supposed to be changeable. If
- they are, this code could conceivably break. If they're not,
- then we should probably make SVN_PATH_EMPTY_PATH a public
- constant and use it here. But either way, note that the test
- for '/' below is kosher, because we know it's the canonical
- APR separator. */
- if ((candidate_url[0] == '/') && (candidate_url[1] == '\0'))
- candidate_url = "";
-
- /* If we're down to an empty path here, and we still haven't
- found the repository, we're just out of luck. Time to bail
- and face the music. */
- if (svn_path_is_empty (candidate_url))
- break;
-
- /* We didn't successfully open the repository, and we haven't
- hacked this path down to a bare nub yet, so we'll chop off
- the last component of this path. */
- candidate_url = svn_path_dirname (candidate_url, pool);
- if (err)
- svn_error_clear (err);
- }
-
- /* If we are still sitting in an error-ful state, we must not have
- found the repository. We give up. */
+ /* Attempt to open a repository at URL. */
+ err = svn_repos_open (repos, repos_root, pool);
if (err)
return svn_error_createf
- (SVN_ERR_RA_LOCAL_REPOS_NOT_FOUND, NULL,
- "svn_ra_local__split_URL: Unable to find valid repository\n"
- " (%s)", URL);
+ (SVN_ERR_RA_LOCAL_REPOS_OPEN_FAILED, err,
+ "Unable to open repository '%s'", URL);
/* What remains of URL after being hacked at in the previous step is
REPOS_URL. FS_PATH is what we've hacked off in the process. */
- *fs_path = apr_pstrdup (pool, path + strlen (candidate_url));
+ *fs_path = apr_pstrdup (pool, path + strlen (repos_root));
*repos_url = apr_pstrmemdup (pool, URL, strlen(URL) - strlen(*fs_path));
return SVN_NO_ERROR;
Index: subversion/libsvn_repos/repos.c
===================================================================
--- subversion/libsvn_repos/repos.c (revision 6839)
+++ subversion/libsvn_repos/repos.c (working copy)
@@ -1043,6 +1043,39 @@
}
+/* Check if @a path is the root of a repository by checking if the
+ * path contains the expected files and directories. */
+static svn_boolean_t
+check_repos_path (const char *path,
+ apr_pool_t *pool)
+{
+ svn_node_kind_t kind;
+ svn_error_t *err;
+
+ err = svn_io_check_path (svn_path_join (path, SVN_REPOS__FORMAT, pool),
+ &kind, pool);
+ if (err)
+ {
+ svn_error_clear (err);
+ return TRUE;
+ }
+ if (kind != svn_node_file)
+ return FALSE;
+
+ err = svn_io_check_path (svn_path_join (path, SVN_REPOS__DB_DIR, pool),
+ &kind, pool);
+ if (err)
+ {
+ svn_error_clear (err);
+ return TRUE;
+ }
+ if (kind != svn_node_dir)
+ return FALSE;
+
+ return TRUE;
+}
+
+
/* Verify that the repository's 'format' file is a suitable version. */
static svn_error_t *
check_repos_version (const char *path,
@@ -1051,21 +1084,14 @@
int version;
svn_error_t *err;
- /* ### for now, an error here might occur because we *just*
- introduced the whole format thing. Until the next time we
- *change* our format, we'll ignore the error (and default to a 0
- version). */
err = svn_io_read_version_file
(&version, svn_path_join (path, SVN_REPOS__FORMAT, pool), pool);
if (err)
- {
- if (0 != SVN_REPOS__VERSION)
- return svn_error_createf
- (SVN_ERR_REPOS_UNSUPPORTED_VERSION, err,
- "Expected version '%d' of repository; found no version at all; "
- "is `%s' a valid repository path?",
- SVN_REPOS__VERSION, path);
- }
+ return svn_error_createf
+ (SVN_ERR_REPOS_UNSUPPORTED_VERSION, err,
+ "Expected version '%d' of repository; found no version at all; "
+ "is `%s' a valid repository path?",
+ SVN_REPOS__VERSION, path);
if (version != SVN_REPOS__VERSION)
return svn_error_createf
@@ -1155,6 +1181,25 @@
+char *
+svn_repos_find_root_path (const char *path,
+ apr_pool_t *pool)
+{
+ char *candidate = apr_pstrdup (pool, path);
+
+ while (1)
+ {
+ if (check_repos_path (candidate, pool))
+ break;
+ if (candidate[0] == '\0' || strcmp(candidate, "/") == 0)
+ return NULL;
+ candidate = svn_path_dirname (candidate, pool);
+ }
+
+ return candidate;
+}
+
+
svn_error_t *
svn_repos_open (svn_repos_t **repos_p,
const char *path,
Index: subversion/svnserve/serve.c
===================================================================
--- subversion/svnserve/serve.c (revision 6839)
+++ subversion/svnserve/serve.c (working copy)
@@ -842,7 +842,7 @@
{
svn_error_t *err;
apr_status_t apr_err;
- const char *client_path, *full_path, *candidate;
+ const char *client_path, *full_path, *repos_root;
const char *client_path_apr, *root_apr;
char *buffer;
@@ -877,19 +877,16 @@
full_path = svn_path_canonicalize(full_path, pool);
/* Search for a repository in the full path. */
- candidate = full_path;
- while (1)
- {
- err = svn_repos_open(repos, candidate, pool);
- if (err == SVN_NO_ERROR)
- break;
- svn_error_clear(err);
- if (!*candidate || strcmp(candidate, "/") == 0)
- return svn_error_createf(SVN_ERR_RA_SVN_REPOS_NOT_FOUND, NULL,
- "No repository found in '%s'", url);
- candidate = svn_path_dirname(candidate, pool);
- }
- *fs_path = apr_pstrdup(pool, full_path + strlen(candidate));
+ repos_root = svn_repos_find_root_path(full_path, pool);
+ if (!repos_root)
+ return svn_error_createf(SVN_ERR_RA_SVN_REPOS_NOT_FOUND, NULL,
+ "No repository found in '%s'", url);
+
+ err = svn_repos_open(repos, repos_root, pool);
+ if (err)
+ return svn_error_createf(SVN_ERR_RA_SVN_REPOS_NOT_FOUND, err,
+ "No repository found in '%s'", url);
+ *fs_path = apr_pstrdup(pool, full_path + strlen(repos_root));
*repos_url = apr_pstrmemdup(pool, url, strlen(url) - strlen(*fs_path));
return SVN_NO_ERROR;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Aug 23 11:56:21 2003