You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2012/04/23 15:53:01 UTC
RE: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c
> -----Original Message-----
> From: hwright@apache.org [mailto:hwright@apache.org]
> Sent: maandag 23 april 2012 15:44
> To: commits@subversion.apache.org
> Subject: svn commit: r1329234 - in /subversion/trunk: ./
> subversion/libsvn_delta/compat.c
>
> Author: hwright
> Date: Mon Apr 23 13:43:33 2012
> New Revision: 1329234
>
> URL: http://svn.apache.org/viewvc?rev=1329234&view=rev
> Log:
> Merge r1325914 from the ev2-export branch.
>
> Modified:
> subversion/trunk/ (props changed)
> subversion/trunk/subversion/libsvn_delta/compat.c
>
> Propchange: subversion/trunk/
> ------------------------------------------------------------------------------
> Merged /subversion/branches/ev2-export:r1325914
>
> Modified: subversion/trunk/subversion/libsvn_delta/compat.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_delta/compa
> t.c?rev=1329234&r1=1329233&r2=1329234&view=diff
> =================================================================
> =============
> --- subversion/trunk/subversion/libsvn_delta/compat.c (original)
> +++ subversion/trunk/subversion/libsvn_delta/compat.c Mon Apr 23 13:43:33
> 2012
> @@ -1088,6 +1088,11 @@ svn_delta__delta_from_editor(const svn_d
> };
> struct ev2_edit_baton *eb = apr_pcalloc(pool, sizeof(*eb));
>
> + if (!base_relpath)
> + base_relpath = "";
> + else if (base_relpath[0] == '/')
> + base_relpath += 1;
> +
Why do you allow invalid relpaths?
I think it is the callers task to pass valid relpaths, so you could just use assert(svn_relpath_is_canonical(base_relpath)); for a debug only check. (or SVN_ERR_ASSERT() if you also want to slow down release versions)
Bert
Re: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c
Posted by Greg Stein <gs...@gmail.com>.
On Apr 23, 2012 10:00 AM, "Hyrum K Wright" <hy...@wandisco.com>
wrote:
>...
> As an aside, I thought SVN_ERR_ASSERT() was compiled out of production
> builds. Perhaps not?
Nope. They remain.
Re: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Mon, Apr 23, 2012 at 8:53 AM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: hwright@apache.org [mailto:hwright@apache.org]
>> Sent: maandag 23 april 2012 15:44
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1329234 - in /subversion/trunk: ./
>> subversion/libsvn_delta/compat.c
>>
>> Author: hwright
>> Date: Mon Apr 23 13:43:33 2012
>> New Revision: 1329234
>>
>> URL: http://svn.apache.org/viewvc?rev=1329234&view=rev
>> Log:
>> Merge r1325914 from the ev2-export branch.
>>
>> Modified:
>> subversion/trunk/ (props changed)
>> subversion/trunk/subversion/libsvn_delta/compat.c
>>
>> Propchange: subversion/trunk/
>> ------------------------------------------------------------------------------
>> Merged /subversion/branches/ev2-export:r1325914
>>
>> Modified: subversion/trunk/subversion/libsvn_delta/compat.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_delta/compa
>> t.c?rev=1329234&r1=1329233&r2=1329234&view=diff
>> =================================================================
>> =============
>> --- subversion/trunk/subversion/libsvn_delta/compat.c (original)
>> +++ subversion/trunk/subversion/libsvn_delta/compat.c Mon Apr 23 13:43:33
>> 2012
>> @@ -1088,6 +1088,11 @@ svn_delta__delta_from_editor(const svn_d
>> };
>> struct ev2_edit_baton *eb = apr_pcalloc(pool, sizeof(*eb));
>>
>> + if (!base_relpath)
>> + base_relpath = "";
>> + else if (base_relpath[0] == '/')
>> + base_relpath += 1;
>> +
>
> Why do you allow invalid relpaths?
>
> I think it is the callers task to pass valid relpaths, so you could just use assert(svn_relpath_is_canonical(base_relpath)); for a debug only check. (or SVN_ERR_ASSERT() if you also want to slow down release versions)
Some callers don't have a relpath to pass, which is the reason for the
first check. I think you are correct, though that we can modify the
code to require callers provide a valid relpath, if they provide one
at all. It might take me a while to get there, though.
As an aside, I thought SVN_ERR_ASSERT() was compiled out of production
builds. Perhaps not?
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
RE: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Julian Foad [mailto:julianfoad@btopenworld.com]
> Sent: maandag 23 april 2012 18:02
> To: Bert Huijben
> Cc: dev@subversion.apache.org
> Subject: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in
> /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
>
> Bert Huijben wrote:
>
> > you could just use
> > assert(svn_relpath_is_canonical(base_relpath)); for a debug only check.
(or
> > SVN_ERR_ASSERT() if you also want to slow down release versions)
>
> The policy is: always use 'SVN_ERR_ASSERT' rather than 'assert' (in
functions
> that return svn_error_t).
No, we also use assert in several places, e.g. in all path processing
functions.
assert() isn't evaluated when NDEBUG is specified. In that case the argument
is ignored.
SVN_ERR_ASSERT() and SVN_ERR_ASSERT_NO_RETURN() *are* evaluated in release
builds.
Bert
>
> Of course he doesn't "want to slow down" Subversion.
>
> The choice between 'assert' and 'SVN_ERR_ASSERT' should be based on
whether
> we want an application program to be able to catch such a failure. We
long ago
> decided that the answer is YES we do want to write our library functions
in such
> a way that an application can catch an assertion failure if its author
chooses to
> do so. SVN_ERR_ASSERT was introduces to fulfil that need.
>
> There isn't currently an easy build switch (such as NDEBUG) to disable
> SVN_ERR_ASSERT completely at compile time. That's just a side issue. If
you
> want such a switch, just ask; we can easily create one. Or if you think
we need
> two levels of assertions -- one for quick tests and another for slow tests
-- and
> want to be able to compile-out the slow ones independently of the quick
ones,
> just ask. But implying we should use 'assert' for slow tests and
> 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
>
> - Julian
Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in
/subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Daniel Shahaf <da...@elego.de>.
Julian Foad wrote on Mon, Apr 23, 2012 at 21:11:30 +0100:
>
>
>
>
> ----- Original Message -----
> > From: Daniel Shahaf <da...@elego.de>
> > To: Julian Foad <ju...@btopenworld.com>
> > Cc: Bert Huijben <be...@qqmail.nl>; "dev@subversion.apache.org" <de...@subversion.apache.org>
> > Sent: Monday, 23 April 2012, 20:59
> > Subject: Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
> >
> > Julian Foad wrote on Mon, Apr 23, 2012 at 20:40:59 +0100:
> >> Daniel Shahaf wrote:
> >>
> >> > Julian Foad wrote:
> >> >> I (Julian Foad) wrote:
> >> >> > There isn't currently an easy build switch (such as
> > NDEBUG) to disable
> >> >> > SVN_ERR_ASSERT completely at compile time. That's just
> > a side issue. If
> >> >> > you want such a switch, just ask; we can easily create one.
> > Or if you think we
> >> >> > need two levels of assertions -- one for quick tests and
> > another for slow tests
> >> >> > -- and want to be able to compile-out the slow ones
> > independently of the quick
> >> >> > ones, just ask. But implying we should use 'assert'
> > for slow tests and
> >> >> > 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
> >> >>
> >> >> We can also introduce run-time control of whether the conditions
> > are
> >> >> evaluated: test a global 'assertions enabled?' variable or
> > function
> >> >> before evaluating the condition. For example:
> >> [...]
> >> > That doesn't sound right. Surely we don't want to allow
> > disabling _all_
> >> > uses of SVN_ERR_ASSERT() this way? (Remember that some of them
> >> > translate to segfaults (possibly corruptions?) if the condition
> > doesn't
> >> > hold)
> >>
> >> Hi Daniel.
> >>
> >> In places where there will be a seg-fault if the condition is false,
> >> the assertion statement doesn't prevent abnormal program termination,
> >> it only makes it easier to see what went wrong.
> >>
> >
> > SVN_ERR_ASSERT() prevents an abnormal termination when a non-default
> > malfunction handler has been installed.
>
> Potentially, but the app can't tell how badly the library data has
> gone wrong, so may want to terminate the program anyway after
> informing the user. (Quoting an email (from Steve King?) within the
> past year or two.)
>
Yes. We don't distinguish various kinds of asserts, so the app needs to
assume the worst. But it's a cleaner termination than SIGABRT.
> >> In places where the processing will continue with wrong data or wrong
> >> behaviour if the condition is false, the assertion statement doesn't
> >> prevent the program from going wrong, it just changes the failure mode
> >> to a more obvious one.
> >>
> >> People who don't care about the failure mode in such cases may wish to
> >> turn off the checks.
> >
> > Depends, I think. If libsvn_ra asserts that a log message is in UTF-8,
> > it is reasonable to want to disable that since the only harm resulting
> > is an svn_error_create() on the server.
>
> How do you know it won't crash?
>
The requirement for UTF-8 is at the repos layer, so at least ra_local
and ra_svn should cope just fine with arbitrary binary data in there.
I don't recall whether that's true for ra_dav too.
But I think that whether or not libsvn_ra* segfault on non-UTF-8 log
message wasn't your point.
> > But if libsvn_fs asserts the
> > sanity of a revision before committing it, I think most people will
> > prefer to have the commit attempt fail.
> >
> > (The latter example is not realistic since, at least in FSFS, we
> > generally return an svn_error_t rather than an assertion when a sanity
> > check fails.)
>
> - Julian
Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Julian Foad <ju...@btopenworld.com>.
----- Original Message -----
> From: Daniel Shahaf <da...@elego.de>
> To: Julian Foad <ju...@btopenworld.com>
> Cc: Bert Huijben <be...@qqmail.nl>; "dev@subversion.apache.org" <de...@subversion.apache.org>
> Sent: Monday, 23 April 2012, 20:59
> Subject: Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
>
> Julian Foad wrote on Mon, Apr 23, 2012 at 20:40:59 +0100:
>> Daniel Shahaf wrote:
>>
>> > Julian Foad wrote:
>> >> I (Julian Foad) wrote:
>> >> > There isn't currently an easy build switch (such as
> NDEBUG) to disable
>> >> > SVN_ERR_ASSERT completely at compile time. That's just
> a side issue. If
>> >> > you want such a switch, just ask; we can easily create one.
> Or if you think we
>> >> > need two levels of assertions -- one for quick tests and
> another for slow tests
>> >> > -- and want to be able to compile-out the slow ones
> independently of the quick
>> >> > ones, just ask. But implying we should use 'assert'
> for slow tests and
>> >> > 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
>> >>
>> >> We can also introduce run-time control of whether the conditions
> are
>> >> evaluated: test a global 'assertions enabled?' variable or
> function
>> >> before evaluating the condition. For example:
>> [...]
>> > That doesn't sound right. Surely we don't want to allow
> disabling _all_
>> > uses of SVN_ERR_ASSERT() this way? (Remember that some of them
>> > translate to segfaults (possibly corruptions?) if the condition
> doesn't
>> > hold)
>>
>> Hi Daniel.
>>
>> In places where there will be a seg-fault if the condition is false,
>> the assertion statement doesn't prevent abnormal program termination,
>> it only makes it easier to see what went wrong.
>>
>
> SVN_ERR_ASSERT() prevents an abnormal termination when a non-default
> malfunction handler has been installed.
Potentially, but the app can't tell how badly the library data has gone wrong, so may want to terminate the program anyway after informing the user. (Quoting an email (from Steve King?) within the past year or two.)
>> In places where the processing will continue with wrong data or wrong
>> behaviour if the condition is false, the assertion statement doesn't
>> prevent the program from going wrong, it just changes the failure mode
>> to a more obvious one.
>>
>> People who don't care about the failure mode in such cases may wish to
>> turn off the checks.
>
> Depends, I think. If libsvn_ra asserts that a log message is in UTF-8,
> it is reasonable to want to disable that since the only harm resulting
> is an svn_error_create() on the server.
How do you know it won't crash?
> But if libsvn_fs asserts the
> sanity of a revision before committing it, I think most people will
> prefer to have the commit attempt fail.
>
> (The latter example is not realistic since, at least in FSFS, we
> generally return an svn_error_t rather than an assertion when a sanity
> check fails.)
- Julian
Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in
/subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Daniel Shahaf <da...@elego.de>.
Julian Foad wrote on Mon, Apr 23, 2012 at 20:40:59 +0100:
> Daniel Shahaf wrote:
>
> > Julian Foad wrote:
> >> I (Julian Foad) wrote:
> >> > There isn't currently an easy build switch (such as NDEBUG) to disable
> >> > SVN_ERR_ASSERT completely at compile time. That's just a side issue. If
> >> > you want such a switch, just ask; we can easily create one. Or if you think we
> >> > need two levels of assertions -- one for quick tests and another for slow tests
> >> > -- and want to be able to compile-out the slow ones independently of the quick
> >> > ones, just ask. But implying we should use 'assert' for slow tests and
> >> > 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
> >>
> >> We can also introduce run-time control of whether the conditions are
> >> evaluated: test a global 'assertions enabled?' variable or function
> >> before evaluating the condition. For example:
> [...]
> > That doesn't sound right. Surely we don't want to allow disabling _all_
> > uses of SVN_ERR_ASSERT() this way? (Remember that some of them
> > translate to segfaults (possibly corruptions?) if the condition doesn't
> > hold)
>
> Hi Daniel.
>
> In places where there will be a seg-fault if the condition is false,
> the assertion statement doesn't prevent abnormal program termination,
> it only makes it easier to see what went wrong.
>
SVN_ERR_ASSERT() prevents an abnormal termination when a non-default
malfunction handler has been installed.
> In places where the processing will continue with wrong data or wrong
> behaviour if the condition is false, the assertion statement doesn't
> prevent the program from going wrong, it just changes the failure mode
> to a more obvious one.
>
> People who don't care about the failure mode in such cases may wish to
> turn off the checks.
>
Depends, I think. If libsvn_ra asserts that a log message is in UTF-8,
it is reasonable to want to disable that since the only harm resulting
is an svn_error_create() on the server. But if libsvn_fs asserts the
sanity of a revision before committing it, I think most people will
prefer to have the commit attempt fail.
(The latter example is not realistic since, at least in FSFS, we
generally return an svn_error_t rather than an assertion when a sanity
check fails.)
> - Julian
Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> Julian Foad wrote:
>> I (Julian Foad) wrote:
>> > There isn't currently an easy build switch (such as NDEBUG) to disable
>> > SVN_ERR_ASSERT completely at compile time. That's just a side issue. If
>> > you want such a switch, just ask; we can easily create one. Or if you think we
>> > need two levels of assertions -- one for quick tests and another for slow tests
>> > -- and want to be able to compile-out the slow ones independently of the quick
>> > ones, just ask. But implying we should use 'assert' for slow tests and
>> > 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
>>
>> We can also introduce run-time control of whether the conditions are
>> evaluated: test a global 'assertions enabled?' variable or function
>> before evaluating the condition. For example:
[...]
> That doesn't sound right. Surely we don't want to allow disabling _all_
> uses of SVN_ERR_ASSERT() this way? (Remember that some of them
> translate to segfaults (possibly corruptions?) if the condition doesn't
> hold)
Hi Daniel.
In places where there will be a seg-fault if the condition is false, the assertion statement doesn't prevent abnormal program termination, it only makes it easier to see what went wrong.
In places where the processing will continue with wrong data or wrong behaviour if the condition is false, the assertion statement doesn't prevent the program from going wrong, it just changes the failure mode to a more obvious one.
People who don't care about the failure mode in such cases may wish to turn off the checks.
- Julian
Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in
/subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Daniel Shahaf <da...@elego.de>.
Julian Foad wrote on Mon, Apr 23, 2012 at 17:18:22 +0100:
> I (Julian Foad) wrote:
>
> > There isn't currently an easy build switch (such as NDEBUG) to disable
> > SVN_ERR_ASSERT completely at compile time. That's just a side issue. If
> > you want such a switch, just ask; we can easily create one. Or if you think we
> > need two levels of assertions -- one for quick tests and another for slow tests
> > -- and want to be able to compile-out the slow ones independently of the quick
> > ones, just ask. But implying we should use 'assert' for slow tests and
> > 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
>
> We can also introduce run-time control of whether the conditions are evaluated: test a global 'assertions enabled?' variable or function before evaluating the condition. For example:
>
> Index: subversion/include/svn_error.h
> ===================================================================
> #define SVN_ERR_ASSERT(expr) \
> do { \
> - if (!(expr)) \
> + if (svn_error__assertions_enabled && !(expr)) \
> SVN_ERR(svn_error__malfunction(TRUE, __FILE__, __LINE__, #expr)); \
> } while (0)
>
> +/* ... */
> +extern svn_boolean_t svn_error__assertions_enabled;
>
That doesn't sound right. Surely we don't want to allow disabling _all_
uses of SVN_ERR_ASSERT() this way? (Remember that some of them
translate to segfaults (possibly corruptions?) if the condition doesn't
hold)
Perhaps we should have two flavours of the SVN_ERR_ASSERT() macros --
one unconditional and one only for developers.
Daniel
> - Julian
Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> There isn't currently an easy build switch (such as NDEBUG) to disable
> SVN_ERR_ASSERT completely at compile time. That's just a side issue. If
> you want such a switch, just ask; we can easily create one. Or if you think we
> need two levels of assertions -- one for quick tests and another for slow tests
> -- and want to be able to compile-out the slow ones independently of the quick
> ones, just ask. But implying we should use 'assert' for slow tests and
> 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
We can also introduce run-time control of whether the conditions are evaluated: test a global 'assertions enabled?' variable or function before evaluating the condition. For example:
Index: subversion/include/svn_error.h
===================================================================
#define SVN_ERR_ASSERT(expr) \
do { \
- if (!(expr)) \
+ if (svn_error__assertions_enabled && !(expr)) \
SVN_ERR(svn_error__malfunction(TRUE, __FILE__, __LINE__, #expr)); \
} while (0)
+/* ... */
+extern svn_boolean_t svn_error__assertions_enabled;
- Julian
Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in
/subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Mon, Apr 23, 2012 at 11:01 AM, Julian Foad
<ju...@btopenworld.com> wrote:
> Bert Huijben wrote:
>
>> you could just use
>> assert(svn_relpath_is_canonical(base_relpath)); for a debug only check. (or
>> SVN_ERR_ASSERT() if you also want to slow down release versions)
>
> The policy is: always use 'SVN_ERR_ASSERT' rather than 'assert' (in functions that return svn_error_t).
>
> Of course he doesn't "want to slow down" Subversion.
>
> The choice between 'assert' and 'SVN_ERR_ASSERT' should be based on whether we want an application program to be able to catch such a failure. We long ago decided that the answer is YES we do want to write our library functions in such a way that an application can catch an assertion failure if its author chooses to do so. SVN_ERR_ASSERT was introduces to fulfil that need.
>
> There isn't currently an easy build switch (such as NDEBUG) to disable SVN_ERR_ASSERT completely at compile time. That's just a side issue. If you want such a switch, just ask; we can easily create one. Or if you think we need two levels of assertions -- one for quick tests and another for slow tests -- and want to be able to compile-out the slow ones independently of the quick ones, just ask. But implying we should use 'assert' for slow tests and 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
For what it's worth, I often just define SVN_ERR_ASSERT() to assert()
so as to make running through the debugger a bit easier.
I think Bert's main argument was that our "heavy" (so-called)
assertion system is in place during release and production builds,
which could have negative performance implications in those systems.
It isn't a question of how fasts the tests run, but how fast our final
product runs.
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in
/subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Daniel Shahaf <da...@elego.de>.
Julian Foad wrote on Mon, Apr 23, 2012 at 17:01:33 +0100:
> Bert Huijben wrote:
>
> > you could just use
> > assert(svn_relpath_is_canonical(base_relpath)); for a debug only check. (or
> > SVN_ERR_ASSERT() if you also want to slow down release versions)
>
> The policy is: always use 'SVN_ERR_ASSERT' rather than 'assert' (in functions that return svn_error_t).
>
> Of course he doesn't "want to slow down" Subversion.
>
> The choice between 'assert' and 'SVN_ERR_ASSERT' should be based on whether we want an application program to be able to catch such a failure. We long ago decided that the answer is YES we do want to write our library functions in such a way that an application can catch an assertion failure if its author chooses to do so. SVN_ERR_ASSERT was introduces to fulfil that need.
>
> There isn't currently an easy build switch (such as NDEBUG) to disable
> SVN_ERR_ASSERT completely at compile time. That's just a side issue.
> If you want such a switch, just ask; we can easily create one.
Or we could do it on a case-by-case basis:
#ifndef NDEBUG
SVN_ERR_ASSERT(system("exit $(find / | wc -l)") >= INT_MIN);
#endif
Always use SVN_ERR_ASSERT [was: svn commit: r1329234 - in /subversion/trunk: ./ subversion/libsvn_delta/compat.c]
Posted by Julian Foad <ju...@btopenworld.com>.
Bert Huijben wrote:
> you could just use
> assert(svn_relpath_is_canonical(base_relpath)); for a debug only check. (or
> SVN_ERR_ASSERT() if you also want to slow down release versions)
The policy is: always use 'SVN_ERR_ASSERT' rather than 'assert' (in functions that return svn_error_t).
Of course he doesn't "want to slow down" Subversion.
The choice between 'assert' and 'SVN_ERR_ASSERT' should be based on whether we want an application program to be able to catch such a failure. We long ago decided that the answer is YES we do want to write our library functions in such a way that an application can catch an assertion failure if its author chooses to do so. SVN_ERR_ASSERT was introduces to fulfil that need.
There isn't currently an easy build switch (such as NDEBUG) to disable SVN_ERR_ASSERT completely at compile time. That's just a side issue. If you want such a switch, just ask; we can easily create one. Or if you think we need two levels of assertions -- one for quick tests and another for slow tests -- and want to be able to compile-out the slow ones independently of the quick ones, just ask. But implying we should use 'assert' for slow tests and 'SVN_ERR_ASSERT' for quick tests is the Wrong Way.
- Julian