You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by pe...@apache.org on 2010/03/31 07:13:45 UTC
svn commit: r929382 -
/subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
Author: peters
Date: Wed Mar 31 05:13:44 2010
New Revision: 929382
URL: http://svn.apache.org/viewvc?rev=929382&view=rev
Log:
On 1.6.x-wc-ng-error branch:
* subversion/libsvn_wc/questions.c
(is_inside_wc_ng): Ignore errors trying to open wc.db in parent
directories. Typically this is a filesystem error which has nothing
to do with wc-ng and everything to do with permissions or similar.
Modified:
subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
Modified: subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c?rev=929382&r1=929381&r2=929382&view=diff
==============================================================================
--- subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c (original)
+++ subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c Wed Mar 31 05:13:44 2010
@@ -54,6 +54,7 @@ is_inside_wc_ng(const char *abspath,
svn_node_kind_t kind;
const char *wc_db_path;
char *wc_ng_check_env_var;
+ svn_error_t *err;
wc_ng_check_env_var = getenv(SVN_WC_NG_CHECK_ENV_VAR);
if (wc_ng_check_env_var &&
@@ -62,7 +63,12 @@ is_inside_wc_ng(const char *abspath,
wc_db_path = svn_path_join_many(pool, abspath, SVN_WC_ADM_DIR_NAME,
"wc.db", NULL);
- SVN_ERR(svn_io_check_path(wc_db_path, &kind, pool));
+ err = svn_io_check_path(wc_db_path, &kind, pool);
+ if (err)
+ {
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+ }
if (kind == svn_node_file)
{
Re: svn commit: r929382 -
/subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
Posted by Julian Foad <ju...@wandisco.com>.
Peter Samuelson wrote:
> [Daniel Shahaf]
> > > @@ -62,7 +63,12 @@ is_inside_wc_ng(const char *abspath,
> > >
> > > wc_db_path = svn_path_join_many(pool, abspath, SVN_WC_ADM_DIR_NAME,
> > > "wc.db", NULL);
> >
> > Shouldn't this use svn_wc_get_adm_dir()?
>
> Probably ... but since this is 1.6-specific code (it is not and never
> will be in trunk), I think we'd need a good reason to bother changing
> it.
>
> > > - SVN_ERR(svn_io_check_path(wc_db_path, &kind, pool));
> > > + err = svn_io_check_path(wc_db_path, &kind, pool);
> > > + if (err)
> > > + {
> > > + svn_error_clear(err);
> > > + return SVN_NO_ERROR;
> > > + }
> > >
> >
> > Given the rationale (in the log message), do we want to ignore *any*
> > errors here? Or only errors related to (for example) permissions.
>
> Well, as we just saw with the r908980 group, we can't always predict
> all the errors we can get. For example, would you have thought of
> ELOOP (too many symbolic link traversals)? EFAULT? ENAMETOOLONG?
> (I got those from 'man 2 open' on my system.)
>
> There are many reasons you may not be able to open
> ../../../../.svn/wc.db, which may not even be on the same filesystem as
> the current directory (common, if /home is its own partition). I don't
> think it is the job of is_inside_wc_db() to discover these things and
> abort your operation. If it can't read wc.db, the answer should be "we
> didn't find a wc-ng", not "there might be a wc-ng up there somewhere,
> because we can't prove there isn't". In other words, it's a question
> of where the burden of proof lies.
>
> The worst that can happen is, we allow the user to create a 1.6 wc
> where there was already a 1.7 wc rooted one or more directories up from
> us which we weren't able to get to for whatever reason. I think that's
> a pretty benign failure case.
>
> Peter
+1 to what Peter says.
- Julian
Re: svn commit: r929382 -
/subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
Posted by Peter Samuelson <pe...@p12n.org>.
[Bert Huijben]
> > > > wc_db_path = svn_path_join_many(pool, abspath,
> > SVN_WC_ADM_DIR_NAME,
> > > > "wc.db", NULL);
> > >
> > > Shouldn't this use svn_wc_get_adm_dir()?
> >
> > Probably ... but since this is 1.6-specific code (it is not and never
> > will be in trunk), I think we'd need a good reason to bother changing
> > it.
>
> Is the common use of "_svn" by setting an environment variable, on
> Windows a good enough reason for you?
Yes, that would be a good reason. But as Julian noted on IRC, it's a
separate bugfix, so please patch and propose it separately.
Peter
RE: svn commit: r929382 - /subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Peter Samuelson [mailto:peter@p12n.org]
> Sent: woensdag 31 maart 2010 18:33
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r929382 - /subversion/branches/1.6.x-wc-ng-
> error/subversion/libsvn_wc/questions.c
>
>
> [Daniel Shahaf]
> > > @@ -62,7 +63,12 @@ is_inside_wc_ng(const char *abspath,
> > >
> > > wc_db_path = svn_path_join_many(pool, abspath,
> SVN_WC_ADM_DIR_NAME,
> > > "wc.db", NULL);
> >
> > Shouldn't this use svn_wc_get_adm_dir()?
>
> Probably ... but since this is 1.6-specific code (it is not and never
> will be in trunk), I think we'd need a good reason to bother changing
> it.
Is the common use of "_svn" by setting an environment variable, on Windows a
good enough reason for you?
Bert
Re: svn commit: r929382 -
/subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
Posted by Peter Samuelson <pe...@p12n.org>.
[Daniel Shahaf]
> > @@ -62,7 +63,12 @@ is_inside_wc_ng(const char *abspath,
> >
> > wc_db_path = svn_path_join_many(pool, abspath, SVN_WC_ADM_DIR_NAME,
> > "wc.db", NULL);
>
> Shouldn't this use svn_wc_get_adm_dir()?
Probably ... but since this is 1.6-specific code (it is not and never
will be in trunk), I think we'd need a good reason to bother changing
it.
> > - SVN_ERR(svn_io_check_path(wc_db_path, &kind, pool));
> > + err = svn_io_check_path(wc_db_path, &kind, pool);
> > + if (err)
> > + {
> > + svn_error_clear(err);
> > + return SVN_NO_ERROR;
> > + }
> >
>
> Given the rationale (in the log message), do we want to ignore *any*
> errors here? Or only errors related to (for example) permissions.
Well, as we just saw with the r908980 group, we can't always predict
all the errors we can get. For example, would you have thought of
ELOOP (too many symbolic link traversals)? EFAULT? ENAMETOOLONG?
(I got those from 'man 2 open' on my system.)
There are many reasons you may not be able to open
../../../../.svn/wc.db, which may not even be on the same filesystem as
the current directory (common, if /home is its own partition). I don't
think it is the job of is_inside_wc_db() to discover these things and
abort your operation. If it can't read wc.db, the answer should be "we
didn't find a wc-ng", not "there might be a wc-ng up there somewhere,
because we can't prove there isn't". In other words, it's a question
of where the burden of proof lies.
The worst that can happen is, we allow the user to create a 1.6 wc
where there was already a 1.7 wc rooted one or more directories up from
us which we weren't able to get to for whatever reason. I think that's
a pretty benign failure case.
Peter
Re: svn commit: r929382 -
/subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
(taking to dev@ from IRC)
peters@apache.org wrote on Wed, 31 Mar 2010 at 05:13 -0000:
> On 1.6.x-wc-ng-error branch:
>
> * subversion/libsvn_wc/questions.c
> (is_inside_wc_ng): Ignore errors trying to open wc.db in parent
> directories. Typically this is a filesystem error which has nothing
> to do with wc-ng and everything to do with permissions or similar.
>
...
> +++ subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c Wed Mar 31 05:13:44 2010
> @@ -62,7 +63,12 @@ is_inside_wc_ng(const char *abspath,
>
> wc_db_path = svn_path_join_many(pool, abspath, SVN_WC_ADM_DIR_NAME,
> "wc.db", NULL);
Shouldn't this use svn_wc_get_adm_dir()?
> - SVN_ERR(svn_io_check_path(wc_db_path, &kind, pool));
> + err = svn_io_check_path(wc_db_path, &kind, pool);
> + if (err)
> + {
> + svn_error_clear(err);
> + return SVN_NO_ERROR;
> + }
>
Given the rationale (in the log message), do we want to ignore *any*
errors here? Or only errors related to (for example) permissions.
(Note that the ../../../.svn/wc.db accessed here may be anywhere, above
or below one's home dir, etc.)
> if (kind == svn_node_file)
> {
>
>
>
Re: svn commit: r929382 -
/subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
(taking to dev@ from IRC)
peters@apache.org wrote on Wed, 31 Mar 2010 at 05:13 -0000:
> On 1.6.x-wc-ng-error branch:
>
> * subversion/libsvn_wc/questions.c
> (is_inside_wc_ng): Ignore errors trying to open wc.db in parent
> directories. Typically this is a filesystem error which has nothing
> to do with wc-ng and everything to do with permissions or similar.
>
...
> +++ subversion/branches/1.6.x-wc-ng-error/subversion/libsvn_wc/questions.c Wed Mar 31 05:13:44 2010
> @@ -62,7 +63,12 @@ is_inside_wc_ng(const char *abspath,
>
> wc_db_path = svn_path_join_many(pool, abspath, SVN_WC_ADM_DIR_NAME,
> "wc.db", NULL);
Shouldn't this use svn_wc_get_adm_dir()?
> - SVN_ERR(svn_io_check_path(wc_db_path, &kind, pool));
> + err = svn_io_check_path(wc_db_path, &kind, pool);
> + if (err)
> + {
> + svn_error_clear(err);
> + return SVN_NO_ERROR;
> + }
>
Given the rationale (in the log message), do we want to ignore *any*
errors here? Or only errors related to (for example) permissions.
(Note that the ../../../.svn/wc.db accessed here may be anywhere, above
or below one's home dir, etc.)
> if (kind == svn_node_file)
> {
>
>
>