You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by mark benedetto king <bk...@inquira.com> on 2002/09/13 17:22:37 UTC
[PATCH] (alternate fix for issue 900)
Bill Tutt convinced me that adding svn_error_cause() was not a good
idea (people might be tempted to use it). I've inlined it instead.
As an added benefit, the scope of the patch is reduced.
Produce a more appropriate error message when the repository is
found, but cannot be opened (fixes issue 900).
* subversion/include/svn_error_codes.h
SVN_ERR_RA_LOCAL_REPOS_OPEN_FAILED: New error code, distinguished
from SVN_ERR_RA_LOCAL_REPOS_NOT_FOUND in that some error other than
not being able to locate the repository has occurred.
* subversion/libsvn_ra_local/split_url.c
(svn_ra_local__split_URL): Check the inner-most error from
svn_repos_open() to determine why it failed, and produce
an appropriate error message.
Index: subversion/include/svn_error_codes.h
===================================================================
--- subversion/include/svn_error_codes.h
+++ subversion/include/svn_error_codes.h Fri Sep 13 09:44:31 2002
@@ -519,6 +519,10 @@
SVN_ERR_RA_LOCAL_CATEGORY_START + 0,
"Couldn't find a repository.")
+ SVN_ERRDEF (SVN_ERR_RA_LOCAL_REPOS_OPEN_FAILED,
+ SVN_ERR_RA_LOCAL_CATEGORY_START + 1,
+ "Couldn't open the repository.")
+
/* svndiff errors */
SVN_ERRDEF (SVN_ERR_SVNDIFF_INVALID_HEADER,
Index: subversion/libsvn_ra_local/split_url.c
===================================================================
--- subversion/libsvn_ra_local/split_url.c
+++ subversion/libsvn_ra_local/split_url.c Fri Sep 13 13:11:59 2002
@@ -104,6 +104,8 @@
the last component from the URL, then try again. */
while (1)
{
+ svn_error_t *cause;
+
/* Attempt to open a repository at URL. */
err = svn_repos_open (&repos, candidate_url, subpool);
@@ -111,6 +113,16 @@
if (err == SVN_NO_ERROR)
break;
+ cause = err;
+ while (cause->child)
+ cause = cause->child;
+
+ if (cause && !APR_STATUS_IS_ENOENT(cause->apr_err))
+ return svn_error_createf
+ (SVN_ERR_RA_LOCAL_REPOS_OPEN_FAILED, 0, err, pool,
+ "svn_ra_local__split_URL: Unable to open repository\n"
+ " (%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.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] (alternate fix for issue 900)
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
mark benedetto king <bk...@inquira.com> writes:
> > It's not exactly the same, svn_repos_open would not need to follow the
> > error chain. It could just check err->apr_err, which is how all the
> > other error checks work. Doing things differently inevitably raises
> > questions. That doesn't mean your patch is wrong :)
> >
>
> I guess my concern is that one of the several layers between
> apr_file_open() and svn_repos_open() might someday decide to wrap
> up the error from apr_file_open (just like svn_repos_open itself
> does, causing trouble in __split_URL).
Hey, we already have a general solution to this problem... Here I'll
describe it in the abstract:
Suppose caller Q calls function X, which depends on Y which depends on
Z. Now, Q needs to test among certain conditions that can result from
the call to X. And the conditions X returns can depend on Y, etc.
The answer is that the possible error returns need to be documented at
*every layer* in this code path, from the top caller down to the
lowest-level internal library function. Just start at the top call:
figure out what you want to test for, and test for it. Choose your
ideal interface here, since you can add newer and more specific error
returns to callees to achieve that ideal. Now document the
next-highest callee as returning those, and make sure the code does
so. If you need to go farther down and make similar changes, that's
fine.
If some of the callees are already documented to return specific
errors, and you're worried about changing their interfaces, it's
usually pretty easy to check the other callers. Anyway, 90% of the
time, you won't need to make code changes beyond the first or second
call level, you'll just need to document what the code already does.
This is what we've been doing we have such a dependency chain, and I
think ought to work in this situation too.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] (alternate fix for issue 900)
Posted by mark benedetto king <bk...@inquira.com>.
On Sun, Sep 15, 2002 at 08:15:22PM +0100, Philip Martin wrote:
> mark benedetto king <bk...@Inquira.Com> writes:
>
> > What I'm getting at is that svn_repos_open can't tell when
> > to return that error, either, without peeking inside the
> > error in *exactly the same way* that we're describing as
> > inappriate for svn_ra_local__split_URL to do.
> >
> > Am I missing something?
>
> It's not exactly the same, svn_repos_open would not need to follow the
> error chain. It could just check err->apr_err, which is how all the
> other error checks work. Doing things differently inevitably raises
> questions. That doesn't mean your patch is wrong :)
>
I guess my concern is that one of the several layers between
apr_file_open() and svn_repos_open() might someday decide to wrap
up the error from apr_file_open (just like svn_repos_open itself
does, causing trouble in __split_URL).
What if we made the following change? Then, it is the reponsibility
of every error to know what apr_err, if any, triggered it. That
would allay my concerns.
Note that, IMO, this code merely memoizes the results of the
while(cause->child) loops in my previous patches, but maybe everyone
will find this approach more palatable. :-)
ndex: subversion/libsvn_subr/svn_error.c
===================================================================
--- subversion/libsvn_subr/svn_error.c
+++ subversion/libsvn_subr/svn_error.c Sun Sep 15 15:37:31 2002
@@ -102,6 +102,8 @@
new_error->apr_err = apr_err;
new_error->src_err = src_err;
new_error->child = child;
+ if (child && !apr_err)
+ new_error->apr_err = child->apr_err;
new_error->pool = this_pool;
#ifdef SVN_DEBUG
new_error->file = error_file;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] (alternate fix for issue 900)
Posted by Philip Martin <ph...@codematters.co.uk>.
mark benedetto king <bk...@Inquira.Com> writes:
> What I'm getting at is that svn_repos_open can't tell when
> to return that error, either, without peeking inside the
> error in *exactly the same way* that we're describing as
> inappriate for svn_ra_local__split_URL to do.
>
> Am I missing something?
It's not exactly the same, svn_repos_open would not need to follow the
error chain. It could just check err->apr_err, which is how all the
other error checks work. Doing things differently inevitably raises
questions. That doesn't mean your patch is wrong :)
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] (alternate fix for issue 900)
Posted by mark benedetto king <bk...@inquira.com>.
On Sun, Sep 15, 2002 at 05:32:09PM +0100, Philip Martin wrote:
> mark benedetto king <bk...@Inquira.Com> writes:
>
> > On Sat, Sep 14, 2002 at 11:02:37AM +0100, Philip Martin wrote:
> > > svn_repos_open doesn't distinguish between a missing format file and a
> > > format file it cannot read, both cause the function to return
> > > SVN_ERR_REPOS_UNSUPPORTED_VERSION. svn_ra_local__split_URL wants to
> > > distinguish between these two cases. It may be better to change
> > > svn_repos_open, document the behaviour, and then use that in
> > > svn_ra_local__split_URL.
> > >
> >
> > Let's say we want to (break the API and) modify svn_repos_open to
> > look like:
>
> Breaking the API is not really a problem at the moment. However I
> really meant changing svn_repos_open so that it returned more useful
> errors. I think the reason it returns REPOS_UNSUPPORTED_VERSION in
> the case where there is no format file is so that it can support old
> repositories which didn't have such a file. I was questioning whether
> this was the best thing to do. It may be better if it simply passed
> through the ENOENT error from APR, or perhaps returned some sort of
> NO_REPOS error.
>
What I'm getting at is that svn_repos_open can't tell when
to return that error, either, without peeking inside the
error in *exactly the same way* that we're describing as
inappriate for svn_ra_local__split_URL to do.
Am I missing something?
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] (alternate fix for issue 900)
Posted by Philip Martin <ph...@codematters.co.uk>.
mark benedetto king <bk...@Inquira.Com> writes:
> On Sat, Sep 14, 2002 at 11:02:37AM +0100, Philip Martin wrote:
> > svn_repos_open doesn't distinguish between a missing format file and a
> > format file it cannot read, both cause the function to return
> > SVN_ERR_REPOS_UNSUPPORTED_VERSION. svn_ra_local__split_URL wants to
> > distinguish between these two cases. It may be better to change
> > svn_repos_open, document the behaviour, and then use that in
> > svn_ra_local__split_URL.
> >
>
> Let's say we want to (break the API and) modify svn_repos_open to
> look like:
Breaking the API is not really a problem at the moment. However I
really meant changing svn_repos_open so that it returned more useful
errors. I think the reason it returns REPOS_UNSUPPORTED_VERSION in
the case where there is no format file is so that it can support old
repositories which didn't have such a file. I was questioning whether
this was the best thing to do. It may be better if it simply passed
through the ENOENT error from APR, or perhaps returned some sort of
NO_REPOS error.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] (alternate fix for issue 900)
Posted by mark benedetto king <bk...@inquira.com>.
On Sat, Sep 14, 2002 at 11:02:37AM +0100, Philip Martin wrote:
> svn_repos_open doesn't distinguish between a missing format file and a
> format file it cannot read, both cause the function to return
> SVN_ERR_REPOS_UNSUPPORTED_VERSION. svn_ra_local__split_URL wants to
> distinguish between these two cases. It may be better to change
> svn_repos_open, document the behaviour, and then use that in
> svn_ra_local__split_URL.
>
Let's say we want to (break the API and) modify svn_repos_open to
look like:
svn_error_t *
svn_repos_open (svn_repos_t **repos_p,
const char *path,
apr_pool_t *pool,
svn_boolean_t *found)
{
[...]
*found = FALSE;
err = svn_io_read_version_file
(&version, svn_path_join (path, SVN_REPOS__FORMAT, pool), pool);
if (err)
{
/* what to do here? We still need to peek inside err to
determine what went wrong with svn_io_read_version_file.
if we're willing to peek inside err, why not just do
that in svn_ra_local__split_URL and not bother breaking
the API? We can't do some sort of access() style check
because of the TOC/TOU race condition. We need to know
why *this* call failed. We could also modify
svn_io_read_version_file to report more information, but
it, in turn, calls svn_io_file_open, which would *also* need
modification.
If the error chain is truly opaque, then we should move
the definition into svn_error.c, and use an opaque type
for svn_error_t. */
if (0 != SVN_REPOS__VERSION)
return svn_error_createf
(SVN_ERR_REPOS_UNSUPPORTED_VERSION, 0, err, pool,
"Expected version '%d' of repository; found no version at all; "
"is `%s' a valid repository path?",
SVN_REPOS__VERSION, path);
}
*found = TRUE;
[...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] (alternate fix for issue 900)
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Äibej <br...@xbc.nu> writes:
> mark benedetto king wrote:
>
> >Bill Tutt convinced me that adding svn_error_cause() was not a good
> >idea (people might be tempted to use it). I've inlined it instead.
> >As an added benefit, the scope of the patch is reduced.
> >
>
> This is the wrong approach, regardless. We're wrapping the error chain
> already, and svn_handle_error will print out the whole chain. So the
> original "cause" should be apparent. Why isn't it?
svn_repos_open doesn't distinguish between a missing format file and a
format file it cannot read, both cause the function to return
SVN_ERR_REPOS_UNSUPPORTED_VERSION. svn_ra_local__split_URL wants to
distinguish between these two cases. It may be better to change
svn_repos_open, document the behaviour, and then use that in
svn_ra_local__split_URL.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: Re: [PATCH] (alternate fix for issue 900)
Posted by Bill Tutt <ra...@lyra.org>.
I didn't really get that bit either. I was kind of assuming it was some
sort of more specific/useful error message wrapping.
Bill
----
Do you want a dangerous fugitive staying in your flat?
No.
Well, don't upset him and he'll be a nice fugitive staying in your flat.
> -----Original Message-----
> From: Branko Cibej [mailto:brane@xbc.nu]
> Sent: Friday, September 13, 2002 5:50 PM
> To: mark benedetto king
> Cc: dev@subversion.tigris.org
> Subject: Re: [PATCH] (alternate fix for issue 900)
>
> mark benedetto king wrote:
>
> >Bill Tutt convinced me that adding svn_error_cause() was not a good
> >idea (people might be tempted to use it). I've inlined it instead.
> >As an added benefit, the scope of the patch is reduced.
> >
> >
> This is the wrong approach, regardless. We're wrapping the error chain
> already, and svn_handle_error will print out the whole chain. So the
> original "cause" should be apparent. Why isn't it?
>
> --
> Brane Čibej <br...@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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] (alternate fix for issue 900)
Posted by Branko Čibej <br...@xbc.nu>.
mark benedetto king wrote:
>Bill Tutt convinced me that adding svn_error_cause() was not a good
>idea (people might be tempted to use it). I've inlined it instead.
>As an added benefit, the scope of the patch is reduced.
>
>
This is the wrong approach, regardless. We're wrapping the error chain
already, and svn_handle_error will print out the whole chain. So the
original "cause" should be apparent. Why isn't it?
--
Brane Čibej <br...@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