You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by br...@apache.org on 2013/11/14 04:08:36 UTC
svn commit: r1541793 - in /subversion/branches/1.8.x-r1541790: ./
subversion/mod_dav_svn/mod_dav_svn.c
Author: breser
Date: Thu Nov 14 03:08:33 2013
New Revision: 1541793
URL: http://svn.apache.org/r1541793
Log:
On 1.8.x-r1541790 branch: Merge r1541790 from trunk.
Modified:
subversion/branches/1.8.x-r1541790/ (props changed)
subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c
Propchange: subversion/branches/1.8.x-r1541790/
------------------------------------------------------------------------------
Merged /subversion/trunk:r1541790
Modified: subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c?rev=1541793&r1=1541792&r2=1541793&view=diff
==============================================================================
--- subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c (original)
+++ subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c Thu Nov 14 03:08:33 2013
@@ -1101,7 +1101,8 @@ static int dav_svn__handler(request_rec
* that %f in logging formats will show as "svn:/path/to/repo/path/in/repo". */
static int dav_svn__translate_name(request_rec *r)
{
- const char *fs_path, *repos_basename, *repos_path, *slash;
+ const char *fs_path, *repos_path, *slash;
+ const char *repos_basename = NULL;
const char *ignore_cleaned_uri, *ignore_relative_path;
int ignore_had_slash;
dav_error *err;
@@ -1112,20 +1113,39 @@ static int dav_svn__translate_name(reque
return DECLINED;
/* Retrieve path to repo and within repo for the request */
- if ((err = dav_svn_split_uri(r, r->uri, conf->root_dir, &ignore_cleaned_uri,
- &ignore_had_slash, &repos_basename,
- &ignore_relative_path, &repos_path)))
- {
- dav_svn__log_err(r, err, APLOG_ERR);
- return HTTP_INTERNAL_SERVER_ERROR;
- }
+ err = dav_svn_split_uri(r, r->uri, conf->root_dir, &ignore_cleaned_uri,
+ &ignore_had_slash, &repos_basename,
+ &ignore_relative_path, &repos_path);
+
if (conf->fs_parent_path)
{
+ if (err)
+ {
+ if (!repos_basename)
+ {
+ /* detect that there is no repos_basename. We can't error out
+ * here due to this because it would mean that SVNListParentPath
+ * wouldn't work. So set things up to just use the parent path
+ * as our bogus path. */
+ repos_basename = "";
+ repos_path = NULL;
+ }
+ else
+ {
+ dav_svn__log_err(r, err, APLOG_ERR);
+ return err->status;
+ }
+ }
fs_path = svn_dirent_join(conf->fs_parent_path, repos_basename,
r->pool);
}
else
{
+ if (err)
+ {
+ dav_svn__log_err(r, err, APLOG_ERR);
+ return err->status;
+ }
fs_path = conf->fs_path;
}
Re: svn commit: r1541793 - in /subversion/branches/1.8.x-r1541790:
./ subversion/mod_dav_svn/mod_dav_svn.c
Posted by Ben Reser <be...@reser.org>.
On 11/14/13 2:01 AM, Philip Martin wrote:
> Looking at that again, the code doesn't rely on repos_basename being
> valid when there is an error but it is checking for non-NULL. So it's
> still a bit odd that we are checking repos_basename when there is an
> error as it means this code relies on dav_svn_split_uri not setting
> repos_basename on error.
>
> I still think the test of repos_basename should be removed. If we need
> to test something then look for HTTP_FORBIDDEN in err.
Thanks a lot for the review. After talking with stefan2 on IRC I came up with
a much better way to deal with this.
I myself wasn't thrilled with what I had done.
Re: svn commit: r1541793 - in /subversion/branches/1.8.x-r1541790: ./ subversion/mod_dav_svn/mod_dav_svn.c
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
>> + err = dav_svn_split_uri(r, r->uri, conf->root_dir, &ignore_cleaned_uri,
>> + &ignore_had_slash, &repos_basename,
>> + &ignore_relative_path, &repos_path);
>> +
>> if (conf->fs_parent_path)
>> {
>> + if (err)
>> + {
>> + if (!repos_basename)
>
> That looks odd. When dav_svn_split_uri returns an error I suppose it
> could have set repos_basename to a valid value but that is not how we
> normally handle errors. Do we need to make the assumption that
> repos_basename could be valid even when an error is returned? We now
> need to document and ensure that dav_svn_split_uri never returns an
> invalid value here even when it returns an error.
>
> I think it would be better to remove this test, and the initialization
> of repos_basename to NULL, and then always set repos_basename at this
> point.
>
>> + {
>> + /* detect that there is no repos_basename. We can't error out
>> + * here due to this because it would mean that SVNListParentPath
>> + * wouldn't work. So set things up to just use the parent path
>> + * as our bogus path. */
>> + repos_basename = "";
>> + repos_path = NULL;
>> + }
>> + else
>> + {
>> + dav_svn__log_err(r, err, APLOG_ERR);
>> + return err->status;
>> + }
>> + }
Looking at that again, the code doesn't rely on repos_basename being
valid when there is an error but it is checking for non-NULL. So it's
still a bit odd that we are checking repos_basename when there is an
error as it means this code relies on dav_svn_split_uri not setting
repos_basename on error.
I still think the test of repos_basename should be removed. If we need
to test something then look for HTTP_FORBIDDEN in err.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1541793 - in /subversion/branches/1.8.x-r1541790: ./ subversion/mod_dav_svn/mod_dav_svn.c
Posted by Philip Martin <ph...@wandisco.com>.
breser@apache.org writes:
> Author: breser
> Date: Thu Nov 14 03:08:33 2013
> New Revision: 1541793
>
> URL: http://svn.apache.org/r1541793
> Log:
> On 1.8.x-r1541790 branch: Merge r1541790 from trunk.
>
> Modified:
> subversion/branches/1.8.x-r1541790/ (props changed)
> subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c
>
> Propchange: subversion/branches/1.8.x-r1541790/
> ------------------------------------------------------------------------------
> Merged /subversion/trunk:r1541790
>
> Modified: subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c
> URL: http://svn.apache.org/viewvc/subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c?rev=1541793&r1=1541792&r2=1541793&view=diff
> ==============================================================================
> --- subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c (original)
> +++ subversion/branches/1.8.x-r1541790/subversion/mod_dav_svn/mod_dav_svn.c Thu Nov 14 03:08:33 2013
> @@ -1101,7 +1101,8 @@ static int dav_svn__handler(request_rec
> * that %f in logging formats will show as "svn:/path/to/repo/path/in/repo". */
> static int dav_svn__translate_name(request_rec *r)
> {
> - const char *fs_path, *repos_basename, *repos_path, *slash;
> + const char *fs_path, *repos_path, *slash;
> + const char *repos_basename = NULL;
> const char *ignore_cleaned_uri, *ignore_relative_path;
> int ignore_had_slash;
> dav_error *err;
> @@ -1112,20 +1113,39 @@ static int dav_svn__translate_name(reque
> return DECLINED;
>
> /* Retrieve path to repo and within repo for the request */
> - if ((err = dav_svn_split_uri(r, r->uri, conf->root_dir, &ignore_cleaned_uri,
> - &ignore_had_slash, &repos_basename,
> - &ignore_relative_path, &repos_path)))
> - {
> - dav_svn__log_err(r, err, APLOG_ERR);
> - return HTTP_INTERNAL_SERVER_ERROR;
> - }
> + err = dav_svn_split_uri(r, r->uri, conf->root_dir, &ignore_cleaned_uri,
> + &ignore_had_slash, &repos_basename,
> + &ignore_relative_path, &repos_path);
> +
> if (conf->fs_parent_path)
> {
> + if (err)
> + {
> + if (!repos_basename)
That looks odd. When dav_svn_split_uri returns an error I suppose it
could have set repos_basename to a valid value but that is not how we
normally handle errors. Do we need to make the assumption that
repos_basename could be valid even when an error is returned? We now
need to document and ensure that dav_svn_split_uri never returns an
invalid value here even when it returns an error.
I think it would be better to remove this test, and the initialization
of repos_basename to NULL, and then always set repos_basename at this
point.
> + {
> + /* detect that there is no repos_basename. We can't error out
> + * here due to this because it would mean that SVNListParentPath
> + * wouldn't work. So set things up to just use the parent path
> + * as our bogus path. */
> + repos_basename = "";
> + repos_path = NULL;
> + }
> + else
> + {
> + dav_svn__log_err(r, err, APLOG_ERR);
> + return err->status;
> + }
> + }
> fs_path = svn_dirent_join(conf->fs_parent_path, repos_basename,
> r->pool);
> }
> else
> {
> + if (err)
> + {
> + dav_svn__log_err(r, err, APLOG_ERR);
> + return err->status;
> + }
> fs_path = conf->fs_path;
> }
>
>
>
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*