You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by cm...@apache.org on 2012/10/30 01:23:39 UTC
svn commit: r1403588 - in /subversion/trunk/subversion:
mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
Author: cmpilato
Date: Tue Oct 30 00:23:38 2012
New Revision: 1403588
URL: http://svn.apache.org/viewvc?rev=1403588&view=rev
Log:
Tweak the server-side logic which validates update report source and
target revision values so that they always get checked for validity,
not only when doing non-client-pegged updates.
* subversion/mod_dav_svn/reports/update.c
(validate_input_revision): New helper function.
(dav_svn__update_report): Always query the youngest FS revision, and
use it (via validate_input_revision()) to raise errors when the
client requests an update to a revision younger than is available
or reports that it has objects whose revision is greater than the
youngest.
* subversion/tests/cmdline/update_tests.py
(update_to_HEAD_plus_1): Look for an error code, not a particular
string.
Modified:
subversion/trunk/subversion/mod_dav_svn/reports/update.c
subversion/trunk/subversion/tests/cmdline/update_tests.py
Modified: subversion/trunk/subversion/mod_dav_svn/reports/update.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/reports/update.c?rev=1403588&r1=1403587&r2=1403588&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/reports/update.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/reports/update.c Tue Oct 30 00:23:38 2012
@@ -922,6 +922,49 @@ malformed_element_error(const char *tagn
}
+/* Validate that REVISION is a valid revision number for repository in
+ which YOUNGEST is the latest revision. Use RESOURCE as a
+ convenient way to access the request record and a pool for error
+ messaging. (It's okay if REVISION is SVN_INVALID_REVNUM, as in
+ the related contexts that just means "the youngest revision".)
+
+ REVTYPE is just a string describing the type/purpose of REVISION,
+ used in the generated error string. */
+static dav_error *
+validate_input_revision(svn_revnum_t revision,
+ svn_revnum_t youngest,
+ const char *revtype,
+ const dav_resource *resource)
+{
+ if (! SVN_IS_VALID_REVNUM(revision))
+ return SVN_NO_ERROR;
+
+ if (revision > youngest)
+ {
+ svn_error_t *serr;
+
+ if (dav_svn__get_master_uri(resource->info->r))
+ {
+ serr = svn_error_createf(SVN_ERR_FS_NO_SUCH_REVISION, 0,
+ "No such %s '%ld' found in the repository. "
+ "Perhaps the repository is out of date with "
+ "respect to the master repository?",
+ revtype, revision);
+ }
+ else
+ {
+ serr = svn_error_createf(SVN_ERR_FS_NO_SUCH_REVISION, 0,
+ "No such %s '%ld' found in the repository.",
+ revtype, revision);
+ }
+ return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
+ "Invalid revision found in update report "
+ "request.", resource->pool);
+ }
+ return SVN_NO_ERROR;
+}
+
+
dav_error *
dav_svn__update_report(const dav_resource *resource,
const apr_xml_doc *doc,
@@ -931,8 +974,7 @@ dav_svn__update_report(const dav_resourc
apr_xml_elem *child;
void *rbaton = NULL;
update_ctx_t uc = { 0 };
- svn_revnum_t revnum = SVN_INVALID_REVNUM;
- svn_boolean_t revnum_is_head = FALSE;
+ svn_revnum_t youngest, revnum = SVN_INVALID_REVNUM;
svn_revnum_t from_revnum = SVN_INVALID_REVNUM;
int ns;
/* entry_counter and entry_is_empty are for operational logging. */
@@ -998,6 +1040,14 @@ dav_svn__update_report(const dav_resourc
}
}
+ /* Ask the repository about its youngest revision (which we'll need
+ for some input validation later). */
+ if ((serr = svn_fs_youngest_rev(&youngest, repos->fs, resource->pool)))
+ return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
+ "Could not determine the youngest "
+ "revision for the update process.",
+ resource->pool);
+
for (child = doc->root->first_child; child != NULL; child = child->next)
{
/* Note that child->name might not match any of the cases below.
@@ -1125,6 +1175,23 @@ dav_svn__update_report(const dav_resourc
}
}
+ /* If a target revision wasn't requested, or the requested target
+ revision was invalid, just update to HEAD as of the moment we
+ queried the youngest revision. Otherwise, at least make sure the
+ request makes sense in light of that youngest revision
+ number. */
+ if (! SVN_IS_VALID_REVNUM(revnum))
+ {
+ revnum = youngest;
+ }
+ else
+ {
+ derr = validate_input_revision(revnum, youngest, "target revision",
+ resource);
+ if (derr)
+ return derr;
+ }
+
if (!saw_depth && !saw_recursive && (requested_depth == svn_depth_unknown))
requested_depth = svn_depth_infinity;
@@ -1140,18 +1207,6 @@ dav_svn__update_report(const dav_resourc
SVN_DAV_ERROR_TAG);
}
- /* If a revision for this operation was not dictated to us, this
- means "update to whatever the current HEAD is now". */
- if (revnum == SVN_INVALID_REVNUM)
- {
- if ((serr = svn_fs_youngest_rev(&revnum, repos->fs, resource->pool)))
- return dav_svn__convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
- "Could not determine the youngest "
- "revision for the update process.",
- resource->pool);
- revnum_is_head = TRUE;
- }
-
uc.svndiff_version = resource->info->svndiff_version;
uc.compression_level = dav_svn__get_compression_level(resource->info->r);
uc.resource = resource;
@@ -1270,27 +1325,10 @@ dav_svn__update_report(const dav_resourc
{
rev = SVN_STR_TO_REV(this_attr->value);
saw_rev = TRUE;
- if (revnum_is_head && rev > revnum)
- {
- if (dav_svn__get_master_uri(resource->info->r))
- return dav_svn__new_error_tag(
- resource->pool,
- HTTP_INTERNAL_SERVER_ERROR, 0,
- "A reported revision is higher than the "
- "current repository HEAD revision. "
- "Perhaps the repository is out of date "
- "with respect to the master repository?",
- SVN_DAV_ERROR_NAMESPACE,
- SVN_DAV_ERROR_TAG);
- else
- return dav_svn__new_error_tag(
- resource->pool,
- HTTP_INTERNAL_SERVER_ERROR, 0,
- "A reported revision is higher than the "
- "current repository HEAD revision.",
- SVN_DAV_ERROR_NAMESPACE,
- SVN_DAV_ERROR_TAG);
- }
+ if ((derr = validate_input_revision(rev, youngest,
+ "reported revision",
+ resource)))
+ return derr;
}
else if (strcmp(this_attr->name, "depth") == 0)
depth = svn_depth_from_word(this_attr->value);
Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/update_tests.py?rev=1403588&r1=1403587&r2=1403588&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/update_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/update_tests.py Tue Oct 30 00:23:38 2012
@@ -5516,7 +5516,7 @@ def update_to_HEAD_plus_1(sbox):
svntest.actions.run_and_verify_update(wc_dir,
None, None, None,
- ".*No such revision",
+ ".*E160006",
None, None,
None, None, None, wc_dir, '-r', '2')
Re: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c
tests/cmdline/update_tests.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 10/30/2012 08:58 AM, Bert Huijben wrote:
>>> How hard would it be to port this one back to 1.7?
>>
>> I don't think it would be hard at all. I certainly plan to make the attempt
>> today.
>
> One of the reasons I was suggesting is that I was thinking that this code
> might also relate to the errors reported by temporarily out of sync
> proxies, but I now think those originate in the reporter handling.
My changes to do affect temporarily out-of-sync proxies.
> Yes great.
> I think you could use "E160006.*No such.*revision".
> (If I remember correctly we don't need a .* at the start.)
Done. (And proposed alongside r1403588 for backport.)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: C. Michael Pilato [mailto:cmpilato@collab.net]
> Sent: dinsdag 30 oktober 2012 13:01
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1403588 - in /subversion/trunk/subversion:
> mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
>
> On 10/30/2012 06:46 AM, Bert Huijben wrote:
> >
> >
> >> -----Original Message-----
> >> From: cmpilato@apache.org [mailto:cmpilato@apache.org]
> >> Sent: dinsdag 30 oktober 2012 01:24
> >> To: commits@subversion.apache.org
> >> Subject: svn commit: r1403588 - in /subversion/trunk/subversion:
> >> mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
> >>
> >> Author: cmpilato
> >> Date: Tue Oct 30 00:23:38 2012
> >> New Revision: 1403588
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1403588&view=rev
> >> Log:
> >> Tweak the server-side logic which validates update report source and
> >> target revision values so that they always get checked for validity,
> >> not only when doing non-client-pegged updates.
> >>
> >> * subversion/mod_dav_svn/reports/update.c
> >> (validate_input_revision): New helper function.
> >> (dav_svn__update_report): Always query the youngest FS revision, and
> >> use it (via validate_input_revision()) to raise errors when the
> >> client requests an update to a revision younger than is available
> >> or reports that it has objects whose revision is greater than the
> >> youngest.
> >
> > How hard would it be to port this one back to 1.7?
>
> I don't think it would be hard at all. I certainly plan to make the attempt
> today.
One of the reasons I was suggesting is that I was thinking that this code might also relate to the errors reported by temporarily out of sync proxies, but I now think those originate in the reporter handling.
>
> >> Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py
> >> URL:
> >>
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/
> >> update_tests.py?rev=1403588&r1=1403587&r2=1403588&view=diff
> >>
> ==========================================================
> >> ====================
> >> --- subversion/trunk/subversion/tests/cmdline/update_tests.py (original)
> >> +++ subversion/trunk/subversion/tests/cmdline/update_tests.py Tue
> Oct 30
> >> 00:23:38 2012
> >> @@ -5516,7 +5516,7 @@ def update_to_HEAD_plus_1(sbox):
> >>
> >> svntest.actions.run_and_verify_update(wc_dir,
> >> None, None, None,
> >> - ".*No such revision",
> >> + ".*E160006",
> >
> > If possible, please also keep some part(s) of an error message in the regex,
> to make the testsuite verify that we keep failing for the same reason.
> > (In this case it checks for SVN_ERR_FS_NO_SUCH_REVISION, which already
> tells almost the whole story, but I don't see that in the test suite code)
>
> Good suggestion. The "No such revision ..." is now (for DAV servers) "No
> such [target|reported] revision ...". Would ".*No such.*revision" serve
> your desires here?
Yes great.
I think you could use "E160006.*No such.*revision".
(If I remember correctly we don't need a .* at the start.)
Bert
Re: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c
tests/cmdline/update_tests.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 10/30/2012 06:46 AM, Bert Huijben wrote:
>
>
>> -----Original Message-----
>> From: cmpilato@apache.org [mailto:cmpilato@apache.org]
>> Sent: dinsdag 30 oktober 2012 01:24
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1403588 - in /subversion/trunk/subversion:
>> mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
>>
>> Author: cmpilato
>> Date: Tue Oct 30 00:23:38 2012
>> New Revision: 1403588
>>
>> URL: http://svn.apache.org/viewvc?rev=1403588&view=rev
>> Log:
>> Tweak the server-side logic which validates update report source and
>> target revision values so that they always get checked for validity,
>> not only when doing non-client-pegged updates.
>>
>> * subversion/mod_dav_svn/reports/update.c
>> (validate_input_revision): New helper function.
>> (dav_svn__update_report): Always query the youngest FS revision, and
>> use it (via validate_input_revision()) to raise errors when the
>> client requests an update to a revision younger than is available
>> or reports that it has objects whose revision is greater than the
>> youngest.
>
> How hard would it be to port this one back to 1.7?
I don't think it would be hard at all. I certainly plan to make the attempt
today.
>> Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/
>> update_tests.py?rev=1403588&r1=1403587&r2=1403588&view=diff
>> ==========================================================
>> ====================
>> --- subversion/trunk/subversion/tests/cmdline/update_tests.py (original)
>> +++ subversion/trunk/subversion/tests/cmdline/update_tests.py Tue Oct 30
>> 00:23:38 2012
>> @@ -5516,7 +5516,7 @@ def update_to_HEAD_plus_1(sbox):
>>
>> svntest.actions.run_and_verify_update(wc_dir,
>> None, None, None,
>> - ".*No such revision",
>> + ".*E160006",
>
> If possible, please also keep some part(s) of an error message in the regex, to make the testsuite verify that we keep failing for the same reason.
> (In this case it checks for SVN_ERR_FS_NO_SUCH_REVISION, which already tells almost the whole story, but I don't see that in the test suite code)
Good suggestion. The "No such revision ..." is now (for DAV servers) "No
such [target|reported] revision ...". Would ".*No such.*revision" serve
your desires here?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: svn commit: r1403588 - in /subversion/trunk/subversion: mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: cmpilato@apache.org [mailto:cmpilato@apache.org]
> Sent: dinsdag 30 oktober 2012 01:24
> To: commits@subversion.apache.org
> Subject: svn commit: r1403588 - in /subversion/trunk/subversion:
> mod_dav_svn/reports/update.c tests/cmdline/update_tests.py
>
> Author: cmpilato
> Date: Tue Oct 30 00:23:38 2012
> New Revision: 1403588
>
> URL: http://svn.apache.org/viewvc?rev=1403588&view=rev
> Log:
> Tweak the server-side logic which validates update report source and
> target revision values so that they always get checked for validity,
> not only when doing non-client-pegged updates.
>
> * subversion/mod_dav_svn/reports/update.c
> (validate_input_revision): New helper function.
> (dav_svn__update_report): Always query the youngest FS revision, and
> use it (via validate_input_revision()) to raise errors when the
> client requests an update to a revision younger than is available
> or reports that it has objects whose revision is greater than the
> youngest.
How hard would it be to port this one back to 1.7?
<snip>
> Modified: subversion/trunk/subversion/tests/cmdline/update_tests.py
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/
> update_tests.py?rev=1403588&r1=1403587&r2=1403588&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/tests/cmdline/update_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/update_tests.py Tue Oct 30
> 00:23:38 2012
> @@ -5516,7 +5516,7 @@ def update_to_HEAD_plus_1(sbox):
>
> svntest.actions.run_and_verify_update(wc_dir,
> None, None, None,
> - ".*No such revision",
> + ".*E160006",
If possible, please also keep some part(s) of an error message in the regex, to make the testsuite verify that we keep failing for the same reason.
(In this case it checks for SVN_ERR_FS_NO_SUCH_REVISION, which already tells almost the whole story, but I don't see that in the test suite code)
Bert
> None, None,
> None, None, None, wc_dir, '-r', '2')
>
>