Philip Martin <philip.mar...@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*
