RE: Performance branch review: single revision changes
-Original Message- From: Stefan Fuhrmann [mailto:stefanfuhrm...@alice-dsl.de] Sent: zondag 19 september 2010 21:59 To: Subversion Development Subject: Performance branch review: single revision changes Hi there, as per public request, this is a list of smaller changes made on the performance branch. Each revision can be reviewed (hopefully) without further context and be merged into trunk independently. * r982335 Limit the amount of unused memory kept in apr_pools to the same amount as everywhere else, e.g. SVN.exe main(). Justification: (1) uniform handling of memory pools (2) Without this change, apr pools will fragment indefinitely (?) for strings (e.g. fulltexts) and other chunks larger than 80kB. This already happens on trunk if memcached usage has been enabled. (not related to the content, specific patches) This mail looks like how we handle branches/1.Y.x/STATUS Maybe you should check it in to your branch to allow further updates (and review) there :) Bert
Re: Performance branch review: single revision changes
On Mon, Sep 20, 2010 at 7:47 AM, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Stefan Fuhrmann [mailto:stefanfuhrm...@alice-dsl.de] Sent: zondag 19 september 2010 21:59 To: Subversion Development Subject: Performance branch review: single revision changes Hi there, as per public request, this is a list of smaller changes made on the performance branch. Each revision can be reviewed (hopefully) without further context and be merged into trunk independently. * r982335 Limit the amount of unused memory kept in apr_pools to the same amount as everywhere else, e.g. SVN.exe main(). Justification: (1) uniform handling of memory pools (2) Without this change, apr pools will fragment indefinitely (?) for strings (e.g. fulltexts) and other chunks larger than 80kB. This already happens on trunk if memcached usage has been enabled. (not related to the content, specific patches) This mail looks like how we handle branches/1.Y.x/STATUS Maybe you should check it in to your branch to allow further updates (and review) there :) A good strategy. And thanks for sending this, Stefan, it will help this work get onto trunk quicker. -Hyrum
Re: svn commit: r998502 - /subversion/trunk/subversion/svnrdump/load_editor.c
Ramkumar Ramachandra wrote on Mon, Sep 20, 2010 at 10:35:34 +0530:
Hi Daniel,
Daniel Shahaf writes:
Ramkumar Ramachandra wrote on Sun, Sep 19, 2010 at 16:52:50 +0530:
Daniel Shahaf writes:
Stefan Sperling wrote on Sun, Sep 19, 2010 at 11:40:49 +0200:
On Sun, Sep 19, 2010 at 10:29:58AM +0100, Daniel Shahaf wrote:
2. If you do duplicate code, then add big comments (in all
instances of
the duplication) pointing to the other instances.
+1
I've mentioned it in the commit message, but alright- I'll add a
comment in the file.
Please add a comment to svnsync's get_lock() as well.
+1 needed :)
+1 given, but please s/##/###/ for consistency :)
[[[
* subversion/svnsync/main.c
(get_lock): Add a comment explaining what the function does along
with a note about it being duplicated in svnrdump.
* subversion/svnrdump/load_editor.c
(get_lock): Add a comment explaining what the function does along
with a note about it being duplicated in svnsync.
]]]
Index: subversion/svnsync/main.c
===
--- subversion/svnsync/main.c (revision 998652)
+++ subversion/svnsync/main.c (working copy)
@@ -285,7 +285,13 @@ check_lib_versions(void)
/* Acquire a lock (of sorts) on the repository associated with the
- * given RA SESSION.
+ * given RA SESSION. This lock is just a revprop change attempt in a
+ * time-delay loop. This function is duplicated by svnrdump in
+ * load_editor.c.
+ *
+ * ## TODO: Make this function more generic and
+ * expose it through a header for use by other Subversion
+ * applications to avoid duplication.
*/
static svn_error_t *
get_lock(svn_ra_session_t *session, apr_pool_t *pool)
Index: subversion/svnrdump/load_editor.c
===
--- subversion/svnrdump/load_editor.c (revision 998772)
+++ subversion/svnrdump/load_editor.c (working copy)
@@ -56,6 +56,14 @@ commit_callback(const svn_commit_info_t *commit_in
return SVN_NO_ERROR;
}
+/* Acquire a lock (of sorts) on the repository associated with the
+ * given RA SESSION. This lock is just a revprop change attempt in a
+ * time-delay loop. This function is duplicated by svnsync in main.c.
+ *
+ * ## TODO: Make this function more generic and
+ * expose it through a header for use by other Subversion
+ * applications to avoid duplication.
+ */
static svn_error_t *
get_lock(svn_ra_session_t *session, apr_pool_t *pool)
{
Thanks for the offer. I can do that myself, though; svnrdump is
a first-class citizen now, as is this duplication (per the direction
this thread seems to be going), so AFAICT all cards say the branch
maintainer should be adding that to svnrdump.
Sure :)
-- Ram
Performance branch: Membuffer cache review
Hi devs, To aid the performance branch review process, I've extracted the change that probably gives the largest performance gain. The changes were grouped in a way that eliminated all possibly confusing temporary code. So, without any further ado, please review: /branches/integrate-cache-membuffer (revisions r998649, r998843 and 998852) Thanks! Stefan^2.
[PATCH 1/3] atomic-revprop: Change error name
Hi,
I'm keen to get the atomic-revprop feature working, so I've had a
go at writing patches for some of the missing bits.
This first patch gives a unique error code for the case where the
old_value passed to the revprop change function doesn't match the
real old value. This allows it to be detected easily. (The
branch currently overloads SVN_ERR_BAD_PROPERTY_VALUE for this,
but the same error code is also used if the new value is illegal).
This feature is on the BRANCH-README's TODO list.
The patch is against the atomic-revprop branch, but it also
applies to trunk.
[[[
Use a new, distinct error code if svn_fs_change_rev_prop2's old_value_p
doesn't match.
* subversion/include/svn_error_codes.h:
(SVN_ERR_BAD_OLD_VALUE): New error code.
* subversion/libsvn_fs_fs/fs_fs.c:
(change_rev_prop_body): Use SVN_ERR_BAD_OLD_VALUE when appropriate.
* subversion/libsvn_fs_base/revs-txns.c:
(svn_fs_base__set_rev_prop): Use SVN_ERR_BAD_OLD_VALUE when
appropriate.
* subversion/include/svn_fs.h:
(svn_fs_change_rev_prop2): Update documentation.
* subversion/tests/libsvn_fs/fs-test.c:
(FAILS_WITH_BPV): Rename to...
(FAILS_WITH_BOV): ... this. Change to test for SVN_ERR_BAD_OLD_VALUE.
(revision_props): Change to use FAILS_WITH_BOV instead of
FAILS_WITH_BPV.
Patch by: Jon Foster jon.fos...@cabot.co.uk
]]]
Kind regards,
Jon
**
This email and its attachments may be confidential and are intended solely for
the use of the individual to whom it is addressed. Any views or opinions
expressed are solely those of the author and do not necessarily represent those
of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you
must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**
__
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
__Index: subversion/include/svn_error_codes.h
===
--- subversion/include/svn_error_codes.h(revision 998620)
+++ subversion/include/svn_error_codes.h(working copy)
@@ -219,6 +219,11 @@
SVN_ERR_BAD_CATEGORY_START + 13,
Unknown string value of token)
+ /** @since New in 1.7. */
+ SVN_ERRDEF(SVN_ERR_BAD_OLD_VALUE,
+ SVN_ERR_BAD_CATEGORY_START + 14,
+ Old value doesn't match repository)
+
/* xml errors */
SVN_ERRDEF(SVN_ERR_XML_ATTRIB_NOT_FOUND,
Index: subversion/libsvn_fs_fs/fs_fs.c
===
--- subversion/libsvn_fs_fs/fs_fs.c (revision 998620)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -7312,7 +7312,7 @@
!svn_string_compare(wanted_value, present_value)))
{
/* What we expected isn't what we found. */
- return svn_error_createf(SVN_ERR_BAD_PROPERTY_VALUE, NULL,
+ return svn_error_createf(SVN_ERR_BAD_OLD_VALUE, NULL,
_(revprop '%s' has unexpected value in
filesystem),
cb-name);
Index: subversion/libsvn_fs_base/revs-txns.c
===
--- subversion/libsvn_fs_base/revs-txns.c (revision 998620)
+++ subversion/libsvn_fs_base/revs-txns.c (working copy)
@@ -270,7 +270,7 @@
!svn_string_compare(wanted_value, present_value)))
{
/* What we expected isn't what we found. */
- return svn_error_createf(SVN_ERR_BAD_PROPERTY_VALUE, NULL,
+ return svn_error_createf(SVN_ERR_BAD_OLD_VALUE, NULL,
_(revprop '%s' has unexpected value in
filesystem),
name);
Index: subversion/include/svn_fs.h
===
--- subversion/include/svn_fs.h (revision 998620)
+++ subversion/include/svn_fs.h (working copy)
@@ -1896,8 +1896,8 @@
* - @a name is the name of the property to change.
* - if @a old_value_p is not @c NULL, then @a *old_value_p is the expected old
* value of the property, and changing the value will fail with error
- * #SVN_ERR_BAD_PROPERTY_VALUE if the present value of the property is not @a
- * *old_value_p.
+ * #SVN_ERR_BAD_OLD_VALUE if the present value of the property is not @a
Re: svn diff optimization to make blame faster?
On 15.09.2010 14:20, Johan Corveleyn wrote: Some update on this: I have implemented this for svn_diff (excluding the identical prefix and suffix of both files, and only then starting to fill up the token tree and let the lcs-agorithm to its thing). It makes a *huge* difference. On my bigfile.xml (1.5 Mb) with only one line changed, the call to svn_diff_diff is ~10 times faster (15-20 ms vs. 150-170 ms). Hmmm ... looks to me like test data tailored to the optimization. :) -- Brane
Re: Performance branch review: single revision changes
Hyrum K. Wright wrote: On Mon, Sep 20, 2010 at 7:47 AM, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Stefan Fuhrmann [mailto:stefanfuhrm...@alice-dsl.de] Sent: zondag 19 september 2010 21:59 To: Subversion Development Subject: Performance branch review: single revision changes Hi there, as per public request, this is a list of smaller changes made on the performance branch. Each revision can be reviewed (hopefully) without further context and be merged into trunk independently. * r982335 Limit the amount of unused memory kept in apr_pools to the same amount as everywhere else, e.g. SVN.exe main(). Justification: (1) uniform handling of memory pools (2) Without this change, apr pools will fragment indefinitely (?) for strings (e.g. fulltexts) and other chunks larger than 80kB. This already happens on trunk if memcached usage has been enabled. (not related to the content, specific patches) This mail looks like how we handle branches/1.Y.x/STATUS Maybe you should check it in to your branch to allow further updates (and review) there :) A good strategy. And thanks for sending this, Stefan, it will help this work get onto trunk quicker. -Hyrum Done in r998858. I was hesitant to do it in the first place because the review / merge direction is reversed in comparison to the 1.y.x branches. -- Stefan^2.
Re: svn commit: r995475 - /subversion/trunk/subversion/libsvn_repos/load.c
On 15.09.2010 21:03, Stefan Sperling wrote: On Fri, Sep 10, 2010 at 04:02:52PM +0200, Branko Čibej wrote: On 10.09.2010 15:26, Stefan Sperling wrote: I know we're using C89, but maybe it's time to move on and upgrade to C99 where the benefits are desirable? When Subversion was started, C89 was about a decade old, and C99 is just as old now... Microsoft's C compiler, to name only one, still does not provide most of the C99 features. I don't know about standard library support. However, I don't see where you gain with using strtol(). First of all, it was already in C89 and has effectively the same interface as the APR conversions. C99 added strtoll() but it has the same interface. What's the benefit? Raising this thread again, because there is a benefit to using strtol()'s unsigned cousin, strtoul(). APR does not provide an interface for it. Can we use it? Can we also use its 64-bit cousin strtoull()? If we can use those two, we might as well use strtol() and strtoul() and skip apr_strtoi64() completely. You'd still have to provide an alternate implementation for all the environments that do not provide this function. I think there may be one with a different spelling in Microsoft's C runtime, but haven't looked in a while ... -- Brane
[PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Hi, For atomic-revprop, the client needs to know if the error was SVN_ERR_BAD_OLD_VALUE or not. For ra_local and ra_svn this is already done; this patch does it for DAV (with Neon). With mod_dav, we only have 2 ways to affect the 207 multistatus response from a failed PROPPATCH: - We can set the text that appears in D:responsedescription, including injecting arbitrary XML. - We can set the D:status value to a (numeric) HTTP error code. The approach that's been discussed on-list and on IRC was to inject the error as XML inside D:responsedescription. I did actually implement that, only to discover that Neon completely rejects the XML in that case[1]. While we would obviously fix this in the 1.7 client code, it could break compatibility between the 1.7 server and 1.6 and older clients. The only way to handle this would be to introduce a new client capability, so the server could fall back to the old code for 1.6 clients. This means we'd have two code paths for error-handling... this gets very complex. However, there is a simpler way! The D:status element contains the HTTP error code, usually 500 Internal Server Error. So we could pick a special HTTP status code to mean SVN_ERR_BAD_OLD_VALUE. I think of old_value_p as a precondition for the operation, a bit like If-Modified-Since:, so I'd suggest 412 Precondition Failed. Note that generic DAV clients won't ever see this message, because they won't be sending old_value_p. This patch only does mod_dav_svn and Neon. I don't want to waste time doing both HTTP libraries if this patch is rejected. (Also, Serf is slightly harder because it doesn't parse D:status yet). The patch is against the atomic-revprop branch, and requires Patch 1 to be applied first. (It does apply to trunk, too). [[[ Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status code inside a 207 multi-status response. * subversion/mod_dav_svn/util.c: (dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412. * subversion/libsvn_ra_neon/util.c: (multistatus_baton_t): New member contains_precondition_error. (end_207_element): Check for HTTP 412 status code and create a SVN_ERR_BAD_OLD_VALUE error if found. Patch by: Jon Foster jon.fos...@cabot.co.uk ]]] Kind regards, Jon [1] More precisely, libsvn_ra_neon/util.c:wrapper_startelm_cb() raises a SVN_ERR_XML_MALFORMED XML data was not well-formed error if D:responsedescription contains any XML tags. See the ELEM_responsedescription row in multistatus_nesting_table[] in the same file - it explicitly states that if there's any XML tags inside D:responsedescription then the XML is invalid. ** This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd. If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone. Cabot Communications Limited Verona House, Filwood Road, Bristol BS16 3RY, UK +44 (0) 1179584232 Co. Registered in England number 02817269 Please contact the sender if you believe you have received this email in error. ** __ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email __Index: subversion/mod_dav_svn/util.c === --- subversion/mod_dav_svn/util.c (revision 998620) +++ subversion/mod_dav_svn/util.c (working copy) @@ -107,6 +107,9 @@ case SVN_ERR_FS_PATH_ALREADY_LOCKED: status = HTTP_LOCKED; break; + case SVN_ERR_BAD_OLD_VALUE: +status = HTTP_PRECONDITION_FAILED; +break; /* add other mappings here */ } Index: subversion/libsvn_ra_neon/util.c === --- subversion/libsvn_ra_neon/util.c(revision 998620) +++ subversion/libsvn_ra_neon/util.c(working copy) @@ -167,6 +167,7 @@ svn_ra_neon__request_t *req; svn_stringbuf_t *description; svn_boolean_t contains_error; + svn_boolean_t contains_precondition_error; } multistatus_baton_t; /* Implements svn_ra_neon__startelm_cb_t. */ @@ -231,9 +232,12 @@ return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL, _(The request response contained at least one error)); + else if (b-contains_precondition_error) +return
Re: Performance branch review: single revision changes
Stefan Fuhrmann wrote on Mon, Sep 20, 2010 at 11:52:37 +0200: Hyrum K. Wright wrote: On Mon, Sep 20, 2010 at 7:47 AM, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Stefan Fuhrmann [mailto:stefanfuhrm...@alice-dsl.de] * r982335 Limit the amount of unused memory kept in apr_pools This mail looks like how we handle branches/1.Y.x/STATUS Maybe you should check it in to your branch to allow further updates (and review) there :) A good strategy. And thanks for sending this, Stefan, it will help this work get onto trunk quicker. *nod* Done in r998858. I was hesitant to do it in the first place because the review / merge direction is reversed in comparison to the 1.y.x branches. Indeed, and if we were to prefer the proposed merges are tracked at the target tree system then we could keep a STATUS file on trunk that lists the pending merges from all feature branches, instead of keeping that information on each individual branch.
State of Python Bindings for Windows
Hello,
I can't find any binaries for Python bindings for Windows. Previously
they were available from tigris.org site, but now there aren't any
updates. It will greatly reduce frustration for users of hgsubversion
and other Windows tools if up to date official bindings could be
available from PyPI.
Why Apache Foundation can't provide the binaries like it does for
Apache, for instance?
1.6 release notes contained information about new ctypes bindings
http://subversion.apache.org/docs/release-notes/1.6.html#ctypes-python-bindings
But where are they? How to use them or install them? Are they stable?
Do any projects use them?
I couldn't find any official information on subversion.apache.org
If there are no people interested in Subversion bindings development
in ASF team, I'd advise to move development to the old
http://code.google.com/p/csvn/ project and somehow sync with ASF tree
('somehow' here means - 'take a look at this use case') - this way
people at least will be able to send patches, monitor progress and
comment on arising issues, as well as explain what do they want.
Please, CC.
Thanks.
--
anatoly t.
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
-Original Message-
From: Jon Foster [mailto:jon.fos...@cabot.co.uk]
Sent: maandag 20 september 2010 12:01
To: Daniel Shahaf; Subversion Development
Subject: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status
code
Hi,
For atomic-revprop, the client needs to know if the error was
SVN_ERR_BAD_OLD_VALUE or not. For ra_local and ra_svn this is
already done; this patch does it for DAV (with Neon).
With mod_dav, we only have 2 ways to affect the 207 multistatus
response from a failed PROPPATCH:
- We can set the text that appears in D:responsedescription,
including injecting arbitrary XML.
- We can set the D:status value to a (numeric) HTTP error code.
The approach that's been discussed on-list and on IRC was to
inject the error as XML inside D:responsedescription. I did
actually implement that, only to discover that Neon completely
rejects the XML in that case[1]. While we would obviously fix
this in the 1.7 client code, it could break compatibility between
the 1.7 server and 1.6 and older clients. The only way to handle
this would be to introduce a new client capability, so the server
could fall back to the old code for 1.6 clients. This means we'd
have two code paths for error-handling... this gets very complex.
However, there is a simpler way! The D:status element contains
the HTTP error code, usually 500 Internal Server Error. So we
could pick a special HTTP status code to mean SVN_ERR_BAD_OLD_VALUE.
I think of old_value_p as a precondition for the operation, a bit
like If-Modified-Since:, so I'd suggest 412 Precondition Failed.
Note that generic DAV clients won't ever see this message, because
they won't be sending old_value_p.
Nice and simple solution to this issue :)
I think 'SVN_ERR_BAD_OLD_VALUE' could use a better/more generic name though.
('Old value' is only valid in the specific function context. Maybe extend
this to something more like this 'precondition failed')
Bert
Re: [PATCH 1/3] atomic-revprop: Change error name
Jon Foster wrote on Mon, Sep 20, 2010 at 10:47:05 +0100: Hi, I'm keen to get the atomic-revprop feature working, so I've had a go at writing patches for some of the missing bits. Hi, and thanks. :-) This first patch gives a unique error code for the case where the old_value passed to the revprop change function doesn't match the real old value. This allows it to be detected easily. (The branch currently overloads SVN_ERR_BAD_PROPERTY_VALUE for this, but the same error code is also used if the new value is illegal). This feature is on the BRANCH-README's TODO list. Yep. I didn't spend time on it because it's a fairly low-hanging fruit and I preferred to do the heavier lifting (ra_dav work) first. The patch is against the atomic-revprop branch, but it also applies to trunk. [[[ Use a new, distinct error code if svn_fs_change_rev_prop2's old_value_p doesn't match. * subversion/include/svn_error_codes.h: (SVN_ERR_BAD_OLD_VALUE): New error code. * subversion/libsvn_fs_fs/fs_fs.c: (change_rev_prop_body): Use SVN_ERR_BAD_OLD_VALUE when appropriate. * subversion/libsvn_fs_base/revs-txns.c: (svn_fs_base__set_rev_prop): Use SVN_ERR_BAD_OLD_VALUE when appropriate. * subversion/include/svn_fs.h: (svn_fs_change_rev_prop2): Update documentation. * subversion/tests/libsvn_fs/fs-test.c: (FAILS_WITH_BPV): Rename to... (FAILS_WITH_BOV): ... this. Change to test for SVN_ERR_BAD_OLD_VALUE. (revision_props): Change to use FAILS_WITH_BOV instead of FAILS_WITH_BPV. Patch by: Jon Foster jon.fos...@cabot.co.uk ]]] Kind regards, Jon ** This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd. If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone. Cabot Communications Limited Verona House, Filwood Road, Bristol BS16 3RY, UK +44 (0) 1179584232 Co. Registered in England number 02817269 Please contact the sender if you believe you have received this email in error. ** __ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email __ +++ subversion/include/svn_error_codes.h (working copy) @@ -219,6 +219,11 @@ + /** @since New in 1.7. */ + SVN_ERRDEF(SVN_ERR_BAD_OLD_VALUE, + SVN_ERR_BAD_CATEGORY_START + 14, + Old value doesn't match repository) + Hmm. Not sure if there's a text change to be made here, but I'm not going to bikeshed about an error text that will never be shown to users :-) Index: subversion/libsvn_fs_fs/fs_fs.c Index: subversion/libsvn_fs_base/revs-txns.c Index: subversion/include/svn_fs.h Index: subversion/tests/libsvn_fs/fs-test.c Applied to trunk, unmodified, in r998880. (See commits@ for some other revisions today.) Thanks! Daniel
Re: [PATCH 2/3] atomic-revprop: Change tests
Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100:
Hi,
This patch changes the tests for the atomic-revprop feature, so that
they test the error number rather than parsing the error text.
This doesn't work with Serf or Neon yet - they're still TODO. This
gives you a way to test Serf Neon patches. (You might like to
hold off on committing this until Serf and Neon are done).
The patch is against the atomic-revprop branch, and requires
Patch 1 to be applied first. It doesn't apply to trunk (I don't
think the tests have been merged to trunk?)
On trunk there are only the libsvn_fs and libsvn_repos parts. The
libsvn_ra parts, and the associated Python tests, are only on the
branch.
[[[
Change atomic-revprop tests to look at the error number rather
than parsing the error text.
* subversion/tests/cmdline/atomic-ra-revprop-change.c:
(main): When printing an error, check if it's SVN_ERR_BAD_OLD_VALUE
and print a special message if it is.
* subversion/tests/cmdline/prop_tests.py:
(FAILS_WITH_BPV): Look for the special message.
Patch by: Jon Foster jon.fos...@cabot.co.uk
]]]
So, this is trying to capture the current ra_dav situation, where the
err-message is correct but err-apr_err isn't? (and will make the test
fail over ra_neon/ra_serf)
In other news, I've been thinking about moving the entire validation
logic to the C helper; that is, to have it get an extra argv argument
saying whether the revpropchange should pass or fail, and test by itself
that it passed/failed as expected; and the Python tests would always
expect it to exit(0). What do you think?
Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
===
--- subversion/tests/cmdline/atomic-ra-revprop-change.c (revision
998620)
+++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working copy)
@@ -226,6 +226,8 @@
http_library, pool);
if (err)
{
+ if (svn_error_has_cause(err, SVN_ERR_BAD_OLD_VALUE))
+fprintf(stderr, atomic-ra-revprop-change failed due to incorrect
old value.\n);
Or even more directly:
- if (err)
+ if (err err-apr_err == SVN_ERR_BAD_OLD_VALUE)
svn_handle_error2(err, stderr, FALSE, atomic-ra-revprop-change: );
svn_error_clear(err);
exit_code = EXIT_FAILURE;
Terminology (was: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code)
Bert Huijben wrote on Mon, Sep 20, 2010 at 12:17:12 +0200:
I think 'SVN_ERR_BAD_OLD_VALUE' could use a better/more generic name though.
('Old value' is only valid in the specific function context. Maybe extend
this to something more like this 'precondition failed')
In the interest of moving forward I've committed it already with its
current macro name and default message, but we could rename the error
code or the message in a followup commit.
Nominated value? Prevalue? BASE value? Entry value?
Re: [PATCH] issue #3641: svnsync fails to mirror certain dir replaces
Ping! Has anybody reviewed the patch? Too late for 1.6.13, I suppose? Thanks, Tino. On Fri, Jul 09, 2010 at 12:57:16AM +0300, Daniel Shahaf wrote: Daniel Shahaf wrote on Tue, 22 Jun 2010 at 20:50 -: http://subversion.tigris.org/issues/show_bug.cgi?id=3641 Briefly, the issue is that when svnsync encounters the following history: r5 | pm | 2010-05-18 17:53:46 +0100 (Tue, 18 May 2010) Changed paths: A /H (from /A:4) R /H/B (from /X:4) r3 | pm | 2010-05-18 17:53:46 +0100 (Tue, 18 May 2010) Changed paths: A /A/B/C it looks for /H/B/C in the sync source when trying to replay r5. I've attached a patch (with log msg) that seems to have some positive effects: it causes the sync to pass (and properly add children of /X as children of /H/B). r962377 and r962378. I've double-checked the authz aspects of the logic, but nevertheless I'll still welcome a review on the two points detailed below. :-), Daniel I'm asking for review for two reasons: * authz. The whole function is authz-sensitive --- its goal is to represent a copy as an add. I think the patch is okay from this angle, but a second pair of eyes wouldn't hurt. * assertions. The patch asserts that svn_fs_path_change2_t-copyfrom_known is TRUE. However, the FS API used --- svn_fs_paths_changed2() --- does not guarantee that copyfrom_known will in fact be TRUE, and I haven't found a different API that does promise to provide the copyfrom information. (I think the patch only needs this information for directory replaces.) Testing, analysis, reviews are welcome. Daniel -- What we nourish flourishes. - Was wir nähren erblüht. www.tisc.de
Re: svn diff optimization to make blame faster?
On Mon, Sep 20, 2010 at 11:52 AM, Branko Čibej br...@xbc.nu wrote: On 15.09.2010 14:20, Johan Corveleyn wrote: Some update on this: I have implemented this for svn_diff (excluding the identical prefix and suffix of both files, and only then starting to fill up the token tree and let the lcs-agorithm to its thing). It makes a *huge* difference. On my bigfile.xml (1.5 Mb) with only one line changed, the call to svn_diff_diff is ~10 times faster (15-20 ms vs. 150-170 ms). Hmmm ... looks to me like test data tailored to the optimization. :) Nope, that's real data from a real repository, with a normal kind of change that happens here every day. Of course this optimization is most effective if there are a lot of common prefix/suffix lines. If there is a single change in the first line, and a single change in the last one, this optimization will do nothing but introduce a little bit of extra overhead. And it will obviously make the most impact on large files (in fact it's just relative to the ratio of the number of common prefix/suffix lines to the number of lines in between). I'm sorry it takes me longer than expected to post a version of this to the list, but I'm still having some problems with a couple of edge conditions (I'm learning C as I go, and I'm struggling with a couple of pointer calculations/comparisons). I plan to post something during this week... Cheers, -- Johan
RE: [PATCH 2/3] atomic-revprop: Change tests
Hi,
Daniel Shahaf wrote:
Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100:
Hi,
This patch changes the tests for the atomic-revprop feature, so that
they test the error number rather than parsing the error text.
This doesn't work with Serf or Neon yet - they're still TODO. This
gives you a way to test Serf Neon patches. (You might like to
hold off on committing this until Serf and Neon are done).
The patch is against the atomic-revprop branch, and requires
Patch 1 to be applied first. It doesn't apply to trunk (I don't
think the tests have been merged to trunk?)
On trunk there are only the libsvn_fs and libsvn_repos parts. The
libsvn_ra parts, and the associated Python tests, are only on the
branch.
[[[
Change atomic-revprop tests to look at the error number rather
than parsing the error text.
* subversion/tests/cmdline/atomic-ra-revprop-change.c:
(main): When printing an error, check if it's
SVN_ERR_BAD_OLD_VALUE
and print a special message if it is.
* subversion/tests/cmdline/prop_tests.py:
(FAILS_WITH_BPV): Look for the special message.
Patch by: Jon Foster jon.fos...@cabot.co.uk
]]]
So, this is trying to capture the current ra_dav situation, where the
err-message is correct but err-apr_err isn't?
Correct.
(and will make the test fail over ra_neon/ra_serf)
Correct.
In other news, I've been thinking about moving the entire validation
logic to the C helper; that is, to have it get an extra argv argument
saying whether the revpropchange should pass or fail, and test by
itself
that it passed/failed as expected; and the Python tests would always
expect it to exit(0). What do you think?
Sounds like a good idea.
Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
===
--- subversion/tests/cmdline/atomic-ra-revprop-change.c
(revision 998620)
+++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working
copy)
@@ -226,6 +226,8 @@
http_library, pool);
if (err)
{
+ if (svn_error_has_cause(err, SVN_ERR_BAD_OLD_VALUE))
+fprintf(stderr, atomic-ra-revprop-change failed due to
incorrect
Or even more directly:
- if (err)
+ if (err err-apr_err == SVN_ERR_BAD_OLD_VALUE)
Wouldn't that leak an error if the error isn't SVN_ERR_BAD_OLD_VALUE?
Also, the current* design requires us to use svn_error_has_cause()
because
the error is wrapped inside an error with a different code. (FWIW,
I think the RA layer shouldn't do that - I think we should change
svn_ra_change_rev_prop() so that the simple test of err-apr_err works.
But that's not urgent, and could even be postponed to 1.8).
Kind regards,
Jon
(* Unless I missed something and the design's been changed?)
**
This email and its attachments may be confidential and are intended solely for
the use of the individual to whom it is addressed. Any views or opinions
expressed are solely those of the author and do not necessarily represent those
of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you
must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**
__
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
__
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100: Hi, For atomic-revprop, the client needs to know if the error was SVN_ERR_BAD_OLD_VALUE or not. For ra_local and ra_svn this is already done; this patch does it for DAV (with Neon). With mod_dav, we only have 2 ways to affect the 207 multistatus response from a failed PROPPATCH: - We can set the text that appears in D:responsedescription, including injecting arbitrary XML. - We can set the D:status value to a (numeric) HTTP error code. There is also dav_svn__error_response_tag(). The approach that's been discussed on-list and on IRC was to inject the error as XML inside D:responsedescription. I did actually implement that, only to discover that Neon completely rejects the XML in that case[1]. While we would obviously fix this in the 1.7 client code, it could break compatibility between the 1.7 server and 1.6 and older clients. The only way to handle In other words, changing that table means changing the protocol, which isn't backwards-compatible. Good call. this would be to introduce a new client capability, so the server could fall back to the old code for 1.6 clients. This means we'd have two code paths for error-handling... this gets very complex. However, there is a simpler way! The D:status element contains the HTTP error code, usually 500 Internal Server Error. So we could pick a special HTTP status code to mean SVN_ERR_BAD_OLD_VALUE. I think of old_value_p as a precondition for the operation, a bit like If-Modified-Since:, so I'd suggest 412 Precondition Failed. Note that generic DAV clients won't ever see this message, because they won't be sending old_value_p. Fair enough. Are there currently any circumstances where HTTP_PRECONDITION_FAILED would be raised? (e.g., by mod_dav) As far as I can tell (by greping httpd sources), the answer is No. This patch only does mod_dav_svn and Neon. I don't want to waste time doing both HTTP libraries if this patch is rejected. (Also, Serf is slightly harder because it doesn't parse D:status yet). The patch is against the atomic-revprop branch, and requires Patch 1 to be applied first. (It does apply to trunk, too). [[[ Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status code inside a 207 multi-status response. * subversion/mod_dav_svn/util.c: (dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412. * subversion/libsvn_ra_neon/util.c: (multistatus_baton_t): New member contains_precondition_error. (end_207_element): Check for HTTP 412 status code and create a SVN_ERR_BAD_OLD_VALUE error if found. Patch by: Jon Foster jon.fos...@cabot.co.uk ]]] Tested it and it works fine: [[[ CMD: atomic-ra-revprop-change http://localhost:8081/svn-test-work/repositories/prop_tests-34 0 flower ( 11 old_value_p 0 5 value 6 violet ) neon CMD: /home/daniel/src/svn/atomic-revprop/subversion/tests/cmdline/atomic-ra-revprop-change http://localhost:8081/svn-test-work/repositories/prop_tests-34 0 flower ( 11 old_value_p 0 5 value 6 violet ) neon exited with 1 TIME = 0.305840 subversion/tests/cmdline/atomic-ra-revprop-change.c:156: (apr_err=175002) subversion/libsvn_ra_neon/fetch.c:1210: (apr_err=175002) atomic-ra-revprop-change: DAV request failed; it's possible that the repository's pre-revprop-change hook either failed or is non-existent subversion/libsvn_ra_neon/props.c:1231: (apr_err=175008) atomic-ra-revprop-change: At least one property change failed; repository is unchanged subversion/libsvn_ra_neon/util.c:1550: (apr_err=125014) subversion/libsvn_ra_neon/util.c:236: (apr_err=125014) atomic-ra-revprop-change: Error setting property 'flower': revprop 'flower' has unexpected value in filesystem ]]] where 125014 is SVN_ERR_BAD_OLD_VALUE. :-) I'll commit this once I have an ra_serf version too. Would you like to write the ra_serf part of the patch, too? Thanks! Daniel Kind regards, Jon [1] More precisely, libsvn_ra_neon/util.c:wrapper_startelm_cb() raises a SVN_ERR_XML_MALFORMED XML data was not well-formed error if D:responsedescription contains any XML tags. See the ELEM_responsedescription row in multistatus_nesting_table[] in the same file - it explicitly states that if there's any XML tags inside D:responsedescription then the XML is invalid. Index: subversion/mod_dav_svn/util.c === --- subversion/mod_dav_svn/util.c (revision 998620) +++ subversion/mod_dav_svn/util.c (working copy) @@ -107,6 +107,9 @@ case SVN_ERR_FS_PATH_ALREADY_LOCKED: status = HTTP_LOCKED; break; + case SVN_ERR_BAD_OLD_VALUE: +status = HTTP_PRECONDITION_FAILED; +break; /* add other mappings here */ } Index: subversion/libsvn_ra_neon/util.c === ---
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100: This patch only does mod_dav_svn and Neon. I don't want to waste time doing both HTTP libraries if this patch is rejected. You always have the option of discussing your plans here or on IRC before starting to write the patches. :-) Feel free to contact me directly, too (via either mail or IRC /msg). Thanks for jumping in! Daniel
Re: [PATCH 2/3] atomic-revprop: Change tests
Jon Foster wrote on Mon, Sep 20, 2010 at 12:39:31 +0100:
Hi,
Daniel Shahaf wrote:
Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100:
Hi,
This patch changes the tests for the atomic-revprop feature, so that
they test the error number rather than parsing the error text.
This doesn't work with Serf or Neon yet - they're still TODO. This
gives you a way to test Serf Neon patches. (You might like to
hold off on committing this until Serf and Neon are done).
The patch is against the atomic-revprop branch, and requires
Patch 1 to be applied first. It doesn't apply to trunk (I don't
think the tests have been merged to trunk?)
On trunk there are only the libsvn_fs and libsvn_repos parts. The
libsvn_ra parts, and the associated Python tests, are only on the
branch.
[[[
Change atomic-revprop tests to look at the error number rather
than parsing the error text.
* subversion/tests/cmdline/atomic-ra-revprop-change.c:
(main): When printing an error, check if it's
SVN_ERR_BAD_OLD_VALUE
and print a special message if it is.
* subversion/tests/cmdline/prop_tests.py:
(FAILS_WITH_BPV): Look for the special message.
Patch by: Jon Foster jon.fos...@cabot.co.uk
]]]
So, this is trying to capture the current ra_dav situation, where the
err-message is correct but err-apr_err isn't?
Correct.
Okay. Perhaps this could have been called out more explicitly in the
log message --- i.e., have it say not only *what* the patch does, but
also *why* it does that.
(and will make the test fail over ra_neon/ra_serf)
Correct.
In other news, I've been thinking about moving the entire validation
logic to the C helper; that is, to have it get an extra argv argument
saying whether the revpropchange should pass or fail, and test by
itself
that it passed/failed as expected; and the Python tests would always
expect it to exit(0). What do you think?
Sounds like a good idea.
Okay. I started a patch in that way at a time; I've touched it up now
a tiny bit and I'm attaching it. Let me know what you think...
(I've tested it when applied on top of your patch 3/3.)
Index: subversion/tests/cmdline/atomic-ra-revprop-change.c
===
--- subversion/tests/cmdline/atomic-ra-revprop-change.c
(revision 998620)
+++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working
copy)
@@ -226,6 +226,8 @@
http_library, pool);
if (err)
{
+ if (svn_error_has_cause(err, SVN_ERR_BAD_OLD_VALUE))
+fprintf(stderr, atomic-ra-revprop-change failed due to
incorrect
Or even more directly:
- if (err)
+ if (err err-apr_err == SVN_ERR_BAD_OLD_VALUE)
Wouldn't that leak an error if the error isn't SVN_ERR_BAD_OLD_VALUE?
Yes, it would.
Also, the current* design requires us to use svn_error_has_cause()
because the error is wrapped inside an error with a different code.
Yup, I've noticed this in the verbose test output I've pasted
elsethread, but you beat me to replying with the correction :)
(FWIW, I think the RA layer shouldn't do that - I think we should
change svn_ra_change_rev_prop() so that the simple test of
err-apr_err works. But that's not urgent, and could even be
postponed to 1.8).
First of all, agreed that this is small change; I think it's more
important to be able to promise that the error situation will be
/recognizable/ then whether the recognition will be with or without
svn_error_has_cause().
For reference, this is the error chain currently (with your patch 3/3):
[[[
subversion/tests/cmdline/atomic-ra-revprop-change.c:156: (apr_err=175002)
subversion/libsvn_ra_neon/fetch.c:1210: (apr_err=175002)
atomic-ra-revprop-change: DAV request failed; it's possible that the
repository's pre-revprop-change hook either failed or is
+non-existent
subversion/libsvn_ra_neon/props.c:1231: (apr_err=175008)
atomic-ra-revprop-change: At least one property change failed; repository is
unchanged
subversion/libsvn_ra_neon/util.c:1550: (apr_err=125014)
subversion/libsvn_ra_neon/util.c:236: (apr_err=125014)
atomic-ra-revprop-change: Error setting property 'flower':
revprop 'flower' has unexpected value in filesystem
]]]
Kind regards,
Jon
(* Unless I missed something and the design's been changed?)
Index: subversion/tests/cmdline/svntest/actions.py
===
--- subversion/tests/cmdline/svntest/actions.py (revision 998887)
+++ subversion/tests/cmdline/svntest/actions.py (working copy)
@@ -152,7 +152,8 @@
expected_stderr,
expected_exit,
url, revision, propname,
-old_propval, propval):
+
Re: [PATCH] issue #3641: svnsync fails to mirror certain dir replaces
Tino Schwarze wrote on Mon, Sep 20, 2010 at 12:59:46 +0200: Ping! Has anybody reviewed the patch? Too late for 1.6.13, I suppose? No. I've just added it to STATUS. That's the easiest way to ask for reviews here :-) Daniel Thanks, Tino. On Fri, Jul 09, 2010 at 12:57:16AM +0300, Daniel Shahaf wrote: Daniel Shahaf wrote on Tue, 22 Jun 2010 at 20:50 -: http://subversion.tigris.org/issues/show_bug.cgi?id=3641 Briefly, the issue is that when svnsync encounters the following history: r5 | pm | 2010-05-18 17:53:46 +0100 (Tue, 18 May 2010) Changed paths: A /H (from /A:4) R /H/B (from /X:4) r3 | pm | 2010-05-18 17:53:46 +0100 (Tue, 18 May 2010) Changed paths: A /A/B/C it looks for /H/B/C in the sync source when trying to replay r5. I've attached a patch (with log msg) that seems to have some positive effects: it causes the sync to pass (and properly add children of /X as children of /H/B). r962377 and r962378. I've double-checked the authz aspects of the logic, but nevertheless I'll still welcome a review on the two points detailed below. :-), Daniel I'm asking for review for two reasons: * authz. The whole function is authz-sensitive --- its goal is to represent a copy as an add. I think the patch is okay from this angle, but a second pair of eyes wouldn't hurt. * assertions. The patch asserts that svn_fs_path_change2_t-copyfrom_known is TRUE. However, the FS API used --- svn_fs_paths_changed2() --- does not guarantee that copyfrom_known will in fact be TRUE, and I haven't found a different API that does promise to provide the copyfrom information. (I think the patch only needs this information for directory replaces.) Testing, analysis, reviews are welcome. Daniel -- What we nourish flourishes. - Was wir nähren erblüht. www.tisc.de
RE: Terminology (was: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code)
-Original Message-
From: 'Daniel Shahaf' [mailto:d...@daniel.shahaf.name]
Sent: maandag 20 september 2010 12:58
To: Bert Huijben
Cc: 'Jon Foster'; 'Subversion Development'
Subject: Terminology (was: [PATCH 3/3] atomic-revprop: Signal the error
as a HTTP status code)
Bert Huijben wrote on Mon, Sep 20, 2010 at 12:17:12 +0200:
I think 'SVN_ERR_BAD_OLD_VALUE' could use a better/more generic name
though.
('Old value' is only valid in the specific function context. Maybe
extend
this to something more like this 'precondition failed')
In the interest of moving forward I've committed it already with its
current macro name and default message, but we could rename the error
code or the message in a followup commit.
Nominated value? Prevalue? BASE value? Entry value?
I think the error should be in the SVN_ERR_RA (or if it originates there:
FS) prefix, not just 'SVN_ERR_BAD' and specify more what happened. (The BAD
category doesn't carry any information on what failed and is used only for
argument validation errors. This makes it hard to diagnose the error from
just the error code. And with our localized error messages that is all
applications that use libsvn_client can do)
So something like:
SVN_ERR_RA_PROP_BASEVALUE_MISMATCH
(specifies the RA problem)
Or just
SVN_ERR_RA_PRECONDITION_FAILED
(generic RA precondition problem)
Bert
Re: UTF-8 NFC/NFD paths issue
Sorry to have left the discussion running so long without contributing to it myself. The reason I started about changing the repository / fs is because it is where we store the dataset that we'll need to support forever: working copies get destroyed and checked out over and over every hour, every day. Repositories get created once and only accumulate data. That doesn't solve the historical revisions containing bad paths. My understanding of the problem was that we'd go into the past and rewrite the paths into a single, canonical form. Agreed: an out-of-band solution fixes thing historically too. As pointed out on IRC, I think it's important to stop adding semantically the same paths to a repository. From the perspective of efficiency, it might be handy to have a normalized version stored somewhere for all paths living in the repository, but to prevent addition of differently encoded paths, such a thing isn't really required: the correct encoding can be calculated when the check happens. Having backend enforce NFC can wait for 2.0 I suppose :) True, but the value of that might be limited: if we required all communications to be NFC encoded, we need to take additional measures - as pointed out by Branko - to make things work on MacOS X: currently, we have MacOS X shops happily working with non-ascii characters in the paths, all NFD encoded. That would change. By the way, Julian Foad, Philip Martin, Bert Huijben and I talked through a possible solution to fix the client-side issue which becomes an option once we switch to wc-ng. The full impact of that change needs to be determined though and probably does not fit in the 1.7 timeline. If it seems it does, we'll bring it up. To recap, the change I'm proposing is that we check pathnames with NFC/D aware comparison routines upon add_file() / add_directory() inside libsvn_repos or libsvn_fs_* - of which I suspect it's easier to handle inside the latter. In my proposal, we don't specify a repository normal encoding. If performance degrades too much, we can enhance the filesystem with a normalized version which doesn't need to be recoded in order to do the comparison with the incoming path. Other than that, I don't think there's anything *required* to make us Unicode-aware on the server. It's also the change I'm proposing cmpilato to implement in libsvn_fs_base as a proof of concept. This proposal says nothing about the client side. The client side can be fixed independently from the server side, given that we can't switch to normalized paths in the protocol until 2.0: whatever paths a server sends, the client will need to use those to communicate back to the server. Bye, Erik.
Re: [PATCH] issue #3641: svnsync fails to mirror certain dir replaces
Tino Schwarze wrote on Mon, Sep 20, 2010 at 16:28:23 +0200: On Mon, Sep 20, 2010 at 02:49:42PM +0200, Daniel Shahaf wrote: Has anybody reviewed the patch? Too late for 1.6.13, I suppose? No. I've just added it to STATUS. That's the easiest way to ask for reviews here :-) Thanks, but the Justification is not just Could lead to sync'd repositories being different from the master. It's more severe: svnsync fails to copy certain operations. It's actually a your sync totally stops working and you can't do anything kind of failure - see http://subversion.tigris.org/issues/show_bug.cgi?id=3641 If you happen to run into this: svnsync: File not found: revision 5, path '/H/B/C' you won't be able to sync the repository anymore after that. Which I consider rather disastrous given that we're going to use svnsync to provide partial copies of our repository to our customers. Scenario #1. As you describe: svnsync bombs out and refuses to continue. Scenario #2. Per my comment on the issue, svnsync looks for /H/B/C --- which DOES happen to exist --- and copies that to the target repository. #1 is noisy failure. #2 is silent corruption (the source and target repositories differ). So, yes, both of them are possible outcomes; #2 is more severe and #1 is more likely to occur in practice. :-) HTH, Tino. PS: Daniel, please do not take offense in that I first sent you a jay! mal in private, then this more critical one in public - I've just read the STATUS after my first mail (which, I felt, did not belong on a public list). Not a hint of offence was taken. -- What we nourish flourishes. - Was wir nähren erblüht. www.tisc.de
Re: [PATCH] NODES table presence values
On Thu, Sep 16, 2010 at 09:21, Julian Foad julian.f...@wandisco.com wrote: Greg Stein wrote: ... Also, please note that I want to expand the presence values dramatically with this move to NODES. I suggest the following new values: Can you explain what these would mean, and what are the main advantages? Primarily: no need to scan to figure out what is going on. Some of them look obvious at first glance but the details of child-of-copy etc. are quite complex. If I were to start to guess... For op_depth = 0, we keep using the current set of 'presence' values, as defined for BASE_NODE. Yes. For op_depth 0, these new values replace the old 'normal', 'base-deleted' and 'not-present' presence values and the old 'moved_here' flag. The old 'excluded' and 'incomplete' are still used. Yes. * copied [-here] * moved-here This is a root or a child of a copied/moved sub-tree. 'op_depth' compared with 'local_relpath' depth indicates whether it's the root. 'copyfrom_*' is non-null iff it's the root (?). Correct on all three parts. Note: the columns should be named original_* or repos_*, as discussed elsewhere. Previous encoding: If it's the root, presence==normal + non-null 'copyfrom_*' + 'moved_here'=FALSE/TRUE. If it's a child, presence==normal and scan_addition reveals it's part of a copy/move. Benefits: Avoid scan_addition. Remove the 'moved_here' flag. Yes, yes. * moved-away (aka deleted) This is a root or a child of a moved-away sub-tree. The new WC location (local relpath) is in 'moved_to' iff it's the root. Correct. With root detection based on op_depth and local_relpath as mentioned above. Previous encoding: presence==base-deleted; if it's the root, non-null 'moved_to', else scan_deletion reveals it's a move-away. The presence could be 'deleted' if you were moving a child of a copy/move-here. But note that wc_db does not implement moves, so we never got to these encoding details. Benefits: Avoid scan_deletion. Yes. * deleted This node is deleted relative to the layer below. Previous encoding: 'base-deleted' or 'not-present' depending on whether it was a child of a copied subtree. Correct. * added A simple addition. Previous encoding: presence==normal, copyfrom_*==null, scan_addition reveals it's a simple add. Benefits: avoid scan_addition. Yes on all. ... Are my notes close to what you were thinking? (I'm trying to write out the states in a table to be more rigorous about it.) Spot-on :-D (part of the reason to expand the set: their semantics become obvious, compared to the current encoding) If this does enable us to eliminate the use of scan_addition and scan_deletion in some cases, even if they (or some simpler versions) are still needed for finding the copyfrom info in other cases, that would be a worthwhile change. I was originally trying to be minimalist with the presence values, but after use/implementation it became obvious that we can simply encode operations in the nodes rather than requiring a scan for them. scan_addition will receive the most benefit since it resolves an add to an add/copy-here/move-here. Or in today's implementation add vs copy. The scan_deletion never really has to resolve since we don't truly implement moved-away. The *concepts* are still in the code because I think we need the design and direction rigor. For the 1.7 release, we should keep scan_* and just change their internals. We can rethink other APIs for future releases since these are internal to WC. The implementation is generally reading just a row or two: read the target row, examine its op_depth, and read the operation root's row. We don't have to scan upwards to find the operation root. Cheers, -g
Re: [PATCH] NODES table presence values
On Mon, 2010-09-20 at 13:17 -0400, Greg Stein wrote: On Thu, Sep 16, 2010 at 09:21, Julian Foad julian.f...@wandisco.com wrote: Greg Stein wrote: ... Also, please note that I want to expand the presence values dramatically with this move to NODES. I suggest the following new values: Can you explain what these would mean, and what are the main advantages? Primarily: no need to scan to figure out what is going on. Some of them look obvious at first glance but the details of child-of-copy etc. are quite complex. If I were to start to guess... For op_depth = 0, we keep using the current set of 'presence' values, as defined for BASE_NODE. Yes. For op_depth 0, these new values replace the old 'normal', 'base-deleted' and 'not-present' presence values and the old 'moved_here' flag. The old 'excluded' and 'incomplete' are still used. Yes. * copied [-here] * moved-here This is a root or a child of a copied/moved sub-tree. 'op_depth' compared with 'local_relpath' depth indicates whether it's the root. 'copyfrom_*' is non-null iff it's the root (?). Correct on all three parts. Note: the columns should be named original_* or repos_*, as discussed elsewhere. Previous encoding: If it's the root, presence==normal + non-null 'copyfrom_*' + 'moved_here'=FALSE/TRUE. If it's a child, presence==normal and scan_addition reveals it's part of a copy/move. Benefits: Avoid scan_addition. Remove the 'moved_here' flag. Yes, yes. * moved-away (aka deleted) This is a root or a child of a moved-away sub-tree. The new WC location (local relpath) is in 'moved_to' iff it's the root. Correct. With root detection based on op_depth and local_relpath as mentioned above. Previous encoding: presence==base-deleted; if it's the root, non-null 'moved_to', else scan_deletion reveals it's a move-away. The presence could be 'deleted' if you were moving a child of a copy/move-here. But note that wc_db does not implement moves, so we never got to these encoding details. Benefits: Avoid scan_deletion. Yes. * deleted This node is deleted relative to the layer below. Previous encoding: 'base-deleted' or 'not-present' depending on whether it was a child of a copied subtree. Correct. * added A simple addition. Previous encoding: presence==normal, copyfrom_*==null, scan_addition reveals it's a simple add. Benefits: avoid scan_addition. Yes on all. ... Are my notes close to what you were thinking? (I'm trying to write out the states in a table to be more rigorous about it.) Spot-on :-D (part of the reason to expand the set: their semantics become obvious, compared to the current encoding) OK, good, thanks. *More* obvious, yes. (Obvious, not quite: it still took me half a day of figuring to come up with this concise description.) FULL SET OF VALUES The values listed above cover most of the cases. Next we must consider how to get a full set of values to represent all possible changes. The possible changes to an unoccupied path are: added copied-here (as root or as child of op) moved-here (as root or as child of op) and the possible changes to an occupied path are: [ delete moved-away (as root or as child of op) ] x [ no replacement added copied-here (as root or as child of op) moved-here (as root or as child of op) ] Are you planning to encode all these combinations as separate values? That's 11 in total, plus whatever valid combinations that involve excluded/absent/file-external. - Julian If this does enable us to eliminate the use of scan_addition and scan_deletion in some cases, even if they (or some simpler versions) are still needed for finding the copyfrom info in other cases, that would be a worthwhile change. I was originally trying to be minimalist with the presence values, but after use/implementation it became obvious that we can simply encode operations in the nodes rather than requiring a scan for them. scan_addition will receive the most benefit since it resolves an add to an add/copy-here/move-here. Or in today's implementation add vs copy. The scan_deletion never really has to resolve since we don't truly implement moved-away. The *concepts* are still in the code because I think we need the design and direction rigor. For the 1.7 release, we should keep scan_* and just change their internals. We can rethink other APIs for future releases since these are internal to WC. The implementation is generally reading just a row or two: read the target row, examine its op_depth, and read the operation root's row. We don't have to scan upwards to find the operation root. Cheers, -g
Re: [PATCH] NODES table presence values
On Mon, 2010-09-20 at 18:45 +0100, Julian Foad wrote: On Mon, 2010-09-20 at 13:17 -0400, Greg Stein wrote: On Thu, Sep 16, 2010 at 09:21, Julian Foad julian.f...@wandisco.com wrote: Greg Stein wrote: ... Also, please note that I want to expand the presence values dramatically with this move to NODES. I suggest the following new values: Can you explain what these would mean, and what are the main advantages? Primarily: no need to scan to figure out what is going on. Some of them look obvious at first glance but the details of child-of-copy etc. are quite complex. If I were to start to guess... For op_depth = 0, we keep using the current set of 'presence' values, as defined for BASE_NODE. Yes. For op_depth 0, these new values replace the old 'normal', 'base-deleted' and 'not-present' presence values and the old 'moved_here' flag. The old 'excluded' and 'incomplete' are still used. Yes. * copied [-here] * moved-here This is a root or a child of a copied/moved sub-tree. 'op_depth' compared with 'local_relpath' depth indicates whether it's the root. 'copyfrom_*' is non-null iff it's the root (?). Correct on all three parts. Note: the columns should be named original_* or repos_*, as discussed elsewhere. Previous encoding: If it's the root, presence==normal + non-null 'copyfrom_*' + 'moved_here'=FALSE/TRUE. If it's a child, presence==normal and scan_addition reveals it's part of a copy/move. Benefits: Avoid scan_addition. Remove the 'moved_here' flag. Yes, yes. * moved-away (aka deleted) This is a root or a child of a moved-away sub-tree. The new WC location (local relpath) is in 'moved_to' iff it's the root. Correct. With root detection based on op_depth and local_relpath as mentioned above. Previous encoding: presence==base-deleted; if it's the root, non-null 'moved_to', else scan_deletion reveals it's a move-away. The presence could be 'deleted' if you were moving a child of a copy/move-here. But note that wc_db does not implement moves, so we never got to these encoding details. Benefits: Avoid scan_deletion. Yes. * deleted This node is deleted relative to the layer below. Previous encoding: 'base-deleted' or 'not-present' depending on whether it was a child of a copied subtree. Correct. * added A simple addition. Previous encoding: presence==normal, copyfrom_*==null, scan_addition reveals it's a simple add. Benefits: avoid scan_addition. Yes on all. ... Are my notes close to what you were thinking? (I'm trying to write out the states in a table to be more rigorous about it.) Spot-on :-D (part of the reason to expand the set: their semantics become obvious, compared to the current encoding) OK, good, thanks. *More* obvious, yes. (Obvious, not quite: it still took me half a day of figuring to come up with this concise description.) FULL SET OF VALUES The values listed above cover most of the cases. Next we must consider how to get a full set of values to represent all possible changes. The possible changes to an unoccupied path are: added copied-here (as root or as child of op) moved-here (as root or as child of op) and the possible changes to an occupied path are: [ delete moved-away (as root or as child of op) ] x [ no replacement added copied-here (as root or as child of op) moved-here (as root or as child of op) ] Are you planning to encode all these combinations as separate values? That's 11 in total, plus whatever valid combinations that involve excluded/absent/file-external. From IRC: julianf ehu: Re. deletes: I think you're feeling, and I feel as well a bit, that delete and move-away are really operations on the layer below and thus should be indicated within the layer below: there is no perhaps need to create a new row because there is no repos-node-id or node-kind or other node information to store about a delete or move-away. The only info there is ... the fact that it's del or mv-away, and (possibly) this (not really well defined) moved_to path. ehu yes. that's my feeling. I understand the choices with respect to where we are today. but the question is if the single presence value in the top-level layer really does what it needs to, now that we're moving to a multi-layer model. julianf ehu: So maybe it makes sense to swap the layering of the creation part of a NODES row and the deletion part of a row. Let the creation part be considered as the lower half of the layer, and then a possible deleted/moved-to-PATH indication to be considered as an operation above the creation (in the sense of nearer to the user's working version). - Julian If this does enable us to eliminate the use of scan_addition and scan_deletion in some cases,
Re: [PATCH] NODES table presence values
On Mon, Sep 20, 2010 at 14:07, Julian Foad julian.f...@wandisco.com wrote: On Mon, 2010-09-20 at 18:45 +0100, Julian Foad wrote: ... FULL SET OF VALUES The values listed above cover most of the cases. Next we must consider how to get a full set of values to represent all possible changes. The possible changes to an unoccupied path are: added copied-here (as root or as child of op) moved-here (as root or as child of op) and the possible changes to an occupied path are: [ delete moved-away (as root or as child of op) ] x [ no replacement added copied-here (as root or as child of op) moved-here (as root or as child of op) ] Are you planning to encode all these combinations as separate values? That's 11 in total, plus whatever valid combinations that involve excluded/absent/file-external. From IRC: julianf ehu: Re. deletes: I think you're feeling, and I feel as well a bit, that delete and move-away are really operations on the layer below and thus should be indicated within the layer below: there is no perhaps need to create a new row because there is no repos-node-id or node-kind or other node information to store about a delete or move-away. The only info there is ... the fact that it's del or mv-away, and (possibly) this (not really well defined) moved_to path. ehu yes. that's my feeling. I understand the choices with respect to where we are today. but the question is if the single presence value in the top-level layer really does what it needs to, now that we're moving to a multi-layer model. julianf ehu: So maybe it makes sense to swap the layering of the creation part of a NODES row and the deletion part of a row. Let the creation part be considered as the lower half of the layer, and then a possible deleted/moved-to-PATH indication to be considered as an operation above the creation (in the sense of nearer to the user's working version). Erik and I talked further on IRC... I believe the right approach is a simple boolean prior-deleted, meaning the nodes visible just under *this* layer have been deleted. Examining the root node's moved_to column can refine how the subtree was deleted/moved-away. I dislike the concept of modifying prior layers (preferring to see them as inviolate/readonly until I revert recent layers). If I say delete, then it should be a new layer describing which nodes got deleted. Several more things came up in conversation: * a simple rule for is this revertable? is does the node's op_depth match its path component count? * adds have variant op_depth values in a subtree. thus, each node is revertable (and implicitly reverting its subtree) * deletes have a single op_depth value, making only the root revertable. when deleting a node, all previously-deleted children will need their op_depth updated And I identified one more problem here [in discussion with Erik now]: * svn mv A B ; svn add A ; svn add A/foo The op_depth of A and A/foo (assuming the latter existed in the original A) has the value 1. After the adds, A has 1, and A/foo has 2. Thus, we lose the op_depth defined for the move. We can certainly scan upwards to find that root (tho we've been wanting to skip these kinds of scans). Any thoughts? Cheers, -g
Re: [PATCH] NODES table presence values
On Mon, Sep 20, 2010 at 15:36, Greg Stein gst...@gmail.com wrote: ... Erik and I talked further on IRC... I believe the right approach is a simple boolean prior-deleted, meaning the nodes visible just under *this* layer have been deleted. Examining the root node's moved_to column can refine how the subtree was deleted/moved-away. I dislike the concept of modifying prior layers (preferring to see them as inviolate/readonly until I revert recent layers). If I say delete, then it should be a new layer describing which nodes got deleted. Several more things came up in conversation: * a simple rule for is this revertable? is does the node's op_depth match its path component count? * adds have variant op_depth values in a subtree. thus, each node is revertable (and implicitly reverting its subtree) * deletes have a single op_depth value, making only the root revertable. when deleting a node, all previously-deleted children will need their op_depth updated And I identified one more problem here [in discussion with Erik now]: * svn mv A B ; svn add A ; svn add A/foo The op_depth of A and A/foo (assuming the latter existed in the original A) has the value 1. After the adds, A has 1, and A/foo has 2. Thus, we lose the op_depth defined for the move. We can certainly scan upwards to find that root (tho we've been wanting to skip these kinds of scans). Okay. We've identified a possible solution here. But first let me back up a bit to clarify the exact problem space. 1. a node (and its children) are deleted. 2. a new subtree is added, copied-here, or moved-here. The problem arises *only* in the added case since the op_depth values within an added subtree will vary. For a copied/moved-here, the op_depth values will specify the root and that root *must* be the same as the deletion root (if the deletion root was an ancestor, then you could not copy/move here because a parent is missing; if the deletion root was a descendent, then you haven't cleared the way for the copy/move to happen). Thus, we are only speaking to an added subtree. Nodes in that subtree are marked with a prior node was deleted before this node could be added here. But we don't know where the root of that deletion is, without scanning for it. Our op_depth values were rejiggered to specify each added node as its own root. And recall: I don't think that we want to modify prior layers. We'd probably just end up in a similar kind of something got overwritten position, and I'd prefer each operation to define a new layer so that we have a 1:1 linkage between operations and layers (with the caveat of delete+something is a replacing operation). On IRC, I proposed to Erik that we switch that boolean to a column named something like deletion_op_depth. When you first delete a subtree, then all nodes' op_depth and deletion_op_depth are set to the value based on the root. If you later delete an ancestor, then both columns are rewritten as proposed earlier. But as you add nodes, with variant op_depths, you leave the deletion_op_depth alone. You can then recover the deletion root by examining deletion_op_depth. If you revert an add, then you can (re)set its op_depth to the deletion_op_depth to incorporate that node back into the original deletion operation. (and set presence to 'deleted', of course). This allows us to avoid scanning for the root, and allows us to properly restore fields of a revert. One last point: in a copied-here/moved-here subtree, if a child is deleted (and maybe later replaced), then it will have a whole new op_depth (and deletion_op_depth) and will create a new layer. We're still good with respect to mixing these operations across subtrees. In terms of scanning: * for an added node, we have to scan for the root of the added subtree because we do not have that information * for an added node, if it is replacing a prior node, then we know its root from deletion_op_depth * for copied/moved-here nodes, we know its root, where we can find the source information * for a deleted node, we know its root from op_depth. we cannot determine deleted vs moved-away without examining that root * for moved-away nodes, we need to look in the root to determine where it went -- Whoops. I just realized something: Given that children within an added subtree have variant op_depth values, that also means they define separate layers. That means the nodes describing the original deletion are *still* in the layer defined by the deletion. We might not need deletion_op_depth at all! The later adds are simply shadowing those deletions. ... okay. I'm not going to edit this email, but leave it all here for posterity. A little reset is necessary, and start a new thread/document :-P Cheers, -g
add NODES.prior_deleted boolean column
After working through the several email messages, and discussion, I believe we're now down to a simple change: * add a prior_deleted flag to NODES The flag simply means that a node exists prior to this layer and has been deleted or moved-away. The 'presence' column may say the same thing, but it might also describe data that is replacing the deletion/move. When a deletion (of a subtree) occurs, then we create a new layer at relpath, op_depth. New rows are written for the root, and all children, using that op_depth value. If this is a moved-away, then we store the destination into moved_to at the root *only* (which can then later discriminate between the two types of deletions; children need to look to the root to discriminate; I bet this need is rare). Note that the deletion process needs to look for mods to descendents: deletes are integrated into this one; other operations may error with can't delete local mods or somesuch. For the following actions, these are applied to the root of a deletion: If an add occurs, then the root is updated to set presence='added'. No other changes are needed. If a copied-here or moved-here occurs, then the root is updated with the appropriate status and source information. Child nodes *may* have their presence switched from 'deleted' to 'copied-here' or 'moved-here' (depends on whether the arriving nodes intersect with the old namespace). New nodes may be introduced, with presence=$whatever and prior_deleted=0 (FALSE) If a deletion of a child (subtree) of copied-here or moved-here occurs, then it has a new op_depth and defines a whole new layer. The prior_deleted is set to 1 (TRUE) indicating the prior layer (which happens to be the copy/move-here) has been deleted. Deletion of an add is effectively a revert. If this is a child, then the layer is simply removed (it only has one node). If the deletion/revert of an add has prior_deleted=1 (meaning it is a root), then the node is rewritten to presence='deleted', restoring it to the state when the deletion first occurred. (and yes, a second revert undoes the deletion, etc...). Reverting a child of a moved/copied-here tree is invalid. When you revert the root, then the children at this op_depth are traversed: any nodes with prior_deleted=1 are restored to presence=deleted, and nodes with prior_deleted=0 (newly-arrived from the copy/move) are simply removed. Note that prior_deleted is set to TRUE only for a deletion operation (when presence is set to 'deleted'). That implies a prior node existed. For the sequence [rm A/B, add A/B, add A/B/foo], the node A/B/foo will have op_depth=3 and prior_deleted=0 since the row was created by an add. Assuming that A/B/foo existed originally, then prior_deleted=1 at A/B/foo, op_depth=2. I think that is it. Summarized a bit better from the earlier thread. Cheers, -g
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Daniel Shahaf wrote: Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100: However, there is a simpler way! The D:status element contains the HTTP error code, usually 500 Internal Server Error. So we could pick a special HTTP status code to mean SVN_ERR_BAD_OLD_VALUE. I think of old_value_p as a precondition for the operation, a bit like If-Modified-Since:, so I'd suggest 412 Precondition Failed. Note that generic DAV clients won't ever see this message, because they won't be sending old_value_p. Fair enough. Are there currently any circumstances where HTTP_PRECONDITION_FAILED would be raised? (e.g., by mod_dav) As far as I can tell (by greping httpd sources), the answer is No. I looked and couldn't find any. The DAV spec doesn't list it as one of the common errors that get sent in that place. (Obviously, it would be very bad if mod_dav started sending that error code for some other error). I'll commit this once I have an ra_serf version too. Would you like to write the ra_serf part of the patch, too? Sure. See attached. -b-description-data); +*b-description-data); ^^^ Unintentional change? (triggers compiler warning) Oops, yes. Fixed in attached. This updated patch is against the atomic-revprop branch. [[[ Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status code inside a 207 multi-status response. * subversion/mod_dav_svn/util.c: (dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412. * subversion/libsvn_ra_neon/util.c: (multistatus_baton_t): New member contains_precondition_error. (end_207_element): Check for HTTP 412 status code and create a SVN_ERR_BAD_OLD_VALUE error if found. * subversion/libsvn_ra_serf/ra_serf.h: (svn_ra_serf__server_error_t): New member contains_precondition_error. * subversion/libsvn_ra_serf/util.c: (parse_dav_status): New method. (svn_ra_serf__handle_discard_body): Initialise new member. (start_207): Parse DAV:status XML element. (end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE error code if applicable. (svn_ra_serf__handle_multistatus_only): Initialise new member. Patch by: Jon Foster jon.fos...@cabot.co.uk ]]] Kind regards, Jon ** This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd. If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone. Cabot Communications Limited Verona House, Filwood Road, Bristol BS16 3RY, UK +44 (0) 1179584232 Co. Registered in England number 02817269 Please contact the sender if you believe you have received this email in error. ** __ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email __Index: subversion/mod_dav_svn/util.c === --- subversion/mod_dav_svn/util.c (revision 999102) +++ subversion/mod_dav_svn/util.c (working copy) @@ -107,6 +107,9 @@ dav_svn__convert_err(svn_error_t *serr, case SVN_ERR_FS_PATH_ALREADY_LOCKED: status = HTTP_LOCKED; break; + case SVN_ERR_BAD_OLD_VALUE: +status = HTTP_PRECONDITION_FAILED; +break; /* add other mappings here */ } Index: subversion/libsvn_ra_neon/util.c === --- subversion/libsvn_ra_neon/util.c(revision 999102) +++ subversion/libsvn_ra_neon/util.c(working copy) @@ -167,6 +167,7 @@ typedef struct svn_ra_neon__request_t *req; svn_stringbuf_t *description; svn_boolean_t contains_error; + svn_boolean_t contains_precondition_error; } multistatus_baton_t; /* Implements svn_ra_neon__startelm_cb_t. */ @@ -231,6 +232,9 @@ end_207_element(void *baton, int state, return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL, _(The request response contained at least one error)); + else if (b-contains_precondition_error) +return svn_error_create(SVN_ERR_BAD_OLD_VALUE, NULL, +b-description-data); else return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
Re: svn client has inconsistent returnstatus for non-existing local files
Sjon Hortensius wrote on Thu, Sep 16, 2010 at 17:04:19 +0200: LS, Please advise if this is a bug, or a known feature. I have a local checkout on which I can execute svn commands. However, there seems to be some inconsistency in the exitcode of the client. `svn ls repository/non-existing` exits with status 1, and so does `svn info repository/non-existing`. However, `svn ls repository/non-existing` has exit-status 0, although it echo's a svn: warning: Using trunk, both 'ls' and 'info' return non-zero, for both URLs and paths under local working copies. (But you said 'svn ls' twice; was that a typo?) Is this a (known) bug? Can I post this issue on the bugtracker? If 'svn' reports an error that cannot be detected by a calling script, then please post an issue. Thanks, Sjon Hortensius
RE: [PATCH 2/3] atomic-revprop: Change tests
Hi, Daniel Shahaf wrote: Jon Foster wrote on Mon, Sep 20, 2010 at 12:39:31 +0100: Daniel Shahaf wrote: Jon Foster wrote on Mon, Sep 20, 2010 at 10:48:44 +0100: [[[ Change atomic-revprop tests to look at the error number rather than parsing the error text. So, this is trying to capture the current ra_dav situation, where the err-message is correct but err-apr_err isn't? Correct. Okay. Perhaps this could have been called out more explicitly in the log message --- i.e., have it say not only *what* the patch does, but also *why* it does that. Argh, I made one of the classic log-message-writing blunders! In other news, I've been thinking about moving the entire validation logic to the C helper; that is, to have it get an extra argv argument saying whether the revpropchange should pass or fail, and test by itself that it passed/failed as expected; and the Python tests would always expect it to exit(0). What do you think? Sounds like a good idea. Okay. I started a patch in that way at a time; I've touched it up now a tiny bit and I'm attaching it. Let me know what you think... As discussed on IRC... Index: subversion/tests/cmdline/atomic-ra-revprop-change.c === --- subversion/tests/cmdline/atomic-ra-revprop-change.c (revision 998887) +++ subversion/tests/cmdline/atomic-ra-revprop-change.c (working copy) [...] @@ -117,6 +122,8 @@ SVN_ERR(svn_config_create(servers, pool)); svn_config_set(servers, SVN_CONFIG_SECTION_GLOBAL, SVN_CONFIG_OPTION_HTTP_LIBRARY, http_library); + svn_config_set(servers, SVN_CONFIG_SECTION_GLOBAL, + SVN_CONFIG_OPTION_NEON_DEBUG_MASK, getenv(SVN_NEON_DEBUG_MASK) /* 131 */); /* Populate CONFIG. */ config = apr_hash_make(pool); If you drop that hunk (which looks totally unrelated) then: +1 (non-binding) Tested (both should pass and should fail) and it works. Kind regards, Jon ** This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd. If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone. Cabot Communications Limited Verona House, Filwood Road, Bristol BS16 3RY, UK +44 (0) 1179584232 Co. Registered in England number 02817269 Please contact the sender if you believe you have received this email in error. ** __ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email __
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Jon Foster wrote on Mon, Sep 20, 2010 at 22:59:02 +0100:
Daniel Shahaf wrote:
Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100:
However, there is a simpler way! The D:status element contains
the HTTP error code, usually 500 Internal Server Error. So we
could pick a special HTTP status code to mean SVN_ERR_BAD_OLD_VALUE.
I think of old_value_p as a precondition for the operation, a bit
like If-Modified-Since:, so I'd suggest 412 Precondition Failed.
Note that generic DAV clients won't ever see this message, because
they won't be sending old_value_p.
I'll commit this once I have an ra_serf version too. Would you
like to write the ra_serf part of the patch, too?
Sure. See attached.
This updated patch is against the atomic-revprop branch.
[[[
Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status
code inside a 207 multi-status response.
* subversion/mod_dav_svn/util.c:
No trailing colon here --^
(dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412.
* subversion/libsvn_ra_neon/util.c:
(multistatus_baton_t): New member contains_precondition_error.
(end_207_element): Check for HTTP 412 status code and create
a SVN_ERR_BAD_OLD_VALUE error if found.
* subversion/libsvn_ra_serf/ra_serf.h:
(svn_ra_serf__server_error_t): New member
contains_precondition_error.
* subversion/libsvn_ra_serf/util.c:
(parse_dav_status): New method.
(svn_ra_serf__handle_discard_body): Initialise new member.
(start_207): Parse DAV:status XML element.
(end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
error code if applicable.
(svn_ra_serf__handle_multistatus_only): Initialise new member.
Patch by: Jon Foster jon.fos...@cabot.co.uk
]]]
Kind regards,
Jon
Index: subversion/libsvn_ra_serf/util.c
===
--- subversion/libsvn_ra_serf/util.c (revision 999102)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t *r
{
server_err-error = svn_error_create(APR_SUCCESS, NULL, NULL);
server_err-has_xml_response = TRUE;
+ server_err-contains_precondition_error = FALSE;
server_err-cdata = svn_stringbuf_create(, pool);
server_err-collect_cdata = FALSE;
server_err-parser.pool = server_err-error-pool;
(So, this is the error handler for a function that normally discards its
response, and the meat of the patch is in
svn_ra_serf__handle_multistatus_only().)
@@ -945,6 +946,31 @@ svn_ra_serf__handle_status_only(serf_request_t *re
return svn_error_return(err);
}
+/* Given a string like HTTP/1.1 500 (status), parse out the numeric status
+ code. Ignores leading whitespace. This function will overwrite and then
+ clear the passed buf. */
Sounds like a strange semantics. Couldn't you just take a scratch_pool
argument and duplicate the buffer, leaving the caller's copy unchanged?
Changing it in-place seems like a shoot-oneself-in-the-foot move...
+static svn_error_t *
+parse_dav_status(svn_stringbuf_t *buf, int *status_code_out)
+{
+ svn_error_t * err;
+ const char * token;
+ char * tok_status = 0;
Style nit: no space after the * in the last three lines.
Also, no need to initialize TOK_STATUS (says svn_cstring_split_append()).
+
+ svn_stringbuf_strip_whitespace(buf);
+ token = apr_strtok(buf-data, \t\r\n, tok_status);
+ if (token)
+token = apr_strtok(NULL, \t\r\n, tok_status);
+ if (!token)
+return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
+Malformed DAV:status CDATA);
Should the cdata be included in the error text? (using svn_error_createf())
+ err = svn_cstring_atoi(status_code_out, token);
+ if (err)
+return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
+Malformed DAV:status CDATA);
+ svn_stringbuf_setempty(buf);
+ return SVN_NO_ERROR;
+}
+
/*
* Expat callback invoked on a start element tag for a 207 response.
*/
@@ -968,6 +994,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
svn_stringbuf_setempty(ctx-cdata);
ctx-collect_cdata = TRUE;
}
+ else if (ctx-in_error
+ strcmp(name.namespace, DAV:) == 0
+ strcmp(name.name, status) == 0)
+{
+ /* Start collecting cdata. */
+ svn_stringbuf_setempty(ctx-cdata);
Okay; D:responsedescription and D:status are siblings [1], so it's okay to
reuse CTX-cdata.
[1] http://paste.lisp.org/display/113346
paranoia on
Are you sure they will always be siblings? If we aren't sure that yes,
we could just use two stringbufs (instead of reusing ctx-cdata).
+ ctx-collect_cdata = TRUE;
+}
return SVN_NO_ERROR;
}
@@ -993,9 +1027,24 @@ end_207(svn_ra_serf__xml_parser_t *parser,
Re: [PATCH 2/3] atomic-revprop: Change tests
Thanks for the review. As discussed on IRC, I'll proceed as follows: 1. Rename the error codes (per Bert and Blair) 2. Commit your patch 3/3 about using 412 to preserve err-apr_err over DAV 3. Commit my want_error patch (which will pass thanks to #2) I think after this there will be only 1-2 items left in BRANCH-README.
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Hi,
Updated patch attached.
[[[
Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status
code inside a 207 multi-status response.
* subversion/mod_dav_svn/util.c
(dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412.
* subversion/libsvn_ra_neon/util.c
(multistatus_baton_t): New member contains_precondition_error.
(end_207_element): Check for HTTP 412 status code and create
a SVN_ERR_BAD_OLD_VALUE error if found.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__server_error_t): New member
contains_precondition_error.
* subversion/libsvn_ra_serf/util.c
(parse_dav_status): New method.
(start_207): Parse DAV:status XML element.
(end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
error code if applicable.
(svn_ra_serf__handle_multistatus_only): Initialise new member.
Patch by: Jon Foster jon.fos...@cabot.co.uk
]]]
Daniel Shahaf wrote:
[[[
Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status
code inside a 207 multi-status response.
* subversion/mod_dav_svn/util.c:
No trailing colon here --^
Fixed.
[...]
Index: subversion/libsvn_ra_serf/util.c
===
--- subversion/libsvn_ra_serf/util.c(revision 999102)
+++ subversion/libsvn_ra_serf/util.c(working copy)
@@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t
*r
+ server_err-contains_precondition_error = FALSE;
(So, this is the error handler for a function that normally discards
its response, and the meat of the patch is in
svn_ra_serf__handle_multistatus_only().)
After reviewing this code again, I've dropped that chunk. I've
convinced
myself that the variable is never used in that code path.
@@ -945,6 +946,31 @@ svn_ra_serf__handle_status_only(serf_request_t
*re
return svn_error_return(err);
}
+/* Given a string like HTTP/1.1 500 (status), parse out the
numeric status
+ code. Ignores leading whitespace. This function will overwrite
and then
+ clear the passed buf. */
Sounds like a strange semantics. Couldn't you just take a
scratch_pool
argument and duplicate the buffer, leaving the caller's copy
unchanged?
Changing it in-place seems like a shoot-oneself-in-the-foot move...
Changed.
+static svn_error_t *
+parse_dav_status(svn_stringbuf_t *buf, int *status_code_out)
+{
+ svn_error_t * err;
+ const char * token;
+ char * tok_status = 0;
Style nit: no space after the * in the last three lines.
Also, no need to initialize TOK_STATUS (says
svn_cstring_split_append()).
Both done.
+
+ svn_stringbuf_strip_whitespace(buf);
+ token = apr_strtok(buf-data, \t\r\n, tok_status);
+ if (token)
+token = apr_strtok(NULL, \t\r\n, tok_status);
+ if (!token)
+return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
+Malformed DAV:status CDATA);
Should the cdata be included in the error text? (using
svn_error_createf())
Can do, and in the new patch I have.
+ err = svn_cstring_atoi(status_code_out, token);
+ if (err)
+return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
+Malformed DAV:status CDATA);
+ svn_stringbuf_setempty(buf);
+ return SVN_NO_ERROR;
+}
+
/*
* Expat callback invoked on a start element tag for a 207
response.
*/
@@ -968,6 +994,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
svn_stringbuf_setempty(ctx-cdata);
ctx-collect_cdata = TRUE;
}
+ else if (ctx-in_error
+ strcmp(name.namespace, DAV:) == 0
+ strcmp(name.name, status) == 0)
+{
+ /* Start collecting cdata. */
+ svn_stringbuf_setempty(ctx-cdata);
Okay; D:responsedescription and D:status are siblings [1], so it's
okay to reuse CTX-cdata.
[1] http://paste.lisp.org/display/113346
paranoia on
Are you sure they will always be siblings? If we aren't sure that
yes,
we could just use two stringbufs (instead of reusing ctx-cdata).
Even with the old code, if there are any elements with CDATA nested
inside D:responsedescription, Serf is going to go wrong. If we
collect CDATA into different variables, then it'll just go wrong in
a different way (e.g. the HTTP status line might be shown as part of
the message).
I think there's a whole package of work that could be done to make
Serf resilient against weird XML, but I think that's separate from
this work. (It's also quite hard to test).
+ ctx-collect_cdata = TRUE;
[...]
+ SVN_ERR(parse_dav_status(ctx-cdata, status_code));
Our convention is to have the output parameters first.
Changed.
Thanks for the thourough review!
Jon
**
This email and its attachments may be confidential and are intended solely for
the use of the individual to whom it is addressed. Any views
RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Hi,
The error code changed while I was e-mailing the patch. Here's the
patch with the new error code.
[[[
Signal SVN_ERR_FS_PROP_BASEVALUE_MISMATCH over DAV using a HTTP 412
status
code inside a 207 multi-status response.
* subversion/mod_dav_svn/util.c
(dav_svn__convert_err): Map SVN_ERR_FS_PROP_BASEVALUE_MISMATCH to 412.
* subversion/libsvn_ra_neon/util.c
(multistatus_baton_t): New member contains_precondition_error.
(end_207_element): Check for HTTP 412 status code and create
a SVN_ERR_FS_PROP_BASEVALUE_MISMATCH error if
found.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__server_error_t): New member
contains_precondition_error.
* subversion/libsvn_ra_serf/util.c
(parse_dav_status): New method.
(start_207): Parse DAV:status XML element.
(end_207): Parse DAV:status XML element. Use
SVN_ERR_FS_PROP_BASEVALUE_MISMATCH error code if
applicable.
(svn_ra_serf__handle_multistatus_only): Initialise new member.
Patch by: Jon Foster jon.fos...@cabot.co.uk
]]]
Kind regards,
Jon
**
This email and its attachments may be confidential and are intended solely for
the use of the individual to whom it is addressed. Any views or opinions
expressed are solely those of the author and do not necessarily represent those
of Cabot Communications Ltd.
If you are not the intended recipient of this email and its attachments, you
must take no action based upon them, nor must you copy or show them to anyone.
Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232
Co. Registered in England number 02817269
Please contact the sender if you believe you have received this email in error.
**
__
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
__Index: subversion/mod_dav_svn/util.c
===
--- subversion/mod_dav_svn/util.c (revision 999161)
+++ subversion/mod_dav_svn/util.c (working copy)
@@ -107,6 +107,9 @@ dav_svn__convert_err(svn_error_t *serr,
case SVN_ERR_FS_PATH_ALREADY_LOCKED:
status = HTTP_LOCKED;
break;
+ case SVN_ERR_FS_PROP_BASEVALUE_MISMATCH:
+status = HTTP_PRECONDITION_FAILED;
+break;
/* add other mappings here */
}
Index: subversion/libsvn_ra_neon/util.c
===
--- subversion/libsvn_ra_neon/util.c(revision 999161)
+++ subversion/libsvn_ra_neon/util.c(working copy)
@@ -167,6 +167,7 @@ typedef struct
svn_ra_neon__request_t *req;
svn_stringbuf_t *description;
svn_boolean_t contains_error;
+ svn_boolean_t contains_precondition_error;
} multistatus_baton_t;
/* Implements svn_ra_neon__startelm_cb_t. */
@@ -231,6 +232,9 @@ end_207_element(void *baton, int state,
return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
_(The request response contained at least
one error));
+ else if (b-contains_precondition_error)
+return svn_error_create(SVN_ERR_FS_PROP_BASEVALUE_MISMATCH, NULL,
+b-description-data);
else
return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
b-description-data);
@@ -260,6 +264,10 @@ end_207_element(void *baton, int state,
else
b-propstat_has_error = (status.klass != 2);
+/* Handle 412 Precondition Failed specially */
+if (status.code == 412)
+ b-contains_precondition_error = TRUE;
+
free(status.reason_phrase);
}
else
Index: subversion/libsvn_ra_serf/util.c
===
--- subversion/libsvn_ra_serf/util.c(revision 999161)
+++ subversion/libsvn_ra_serf/util.c(working copy)
@@ -945,6 +945,34 @@ svn_ra_serf__handle_status_only(serf_request_t *re
return svn_error_return(err);
}
+/* Given a string like HTTP/1.1 500 (status), parse out the numeric status
+ code. Ignores leading whitespace. */
+static svn_error_t *
+parse_dav_status(int *status_code_out, svn_stringbuf_t *buf,
+ apr_pool_t *scratch_pool)
+{
+ svn_error_t *err;
+ const char *token;
+ char *tok_status;
+ svn_stringbuf_t *temp_buf = svn_stringbuf_create(buf-data, scratch_pool);
+
+ svn_stringbuf_strip_whitespace(temp_buf);
+ token = apr_strtok(temp_buf-data, \t\r\n, tok_status);
+ if (token)
+token = apr_strtok(NULL, \t\r\n, tok_status);
+ if
Re: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code
Jon Foster wrote on Tue, Sep 21, 2010 at 00:43:13 +0100:
Hi,
Updated patch attached.
[[[
Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status
code inside a 207 multi-status response.
* subversion/mod_dav_svn/util.c
(dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412.
* subversion/libsvn_ra_neon/util.c
(multistatus_baton_t): New member contains_precondition_error.
(end_207_element): Check for HTTP 412 status code and create
a SVN_ERR_BAD_OLD_VALUE error if found.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__server_error_t): New member
contains_precondition_error.
* subversion/libsvn_ra_serf/util.c
(parse_dav_status): New method.
(start_207): Parse DAV:status XML element.
(end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
error code if applicable.
(svn_ra_serf__handle_multistatus_only): Initialise new member.
Patch by: Jon Foster jon.fos...@cabot.co.uk
]]]
Index: subversion/libsvn_ra_serf/util.c
===
--- subversion/libsvn_ra_serf/util.c (revision 999102)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t
*r
+ server_err-contains_precondition_error = FALSE;
(So, this is the error handler for a function that normally discards
its response, and the meat of the patch is in
svn_ra_serf__handle_multistatus_only().)
After reviewing this code again, I've dropped that chunk. I've
convinced
myself that the variable is never used in that code path.
True.
Though I'm tempted to leave it in anyway; the next person to add code to
initialize server_err might copy this initalizer...
But I speculate that the thing is allocated with apr_pCalloc() anyway,
so this whole discussion doesn't matter :-)
+ err = svn_cstring_atoi(status_code_out, token);
+ if (err)
+return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
+Malformed DAV:status CDATA);
+ svn_stringbuf_setempty(buf);
+ return SVN_NO_ERROR;
+}
+
/*
* Expat callback invoked on a start element tag for a 207
response.
*/
@@ -968,6 +994,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
svn_stringbuf_setempty(ctx-cdata);
ctx-collect_cdata = TRUE;
}
+ else if (ctx-in_error
+ strcmp(name.namespace, DAV:) == 0
+ strcmp(name.name, status) == 0)
+{
+ /* Start collecting cdata. */
+ svn_stringbuf_setempty(ctx-cdata);
Okay; D:responsedescription and D:status are siblings [1], so it's
okay to reuse CTX-cdata.
paranoia on
Are you sure they will always be siblings? If we aren't sure that
yes,
we could just use two stringbufs (instead of reusing ctx-cdata).
Even with the old code, if there are any elements with CDATA nested
inside D:responsedescription, Serf is going to go wrong. If we
Ah, right.
collect CDATA into different variables, then it'll just go wrong in
a different way (e.g. the HTTP status line might be shown as part of
the message).
I think there's a whole package of work that could be done to make
Serf resilient against weird XML, but I think that's separate from
this work. (It's also quite hard to test).
Agreed.
Thanks for the thourough review!
You're welcome.
Content-Description: patch_atomic_revprops_dav2.txt
Index: subversion/libsvn_ra_serf/util.c
===
--- subversion/libsvn_ra_serf/util.c (revision 999156)
+++ subversion/libsvn_ra_serf/util.c (working copy)
@@ -945,6 +945,34 @@ svn_ra_serf__handle_status_only(serf_request_t *re
return svn_error_return(err);
}
+/* Given a string like HTTP/1.1 500 (status), parse out the numeric status
+ code. Ignores leading whitespace. */
Good docstring, but it will be slightly clearer (and extensible) if you
called out the parameter names:
+/* Given a string like HTTP/1.1 500 (status) in BUF, parse out the numeric
status
+ code into *STATUS_CODE_OUT. Ignores leading whitespace. */
(but yeah, I'm being lazy and dropping the Use POOL for temporary
allocations boilerplate)
+static svn_error_t *
+parse_dav_status(int *status_code_out, svn_stringbuf_t *buf,
+ apr_pool_t *scratch_pool)
+{
+ svn_error_t *err;
+ const char *token;
+ char *tok_status;
+ svn_stringbuf_t *temp_buf = svn_stringbuf_create(buf-data, scratch_pool);
*cough* svn_stringbuf_dup()
+
+ svn_stringbuf_strip_whitespace(temp_buf);
+ token = apr_strtok(temp_buf-data, \t\r\n, tok_status);
+ if (token)
+token = apr_strtok(NULL, \t\r\n, tok_status);
+ if (!token)
+return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
+ Malformed DAV:status CDATA '%s',
+
[PATCH 2/3],[PATCH 3/3] atomic-revprop: status
Daniel Shahaf wrote on Tue, Sep 21, 2010 at 01:19:06 +0200:
Thanks for the review.
As discussed on IRC, I'll proceed as follows:
1. Rename the error codes (per Bert and Blair)
Done.
2. Commit your patch 3/3 about using 412 to preserve err-apr_err over DAV
Running tests on the attached patch_atomic_revprops_dav3.txt.tweaked-by-me.
3. Commit my want_error patch (which will pass thanks to #2)
I'm satisfied with the attached crp-want_error-v3.diff on top of the
above patch.
I think after this there will be only 1-2 items left in BRANCH-README.
Once I commit those patches, I'll update STATUS too and we'll see what's left.
Index: subversion/mod_dav_svn/util.c
===
--- subversion/mod_dav_svn/util.c (revision 999170)
+++ subversion/mod_dav_svn/util.c (working copy)
@@ -107,6 +107,9 @@ dav_svn__convert_err(svn_error_t *serr,
case SVN_ERR_FS_PATH_ALREADY_LOCKED:
status = HTTP_LOCKED;
break;
+ case SVN_ERR_FS_PROP_BASEVALUE_MISMATCH:
+status = HTTP_PRECONDITION_FAILED;
+break;
/* add other mappings here */
}
Index: subversion/libsvn_ra_neon/util.c
===
--- subversion/libsvn_ra_neon/util.c(revision 999170)
+++ subversion/libsvn_ra_neon/util.c(working copy)
@@ -167,6 +167,7 @@ typedef struct
svn_ra_neon__request_t *req;
svn_stringbuf_t *description;
svn_boolean_t contains_error;
+ svn_boolean_t contains_precondition_error;
} multistatus_baton_t;
/* Implements svn_ra_neon__startelm_cb_t. */
@@ -231,6 +232,9 @@ end_207_element(void *baton, int state,
return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
_(The request response contained at least
one error));
+ else if (b-contains_precondition_error)
+return svn_error_create(SVN_ERR_FS_PROP_BASEVALUE_MISMATCH, NULL,
+b-description-data);
else
return svn_error_create(SVN_ERR_RA_DAV_REQUEST_FAILED, NULL,
b-description-data);
@@ -260,6 +264,10 @@ end_207_element(void *baton, int state,
else
b-propstat_has_error = (status.klass != 2);
+/* Handle 412 Precondition Failed specially */
+if (status.code == 412)
+ b-contains_precondition_error = TRUE;
+
free(status.reason_phrase);
}
else
Index: subversion/libsvn_ra_serf/util.c
===
--- subversion/libsvn_ra_serf/util.c(revision 999170)
+++ subversion/libsvn_ra_serf/util.c(working copy)
@@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t *r
{
server_err-error = svn_error_create(APR_SUCCESS, NULL, NULL);
server_err-has_xml_response = TRUE;
+ server_err-contains_precondition_error = FALSE;
server_err-cdata = svn_stringbuf_create(, pool);
server_err-collect_cdata = FALSE;
server_err-parser.pool = server_err-error-pool;
@@ -945,6 +946,34 @@ svn_ra_serf__handle_status_only(serf_request_t *re
return svn_error_return(err);
}
+/* Given a string like HTTP/1.1 500 (status) in BUF, parse out the numeric
+ status code into *STATUS_CODE_OUT. Ignores leading whitespace. */
+static svn_error_t *
+parse_dav_status(int *status_code_out, svn_stringbuf_t *buf,
+ apr_pool_t *scratch_pool)
+{
+ svn_error_t *err;
+ const char *token;
+ char *tok_status;
+ svn_stringbuf_t *temp_buf = svn_stringbuf_dup(buf, scratch_pool);
+
+ svn_stringbuf_strip_whitespace(temp_buf);
+ token = apr_strtok(temp_buf-data, \t\r\n, tok_status);
+ if (token)
+token = apr_strtok(NULL, \t\r\n, tok_status);
+ if (!token)
+return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
+ Malformed DAV:status CDATA '%s',
+ buf-data);
+ err = svn_cstring_atoi(status_code_out, token);
+ if (err)
+return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
+ Malformed DAV:status CDATA '%s',
+ buf-data);
+
+ return SVN_NO_ERROR;
+}
+
/*
* Expat callback invoked on a start element tag for a 207 response.
*/
@@ -968,6 +997,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
svn_stringbuf_setempty(ctx-cdata);
ctx-collect_cdata = TRUE;
}
+ else if (ctx-in_error
+ strcmp(name.namespace, DAV:) == 0
+ strcmp(name.name, status) == 0)
+{
+ /* Start collecting cdata. */
+ svn_stringbuf_setempty(ctx-cdata);
+ ctx-collect_cdata = TRUE;
+}
return SVN_NO_ERROR;
}
@@ -993,9 +1030,24 @@ end_207(svn_ra_serf__xml_parser_t *parser,
Re: [PATCH 2/3],[PATCH 3/3] atomic-revprop: status
Done in r999179, r999182, r999184 on the branch.
