You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2011/12/23 01:35:16 UTC
svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Author: stsp
Date: Fri Dec 23 00:35:15 2011
New Revision: 1222522
URL: http://svn.apache.org/viewvc?rev=1222522&view=rev
Log:
* STATUS: Suggest r1222521.
Modified:
subversion/branches/1.7.x/STATUS
Modified: subversion/branches/1.7.x/STATUS
URL: http://svn.apache.org/viewvc/subversion/branches/1.7.x/STATUS?rev=1222522&r1=1222521&r2=1222522&view=diff
==============================================================================
--- subversion/branches/1.7.x/STATUS (original)
+++ subversion/branches/1.7.x/STATUS Fri Dec 23 00:35:15 2011
@@ -166,6 +166,15 @@ Candidate changes:
Votes:
+1: cmpilato
+ * r1222521
+ Turn another wc-db assertion people are reporting into a normal error.
+ Justification:
+ An informative error message helps people more than an assert (which
+ in case of TSVN can mean crashing the Windows Explorer).
+ Reported several times on users@, e.g. here:
+ http://svn.haxx.se/users/archive-2011-12/0299.shtml
+ Votes:
+ +1: stsp
Veto-blocked changes:
=====================
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Dec 23, 2011 at 11:52:08PM +0100, Branko Čibej wrote:
> Ranting is all very well, but I've yet to hear a suggestion from you
> about how the libraries should handle unrecoverable errors. Like, for
> example, the case where wc.db contains inconsistent and/or invalid data.
It's already been pointed out that throwing a descriptive error message
at the user and aborting whatever operation is running is better than
asserting. The end result is the same -- the operation is aborted. But
an assert confuses users because it provides no useful information to them.
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@apache.org>.
On 24.12.2011 11:25, Branko Čibej wrote:
> On 24.12.2011 11:06, Daniel Shahaf wrote:
>> Branko Čibej wrote on Fri, Dec 23, 2011 at 23:52:08 +0100:
>>> Ranting is all very well, but I've yet to hear a suggestion from you
>>> about how the libraries should handle unrecoverable errors. Like, for
>>> example, the case where wc.db contains inconsistent and/or invalid data.
>> Distinguish "The internal state of the library is fubar'd" from "The
>> {wc,repos} is fubar'd", and use SVN_ERR_ASSERT() for the former and
>> normal errors for invalid on-disk data?
>>
>> (and, sure, if we detect a wc is corrupt then we block all further
>> operations on it)
> That's pretty much the definition of meeting TSVN halfway, isn't it?
>
And ... good luck in finding a way to reliably tell the difference
between data corruption and library fubar.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 24.12.2011 11:06, Daniel Shahaf wrote:
> Branko Čibej wrote on Fri, Dec 23, 2011 at 23:52:08 +0100:
>> Ranting is all very well, but I've yet to hear a suggestion from you
>> about how the libraries should handle unrecoverable errors. Like, for
>> example, the case where wc.db contains inconsistent and/or invalid data.
> Distinguish "The internal state of the library is fubar'd" from "The
> {wc,repos} is fubar'd", and use SVN_ERR_ASSERT() for the former and
> normal errors for invalid on-disk data?
>
> (and, sure, if we detect a wc is corrupt then we block all further
> operations on it)
That's pretty much the definition of meeting TSVN halfway, isn't it?
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Fri, Dec 23, 2011 at 23:52:08 +0100:
> Ranting is all very well, but I've yet to hear a suggestion from you
> about how the libraries should handle unrecoverable errors. Like, for
> example, the case where wc.db contains inconsistent and/or invalid data.
Distinguish "The internal state of the library is fubar'd" from "The
{wc,repos} is fubar'd", and use SVN_ERR_ASSERT() for the former and
normal errors for invalid on-disk data?
(and, sure, if we detect a wc is corrupt then we block all further
operations on it)
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
On 2011-12-25 06:58, Branko Čibej wrote:
> On 24.12.2011 18:32, Stefan Küng wrote:
>> On 24.12.2011 18:18, Branko Čibej wrote:
>>> On 24.12.2011 17:37, Daniel Shahaf wrote:
>>>> Stefan Küng wrote on Sat, Dec 24, 2011 at 17:24:51 +0100:
>>>>> Then why are there multiple statements like this in the svn code:
>>>>> SVN_ERR_ASSERT_NO_RETURN(svn_dirent_is_absolute(local_abspath));
>>>>> (example from libsvn_wc\util.c, line 197).
>>>> Because the containing function doesn't return svn_error_t.
>>>
>>> Which is exactly what I said earlier. As long as we have /any/ function
>>> that does not return an error code, unless we can prove that it cannot
>>> fail in a detectable manner, there's not even a theoretical chance of
>>> removing such assertions.
>>
>> But that function doesn't need an absolute path at all. Callers of
>> that function might, but those functions could most likely return an
>> error.
>>
>> and please take a look at the file libsvn_wc\props.c, search for
>> SVN_ERR_ASSERT_NO_RETURN.
>> I don't even know how to respond to something like this: calling
>> SVN_ERR_ASSERT_NO_RETURN() but in the very next line there's a
>> statement that could return an error, just that it never goes there
>> because SVN_ERR_ASSERT_NO_RETURN as the name implies never returns.
>>
>> I can't add anything more to code like this.
>
> Stefan, stop pointing fingers at implementation issues. You have commit
> access, so you can fix that kind of abuse of aborts yourself instead of
> blaming "subversion." Granted that these constructs are being abused in
> the code, just sitting back and yammering on the list and then being
> offended when you don't get immediate results isn't helping anyone.
>
> My point is that you are never going to get rid of aborts. You can
> certainly reduce their number, which is already happening. But even that
> is going to become increasingly difficult.
"increasingly difficult" is somehow understandable,
I (and I also guess Stefan) understands this immediately.
What I just object is declaring a bad practice/design
as good behavior/design just because it is difficult
to fix the bad practice everywhere, because this removes
the motivation to at least do it better in the future.
>
> It's perfectly OK to raise these issues on the dev@ list. It's not OK to
> imply that the volunteers who built Subversion are a bunch of
> incompetents just because you found some bugs in the code. It doesn't
> matter how long those bugs have been there.
>
> Don't make me go and review your TSVN code for the same kind of
> silliness. :)
>
> -- Brane
>
>
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@apache.org>.
On 24.12.2011 18:32, Stefan Küng wrote:
> On 24.12.2011 18:18, Branko Čibej wrote:
>> On 24.12.2011 17:37, Daniel Shahaf wrote:
>>> Stefan Küng wrote on Sat, Dec 24, 2011 at 17:24:51 +0100:
>>>> Then why are there multiple statements like this in the svn code:
>>>> SVN_ERR_ASSERT_NO_RETURN(svn_dirent_is_absolute(local_abspath));
>>>> (example from libsvn_wc\util.c, line 197).
>>> Because the containing function doesn't return svn_error_t.
>>
>> Which is exactly what I said earlier. As long as we have /any/ function
>> that does not return an error code, unless we can prove that it cannot
>> fail in a detectable manner, there's not even a theoretical chance of
>> removing such assertions.
>
> But that function doesn't need an absolute path at all. Callers of
> that function might, but those functions could most likely return an
> error.
>
> and please take a look at the file libsvn_wc\props.c, search for
> SVN_ERR_ASSERT_NO_RETURN.
> I don't even know how to respond to something like this: calling
> SVN_ERR_ASSERT_NO_RETURN() but in the very next line there's a
> statement that could return an error, just that it never goes there
> because SVN_ERR_ASSERT_NO_RETURN as the name implies never returns.
>
> I can't add anything more to code like this.
Stefan, stop pointing fingers at implementation issues. You have commit
access, so you can fix that kind of abuse of aborts yourself instead of
blaming "subversion." Granted that these constructs are being abused in
the code, just sitting back and yammering on the list and then being
offended when you don't get immediate results isn't helping anyone.
My point is that you are never going to get rid of aborts. You can
certainly reduce their number, which is already happening. But even that
is going to become increasingly difficult.
It's perfectly OK to raise these issues on the dev@ list. It's not OK to
imply that the volunteers who built Subversion are a bunch of
incompetents just because you found some bugs in the code. It doesn't
matter how long those bugs have been there.
Don't make me go and review your TSVN code for the same kind of
silliness. :)
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Küng <to...@gmail.com>.
On 24.12.2011 18:18, Branko Čibej wrote:
> On 24.12.2011 17:37, Daniel Shahaf wrote:
>> Stefan Küng wrote on Sat, Dec 24, 2011 at 17:24:51 +0100:
>>> Then why are there multiple statements like this in the svn code:
>>> SVN_ERR_ASSERT_NO_RETURN(svn_dirent_is_absolute(local_abspath));
>>> (example from libsvn_wc\util.c, line 197).
>> Because the containing function doesn't return svn_error_t.
>
> Which is exactly what I said earlier. As long as we have /any/ function
> that does not return an error code, unless we can prove that it cannot
> fail in a detectable manner, there's not even a theoretical chance of
> removing such assertions.
But that function doesn't need an absolute path at all. Callers of that
function might, but those functions could most likely return an error.
and please take a look at the file libsvn_wc\props.c, search for
SVN_ERR_ASSERT_NO_RETURN.
I don't even know how to respond to something like this: calling
SVN_ERR_ASSERT_NO_RETURN() but in the very next line there's a statement
that could return an error, just that it never goes there because
SVN_ERR_ASSERT_NO_RETURN as the name implies never returns.
I can't add anything more to code like this.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 24.12.2011 17:37, Daniel Shahaf wrote:
> Stefan Küng wrote on Sat, Dec 24, 2011 at 17:24:51 +0100:
>> Then why are there multiple statements like this in the svn code:
>> SVN_ERR_ASSERT_NO_RETURN(svn_dirent_is_absolute(local_abspath));
>> (example from libsvn_wc\util.c, line 197).
> Because the containing function doesn't return svn_error_t.
Which is exactly what I said earlier. As long as we have /any/ function
that does not return an error code, unless we can prove that it cannot
fail in a detectable manner, there's not even a theoretical chance of
removing such assertions.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Küng wrote on Sat, Dec 24, 2011 at 17:24:51 +0100:
> Then why are there multiple statements like this in the svn code:
> SVN_ERR_ASSERT_NO_RETURN(svn_dirent_is_absolute(local_abspath));
> (example from libsvn_wc\util.c, line 197).
Because the containing function doesn't return svn_error_t.
> That's just one example of many, many more. A simple search for
> SVN_ERR_ASSERT_NO_RETURN will give you an idea. I only found about
> two which I would consider real _NO_RETURN situations. All others
> are completely recoverable.
You have commit access.
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Mark Mielke <ma...@mark.mielke.cc>.
On 12/25/2011 04:08 AM, Daniel Shahaf wrote:
> Mark Mielke wrote on Sat, Dec 24, 2011 at 19:01:07 -0500:
>> All code has bugs, of course, but from what I'm reading Subversion
>> is doing something particular in that the developers are treating
>> assert() as a list of TODO items that are not properly handled and
>> really should be handled...
> We do not use asserts as a todo list.
Well you trimmed the part of my post that describes why I think this...
that is, if assert is raised in production, it probably means the code
wasn't properly tested in the first place.
assert() is a dilemma. If you know enough to write the assert, you can
probably write a little more to handle the situation in some way. Not
stop, but at least pass the error back through the stack in some sort of
reliable way.
The case where you don't know enough about how to handle the situation,
you probably didn't know to add an assert().
I don't want to really get into yet another philosophy debate on this -
as the Subversion developers definitely have their own feelings on these
matters - but I do want to point out that other that I don't think this
practice is typical. assert() doesn't make your code better. It just
lets you mark which parts of the software that the authors failed to
provide adequate error handling, and the choice being made is to give a
horrendous looking failure to users instead of putting in the effort to
make it fail in a smarter way.
Then this allows the statement that "I'd rather have it fail than
proceed to do it wrong." The best answer is to do it right. To fail or
do it it wrong are a rock and a hard place.
--
Mark Mielke<ma...@mielke.cc>
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Mark Mielke wrote on Sat, Dec 24, 2011 at 19:01:07 -0500:
> All code has bugs, of course, but from what I'm reading Subversion
> is doing something particular in that the developers are treating
> assert() as a list of TODO items that are not properly handled and
> really should be handled...
We do not use asserts as a todo list.
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@apache.org>.
On 24.12.2011 11:53, Stefan Sperling wrote:
> On Sat, Dec 24, 2011 at 11:24:14AM +0100, Branko Čibej wrote:
>> On 24.12.2011 09:41, Stefan Küng wrote:
>>> On 23.12.2011 23:52, Branko Čibej wrote:
>>>> Ranting is all very well, but I've yet to hear a suggestion from you
>>>> about how the libraries should handle unrecoverable errors. Like, for
>>>> example, the case where wc.db contains inconsistent and/or invalid data.
>>> Simple: return an error!
>>> That way the application can go on running, only the svn command is
>>> stopped.
>>> Sure, returning an error isn't always easy because it requires that
>>> you code a path to return to a known state. But for a library that's
>>> what has to be done.
>> In the case of Subversion, that would imply:
>>
>> * second, removing all uses of SVN_ERR_MALFUNCTION* and SVN_ERR_ASSERT*;
> I disagree.
Exactly. So do I. :)
>> * but first, making every library function return an svn_error_t.
> I doubt that's really an issue.
> Virtually any important call TSVN needs does return svn_error_t already.
Ah, but our private functions definitely do not.
Just saying, if we wanted to have a "completely recoverable" error
handling model, then what I said is what would have to be done. With the
obvious alternative of doing setjmp/longjmp magic, but that's a /really/
bad idea.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 24.12.2011 23:03, schamel23@spinor.com wrote:
> On 2011-12-24 16:58, Branko Čibej wrote:
>> On 24.12.2011 11:57, Stefan Küng wrote:
>>> On 24.12.2011 11:53, Stefan Sperling wrote:
>>>> On Sat, Dec 24, 2011 at 11:24:14AM +0100, Branko Čibej wrote:
>>>>> On 24.12.2011 09:41, Stefan Küng wrote:
>>>>>> On 23.12.2011 23:52, Branko Čibej wrote:
>>>>>>> Ranting is all very well, but I've yet to hear a suggestion from
>>>>>>> you
>>>>>>> about how the libraries should handle unrecoverable errors.
>>>>>>> Like, for
>>>>>>> example, the case where wc.db contains inconsistent and/or invalid
>>>>>>> data.
>>>>>>
>>>>>> Simple: return an error!
>>>>>> That way the application can go on running, only the svn command is
>>>>>> stopped.
>>>>>> Sure, returning an error isn't always easy because it requires that
>>>>>> you code a path to return to a known state. But for a library that's
>>>>>> what has to be done.
>>>>>
>>>>> In the case of Subversion, that would imply:
>>>>>
>>>>> * second, removing all uses of SVN_ERR_MALFUNCTION* and
>>>>> SVN_ERR_ASSERT*;
>>>>
>>>> I disagree. I am still putting assertions into the code at spots where
>>>> they are useful to catch bugs that get introduced by future code
>>>> changes.
>>>> Just recently I added assertions to the hotcopy code to check
>>>> variables
>>>> we rely on having certain values. That's totally fine. I made our test
>>>> suite exercise these assertions to make sure they don't trigger when
>>>> a hotcopy is made.
>>>
>>> asserts are not bad per se. They're very useful in debug builds. But
>>> for release builds they have to be replaced by something that returns
>>> an error message.
>>> That's why the c-runtime assert() is changed to an empty statement
>>> when NDEBUG is defined for release builds.
>>
>> This is where I most strenuously disagree. Microsoft's insistence on
>> disabling asserts in what they call "production" builds is the stupidest
>> idea ever.
>>
>> You put an assert into the code at a point where there is no safe way to
>> continue execution, where "safe" is defined as "will provably not
>> corrupt anything." Having asserts active only in "debug" builds assumes
>> that you have a test suite that provides 100% coverage of all possible
>> error conditions and will let you find all the bugs that can trigger the
>> assertion before you ship your production build. That assumption is
>> patently absurd.
>>
>> So, in real life, you run your tests, find some bugs, decide that's good
>> enough, turn off assertions, install the production build somewhere,
>> encounter a bug that would have triggered the assertion (except the
>> assert is not there any more) and happily, silently zap your
>> database/disk/whatever. You'll have a hard time convincing me that this
>> is a better idea than crashing Explorer.
>
> Why not just returning an error?
You're confusing the issue. I was responding to Stefan's assertion (sic)
that asserts (or rather, the aborts they cause) should be disabled in
production builds, and giving an example of what happens if you do that.
Having been down that road myself, I naturally assume Stefan's
assumption stems from the fact that Microsoft's compilers (and, once
upon a time, even libraries) do that by default. Microsoft even
recommends that practice for writing kernel device drivers ...
presumably on the assumption that it's better to trash your data than
crash your system?
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
On 2011-12-24 16:58, Branko Čibej wrote:
> On 24.12.2011 11:57, Stefan Küng wrote:
>> On 24.12.2011 11:53, Stefan Sperling wrote:
>>> On Sat, Dec 24, 2011 at 11:24:14AM +0100, Branko Čibej wrote:
>>>> On 24.12.2011 09:41, Stefan Küng wrote:
>>>>> On 23.12.2011 23:52, Branko Čibej wrote:
>>>>>> Ranting is all very well, but I've yet to hear a suggestion from you
>>>>>> about how the libraries should handle unrecoverable errors. Like, for
>>>>>> example, the case where wc.db contains inconsistent and/or invalid
>>>>>> data.
>>>>>
>>>>> Simple: return an error!
>>>>> That way the application can go on running, only the svn command is
>>>>> stopped.
>>>>> Sure, returning an error isn't always easy because it requires that
>>>>> you code a path to return to a known state. But for a library that's
>>>>> what has to be done.
>>>>
>>>> In the case of Subversion, that would imply:
>>>>
>>>> * second, removing all uses of SVN_ERR_MALFUNCTION* and
>>>> SVN_ERR_ASSERT*;
>>>
>>> I disagree. I am still putting assertions into the code at spots where
>>> they are useful to catch bugs that get introduced by future code
>>> changes.
>>> Just recently I added assertions to the hotcopy code to check variables
>>> we rely on having certain values. That's totally fine. I made our test
>>> suite exercise these assertions to make sure they don't trigger when
>>> a hotcopy is made.
>>
>> asserts are not bad per se. They're very useful in debug builds. But
>> for release builds they have to be replaced by something that returns
>> an error message.
>> That's why the c-runtime assert() is changed to an empty statement
>> when NDEBUG is defined for release builds.
>
> This is where I most strenuously disagree. Microsoft's insistence on
> disabling asserts in what they call "production" builds is the stupidest
> idea ever.
>
> You put an assert into the code at a point where there is no safe way to
> continue execution, where "safe" is defined as "will provably not
> corrupt anything." Having asserts active only in "debug" builds assumes
> that you have a test suite that provides 100% coverage of all possible
> error conditions and will let you find all the bugs that can trigger the
> assertion before you ship your production build. That assumption is
> patently absurd.
>
> So, in real life, you run your tests, find some bugs, decide that's good
> enough, turn off assertions, install the production build somewhere,
> encounter a bug that would have triggered the assertion (except the
> assert is not there any more) and happily, silently zap your
> database/disk/whatever. You'll have a hard time convincing me that this
> is a better idea than crashing Explorer.
Why not just returning an error?
>
> -- Brane
>
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@apache.org>.
On 24.12.2011 11:57, Stefan Küng wrote:
> On 24.12.2011 11:53, Stefan Sperling wrote:
>> On Sat, Dec 24, 2011 at 11:24:14AM +0100, Branko Čibej wrote:
>>> On 24.12.2011 09:41, Stefan Küng wrote:
>>>> On 23.12.2011 23:52, Branko Čibej wrote:
>>>>> Ranting is all very well, but I've yet to hear a suggestion from you
>>>>> about how the libraries should handle unrecoverable errors. Like, for
>>>>> example, the case where wc.db contains inconsistent and/or invalid
>>>>> data.
>>>>
>>>> Simple: return an error!
>>>> That way the application can go on running, only the svn command is
>>>> stopped.
>>>> Sure, returning an error isn't always easy because it requires that
>>>> you code a path to return to a known state. But for a library that's
>>>> what has to be done.
>>>
>>> In the case of Subversion, that would imply:
>>>
>>> * second, removing all uses of SVN_ERR_MALFUNCTION* and
>>> SVN_ERR_ASSERT*;
>>
>> I disagree. I am still putting assertions into the code at spots where
>> they are useful to catch bugs that get introduced by future code
>> changes.
>> Just recently I added assertions to the hotcopy code to check variables
>> we rely on having certain values. That's totally fine. I made our test
>> suite exercise these assertions to make sure they don't trigger when
>> a hotcopy is made.
>
> asserts are not bad per se. They're very useful in debug builds. But
> for release builds they have to be replaced by something that returns
> an error message.
> That's why the c-runtime assert() is changed to an empty statement
> when NDEBUG is defined for release builds.
This is where I most strenuously disagree. Microsoft's insistence on
disabling asserts in what they call "production" builds is the stupidest
idea ever.
You put an assert into the code at a point where there is no safe way to
continue execution, where "safe" is defined as "will provably not
corrupt anything." Having asserts active only in "debug" builds assumes
that you have a test suite that provides 100% coverage of all possible
error conditions and will let you find all the bugs that can trigger the
assertion before you ship your production build. That assumption is
patently absurd.
So, in real life, you run your tests, find some bugs, decide that's good
enough, turn off assertions, install the production build somewhere,
encounter a bug that would have triggered the assertion (except the
assert is not there any more) and happily, silently zap your
database/disk/whatever. You'll have a hard time convincing me that this
is a better idea than crashing Explorer.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Küng <to...@gmail.com>.
On 24.12.2011 11:53, Stefan Sperling wrote:
> On Sat, Dec 24, 2011 at 11:24:14AM +0100, Branko Čibej wrote:
>> On 24.12.2011 09:41, Stefan Küng wrote:
>>> On 23.12.2011 23:52, Branko Čibej wrote:
>>>> Ranting is all very well, but I've yet to hear a suggestion from you
>>>> about how the libraries should handle unrecoverable errors. Like, for
>>>> example, the case where wc.db contains inconsistent and/or invalid data.
>>>
>>> Simple: return an error!
>>> That way the application can go on running, only the svn command is
>>> stopped.
>>> Sure, returning an error isn't always easy because it requires that
>>> you code a path to return to a known state. But for a library that's
>>> what has to be done.
>>
>> In the case of Subversion, that would imply:
>>
>> * second, removing all uses of SVN_ERR_MALFUNCTION* and SVN_ERR_ASSERT*;
>
> I disagree. I am still putting assertions into the code at spots where
> they are useful to catch bugs that get introduced by future code changes.
> Just recently I added assertions to the hotcopy code to check variables
> we rely on having certain values. That's totally fine. I made our test
> suite exercise these assertions to make sure they don't trigger when
> a hotcopy is made.
asserts are not bad per se. They're very useful in debug builds. But for
release builds they have to be replaced by something that returns an
error message.
That's why the c-runtime assert() is changed to an empty statement when
NDEBUG is defined for release builds.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Dec 24, 2011 at 11:24:14AM +0100, Branko Čibej wrote:
> On 24.12.2011 09:41, Stefan Küng wrote:
> > On 23.12.2011 23:52, Branko Čibej wrote:
> >> Ranting is all very well, but I've yet to hear a suggestion from you
> >> about how the libraries should handle unrecoverable errors. Like, for
> >> example, the case where wc.db contains inconsistent and/or invalid data.
> >
> > Simple: return an error!
> > That way the application can go on running, only the svn command is
> > stopped.
> > Sure, returning an error isn't always easy because it requires that
> > you code a path to return to a known state. But for a library that's
> > what has to be done.
>
> In the case of Subversion, that would imply:
>
> * second, removing all uses of SVN_ERR_MALFUNCTION* and SVN_ERR_ASSERT*;
I disagree. I am still putting assertions into the code at spots where
they are useful to catch bugs that get introduced by future code changes.
Just recently I added assertions to the hotcopy code to check variables
we rely on having certain values. That's totally fine. I made our test
suite exercise these assertions to make sure they don't trigger when
a hotcopy is made.
The problem we're having is that early in wc-ng development people
put a lot of assertions into code reading data from the DB tables.
Those assertions were put there with all the right intentions but they
aren't covered by the test suite and it turns out sometimes data can get
into the DB that triggers these assertions. This is improper use of
assertions.
Putting the Explorer integration issue aside, these assertions also
cause problems for users of the 'svn' client and for ourselves when
trying to diagnose issues people are running into. Assertions don't
provide any context information. People report these assertions to the
list and we have no idea why they were triggered because we lack
information a proper error message would have provided.
E.g. we seriously had to ask users to dig out of the DB on our behalf
the path to a file lacking its BASE checksum which triggered a
checksum != NULL assertion. A proper error includes this information.
This is needless work we're asking our users to do.
> * but first, making every library function return an svn_error_t.
I doubt that's really an issue.
Virtually any important call TSVN needs does return svn_error_t already.
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
On 2011-12-27 10:50, Branko Čibej wrote:
> On 27.12.2011 09:57, schamel23@spinor.com wrote:
>> On 2011-12-25 10:57, Branko Čibej wrote:
>>> On 25.12.2011 10:20, schamel23@spinor.com wrote:
>>>> On 2011-12-25 07:00, Branko Čibej wrote:
>>>>> On 25.12.2011 01:01, Mark Mielke wrote:
>>>>>> Hey all. Just reading and I'm surprised that Subversion uses assert()
>>>>>> in production code. I don't think this is typical practice.
>>>>>
>>>>> You should read my rant about that in this thread. :)
>>>>
>>>> Assertions should never call abort.
>>>
>>> You should tell that to the designers of the C runtime library. :)
>>
>> You mean the GNU runtime library,
>
> No, I mean the C runtime library. Read a C textbook if you don't believe
> me. assert() is required to call abort() if the condition is false.
I believe you, you obviously know it better than me.
Just my point is still;
abort() may be fine for command line tools,
but is silly for libraries or GUI applications.
Today's software industry has evolved beyond the seventies. ;-)
Folker
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 27.12.2011 09:57, schamel23@spinor.com wrote:
> On 2011-12-25 10:57, Branko Čibej wrote:
>> On 25.12.2011 10:20, schamel23@spinor.com wrote:
>>> On 2011-12-25 07:00, Branko Čibej wrote:
>>>> On 25.12.2011 01:01, Mark Mielke wrote:
>>>>> Hey all. Just reading and I'm surprised that Subversion uses assert()
>>>>> in production code. I don't think this is typical practice.
>>>>
>>>> You should read my rant about that in this thread. :)
>>>
>>> Assertions should never call abort.
>>
>> You should tell that to the designers of the C runtime library. :)
>
> You mean the GNU runtime library,
No, I mean the C runtime library. Read a C textbook if you don't believe
me. assert() is required to call abort() if the condition is false.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
On 2011-12-25 10:57, Branko Čibej wrote:
> On 25.12.2011 10:20, schamel23@spinor.com wrote:
>> On 2011-12-25 07:00, Branko Čibej wrote:
>>> On 25.12.2011 01:01, Mark Mielke wrote:
>>>> Hey all. Just reading and I'm surprised that Subversion uses assert()
>>>> in production code. I don't think this is typical practice.
>>>
>>> You should read my rant about that in this thread. :)
>>
>> Assertions should never call abort.
>
> You should tell that to the designers of the C runtime library. :)
You mean the GNU runtime library, designed by people who were
primarily used to command line tools where abort is no problem. ;-)
The Windows / Visual Studio C runtime behaves different, better,
you never get an abort in release mode.
And more modern languages like Python or Java behave much much
better. ;-)
Folker
>
> -- Brane
>
>
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 25.12.2011 10:20, schamel23@spinor.com wrote:
> On 2011-12-25 07:00, Branko Čibej wrote:
>> On 25.12.2011 01:01, Mark Mielke wrote:
>>> Hey all. Just reading and I'm surprised that Subversion uses assert()
>>> in production code. I don't think this is typical practice.
>>
>> You should read my rant about that in this thread. :)
>
> Assertions should never call abort.
You should tell that to the designers of the C runtime library. :)
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
On 2011-12-25 07:00, Branko Čibej wrote:
> On 25.12.2011 01:01, Mark Mielke wrote:
>> Hey all. Just reading and I'm surprised that Subversion uses assert()
>> in production code. I don't think this is typical practice.
>
> You should read my rant about that in this thread. :)
Assertions should never call abort.
Assertions should be just like any error,
possibly providing more information like a call stack.
Like in Python where assertions are just normal exceptions,
which can be caught normally including all call stack information
(which for example in our case are logged into a log file
which a customer can send us. So we get the best of both worlds:
An assert does not crash the app, the app can continue,
but we can still get the callstack information).
Folker
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 25.12.2011 01:01, Mark Mielke wrote:
> Hey all. Just reading and I'm surprised that Subversion uses assert()
> in production code. I don't think this is typical practice.
You should read my rant about that in this thread. :)
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Mark Mielke <ma...@mark.mielke.cc>.
On 12/24/2011 05:54 AM, Stefan Küng wrote:
> Explorer is of course a bigger issue than other apps using the svn
> library. But other UI have the same issue.
> I've mentioned this example before:
> Imagine you're using an image editor (e.g. photoshop, paint.net, ...)
> and instead of showing an error when loading a corrupted image and
> then proceed, they would just show an assert dialog and then exit.
> Do you _really_ think that's how an UI should work?
Heh. That's what Outlook does. :-) Although at least it brings up a
pretty dialog that offers to send crash results to Microsoft and restart.
Hey all. Just reading and I'm surprised that Subversion uses assert() in
production code. I don't think this is typical practice. Is it not
typical practice to enable assert() in development code but remove it
for production? The idea is to provide extra guards during the
development and testing, but not to take down production environments?
My thinking is that if you have production code raise assert(), then the
code is not of sufficient quality to have been released in the first place.
All code has bugs, of course, but from what I'm reading Subversion is
doing something particular in that the developers are treating assert()
as a list of TODO items that are not properly handled and really should
be handled...
--
Mark Mielke<ma...@mielke.cc>
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
On 2011-12-25 11:31, Stefan Sperling wrote:
> On Sun, Dec 25, 2011 at 11:00:26AM +0100, Branko Čibej wrote:
>> On 25.12.2011 10:20, schamel23@spinor.com wrote:
>>> On 2011-12-25 06:37, Branko Čibej wrote:
>>>> There are always going to be cases where you have to
>>>> decide between aborting, or risking data corruption (or worse). Which
>>>> would you pick?
>>>
>>> Definitely data corruption, because (except for bugs) every data
>>> corruption is continuable and somehow recoverable,
>>> e.g. in the worst case by the user re-checking out the wc.
>>
>> That's an interesting point of view. You are of course assuming that
>> such data corruption is easily detectable.
Cannot be and needn't be done.
> And that it doesn't waste
>> days of work.
>
> And that it isn't exploitable...
That's also not harder.
In the simplest case, basically the only thing you have to do
is replace all asserts by returning an error, possibly
also setting your wc handle to a cannot-be-used-anymore state.
That's it.
(Even better, your code just reports an error and continues,
like if you open a word file but it cannot find a referenced
image file, but for svn there are probably not such cases.)
Yes, it requires a completely different programming philosophy.
But this is not more work in practice.
But of course, a programming language can help you a lot
(like Python/Java).
> I don't think this conversation can get anywhere because the terms are
> too abstract. We should be discussing specific examples.
> Stefan already provided some and I agree that we've been using assertions
> too generously in some cases. In other cases they're warranted.
> We'll have to review our SVN_ERR_ASSERT calls and take appropriate
> action on a case-by-case basis.
Yes, I think this is the right approach in practice for svn,
and what Stefan would like to see.
But not only, I think the value of the "abstract discussion" is
influencing future API designs, e.g. as already mentioned by someone,
that every API function should be able to return an error
to not force the implementation to use an assert as last resort.
Folker
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Dec 25, 2011 at 11:00:26AM +0100, Branko Čibej wrote:
> On 25.12.2011 10:20, schamel23@spinor.com wrote:
> > On 2011-12-25 06:37, Branko Čibej wrote:
> >> There are always going to be cases where you have to
> >> decide between aborting, or risking data corruption (or worse). Which
> >> would you pick?
> >
> > Definitely data corruption, because (except for bugs) every data
> > corruption is continuable and somehow recoverable,
> > e.g. in the worst case by the user re-checking out the wc.
>
> That's an interesting point of view. You are of course assuming that
> such data corruption is easily detectable. And that it doesn't waste
> days of work.
And that it isn't exploitable...
I don't think this conversation can get anywhere because the terms are
too abstract. We should be discussing specific examples.
Stefan already provided some and I agree that we've been using assertions
too generously in some cases. In other cases they're warranted.
We'll have to review our SVN_ERR_ASSERT calls and take appropriate
action on a case-by-case basis.
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Küng <to...@gmail.com>.
On 24.12.2011 17:01, Branko Čibej wrote:
> On 24.12.2011 16:57, Stefan Küng wrote:
>> an assert implies that you _know_ something is wrong and you could
>> back out without taking the process down with you
>
> No. That is inappropriate usage of assertions. Assert means that if the
> condition is not met, you cannot continue. One should not use assertions
> for, e.g., validation of public API parameters.
So, in case of e.g. a corrupted working copy which has paths messed up
(and you know that the paths are messed up), then you would agree that
returning an error like "paths not correct, wc might be corrupted" and
let the application continue would be better than just aborting the
process? Yes?
Then why are there multiple statements like this in the svn code:
SVN_ERR_ASSERT_NO_RETURN(svn_dirent_is_absolute(local_abspath));
(example from libsvn_wc\util.c, line 197).
A simple SVN_ERR_ASSERT() would be much better, but no, the whole
process is not allowed to proceed (notice the _NO_RETURN) and must be
aborted.
That's just one example of many, many more. A simple search for
SVN_ERR_ASSERT_NO_RETURN will give you an idea. I only found about two
which I would consider real _NO_RETURN situations. All others are
completely recoverable.
I have no problems with asserts, or even the _NO_RETURN asserts if they
are used right. _NO_RETURN must not be used where the situation is
recoverable.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 24.12.2011 16:57, Stefan Küng wrote:
> an assert implies that you _know_ something is wrong and you could
> back out without taking the process down with you
No. That is inappropriate usage of assertions. Assert means that if the
condition is not met, you cannot continue. One should not use assertions
for, e.g., validation of public API parameters.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Küng <to...@gmail.com>.
On 24.12.2011 16:50, Branko Čibej wrote:
> On 24.12.2011 11:54, Stefan Küng wrote:
>> maybe you have a 10GHz machine on your hands. But most people don't.
>> Using RPC for every svn API call would bring every machine down easily.
>
> Oh come now. We're not talking about some Enterprise XYZ RPC thingamabob
> that does everything through a distributed transaction manager. Local
> IPC-based RPC isn't all that slow. But that's beside the point.
>
> My point is that (a) there are alternatives, and (b) there is no way
> under the sun to make the Subversion libraries 100% crash-safe, so if
> you need to protect your plugin environment from crashes, splitting off
> the "unstable" code into a separate daemon process is a fairly standard
> method for doing that.
1) I'm already using a separate process where appropriate
2) when did I ever request a 100% crash-safe lib? All I ever asked was
to not crash when it can be clearly avoided (an assert implies that you
_know_ something is wrong and you could back out without taking the
process down with you)
3) why am I still replying to this thread? maybe you're right and I'm
already insane.
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@apache.org>.
On 25.12.2011 10:20, schamel23@spinor.com wrote:
> On 2011-12-25 06:37, Branko Čibej wrote:
>> On 24.12.2011 23:03, schamel23@spinor.com wrote:
>>>> On 24.12.2011 11:54, Stefan Küng wrote:
>>>>> maybe you have a 10GHz machine on your hands. But most people don't.
>>>>> Using RPC for every svn API call would bring every machine down
>>>>> easily.
>>>>
>>>> Oh come now. We're not talking about some Enterprise XYZ RPC
>>>> thingamabob
>>>> that does everything through a distributed transaction manager. Local
>>>> IPC-based RPC isn't all that slow. But that's beside the point.
>>>>
>>>> My point is that (a) there are alternatives, and (b) there is no way
>>>> under the sun to make the Subversion libraries 100% crash-safe,
>>>
>>> It is a very, very, very broken design if a library can abort.
>>> Even "only" crashing a plugin is not acceptable.
>>
>> It's not a matter of design. I agree that abort-like constructs are
>> being abused in the libraries, and that's an implementation issue, not a
>> design issue. But no design in the world is going to avoid external
>> circumstances. There are always going to be cases where you have to
>> decide between aborting, or risking data corruption (or worse). Which
>> would you pick?
>
> Definitely data corruption, because (except for bugs) every data
> corruption is continuable and somehow recoverable,
> e.g. in the worst case by the user re-checking out the wc.
That's an interesting point of view. You are of course assuming that
such data corruption is easily detectable. And that it doesn't waste
days of work.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
On 2011-12-25 06:37, Branko Čibej wrote:
> On 24.12.2011 23:03, schamel23@spinor.com wrote:
>>> On 24.12.2011 11:54, Stefan Küng wrote:
>>>> maybe you have a 10GHz machine on your hands. But most people don't.
>>>> Using RPC for every svn API call would bring every machine down easily.
>>>
>>> Oh come now. We're not talking about some Enterprise XYZ RPC thingamabob
>>> that does everything through a distributed transaction manager. Local
>>> IPC-based RPC isn't all that slow. But that's beside the point.
>>>
>>> My point is that (a) there are alternatives, and (b) there is no way
>>> under the sun to make the Subversion libraries 100% crash-safe,
>>
>> It is a very, very, very broken design if a library can abort.
>> Even "only" crashing a plugin is not acceptable.
>
> It's not a matter of design. I agree that abort-like constructs are
> being abused in the libraries, and that's an implementation issue, not a
> design issue. But no design in the world is going to avoid external
> circumstances. There are always going to be cases where you have to
> decide between aborting, or risking data corruption (or worse). Which
> would you pick?
Definitely data corruption, because (except for bugs) every data
corruption is continuable and somehow recoverable,
e.g. in the worst case by the user re-checking out the wc.
Having one broken wc on your disk is definitely better
than crashing the whole explorer and *all* other running operations
with it.
In our (much bigger) software every error is recoverable
without memory leaks (except for bugs we try to fix),
even broken low-level serialization streams.
In the worst case you cannot use some particular data/project file
anymore, but the app should never crash, you should always be open
a different project for instance.
Folker
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 24.12.2011 23:03, schamel23@spinor.com wrote:
>> On 24.12.2011 11:54, Stefan Küng wrote:
>>> maybe you have a 10GHz machine on your hands. But most people don't.
>>> Using RPC for every svn API call would bring every machine down easily.
>>
>> Oh come now. We're not talking about some Enterprise XYZ RPC thingamabob
>> that does everything through a distributed transaction manager. Local
>> IPC-based RPC isn't all that slow. But that's beside the point.
>>
>> My point is that (a) there are alternatives, and (b) there is no way
>> under the sun to make the Subversion libraries 100% crash-safe,
>
> It is a very, very, very broken design if a library can abort.
> Even "only" crashing a plugin is not acceptable.
It's not a matter of design. I agree that abort-like constructs are
being abused in the libraries, and that's an implementation issue, not a
design issue. But no design in the world is going to avoid external
circumstances. There are always going to be cases where you have to
decide between aborting, or risking data corruption (or worse). Which
would you pick?
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
> On 24.12.2011 11:54, Stefan Küng wrote:
>> maybe you have a 10GHz machine on your hands. But most people don't.
>> Using RPC for every svn API call would bring every machine down easily.
>
> Oh come now. We're not talking about some Enterprise XYZ RPC thingamabob
> that does everything through a distributed transaction manager. Local
> IPC-based RPC isn't all that slow. But that's beside the point.
>
> My point is that (a) there are alternatives, and (b) there is no way
> under the sun to make the Subversion libraries 100% crash-safe,
It is a very, very, very broken design if a library can abort.
Even "only" crashing a plugin is not acceptable. (*)
(*) Of course there may be the practical problem of too much work
to fix this. Stefan understands this, as long as he has the feeling
that you at least try to get low hanging fruits by avoiding abort
where they can be avoided easily.
And that for example every new API function supports return an error:
It may be too hard to fix an old design bug,
but you should at least avoid it in the future as much as possible.
>
> so if
> you need to protect your plugin environment from crashes, splitting off
> the "unstable" code into a separate daemon process is a fairly standard
> method for doing that.
>
> -- Brane
>
>
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 24.12.2011 11:54, Stefan Küng wrote:
> maybe you have a 10GHz machine on your hands. But most people don't.
> Using RPC for every svn API call would bring every machine down easily.
Oh come now. We're not talking about some Enterprise XYZ RPC thingamabob
that does everything through a distributed transaction manager. Local
IPC-based RPC isn't all that slow. But that's beside the point.
My point is that (a) there are alternatives, and (b) there is no way
under the sun to make the Subversion libraries 100% crash-safe, so if
you need to protect your plugin environment from crashes, splitting off
the "unstable" code into a separate daemon process is a fairly standard
method for doing that.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Küng <to...@gmail.com>.
On 24.12.2011 11:24, Branko Čibej wrote:
> I could easily turn your reasoning around and simply state that you're
> not using the libraries correctly, since you knew from day one that they
> can "throw unrecoverable errors" (which is what assert() is all about,
Sorry, that's not true.
The abuse of asserts was introduced to the svn library somewhere after
v1.2 or even later. The first versions returned errors.
And if there is a situation from which you can't recover, then please
use an assert. But since when is "path is not canonicalized" an
unrecoverable situation? And that's just the most obvious abuse, there
are many many more.
> after all), and therefore you should not be loading SVN libraries
> directly into an Explorer process but should instead spawn them in a
> separate, isolated daemon process and RPC that from your shell
> extension. Sounds fun? No doubt. My guess is that it's less work than
> making every possible error condition that the Subversion libraries can
> encounter recoverable.
maybe you have a 10GHz machine on your hands. But most people don't.
Using RPC for every svn API call would bring every machine down easily.
If you want to provide a library that's not safe to call in to from any
bigger process, then you have to either state that or provide your own
RPC mechanism.
> It's not even a valid argument to say that the libraries, such as they
> are now, are not suitable for UI development, since not every UI is such
> that a glitch in one of its plugins effectively brings down the whole
> system. The Explorer extension environment is an exception. It's
> basically your responsibility to take that into account, and whilst it's
> reasonable to expect the SVN libraries to meet you halfway, it's quite
> unreasonable to lay all the blame for your problems at their (rather,
> our) doorstep.
Explorer is of course a bigger issue than other apps using the svn
library. But other UI have the same issue.
I've mentioned this example before:
Imagine you're using an image editor (e.g. photoshop, paint.net, ...)
and instead of showing an error when loading a corrupted image and then
proceed, they would just show an assert dialog and then exit.
Do you _really_ think that's how an UI should work?
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@apache.org>.
On 24.12.2011 09:41, Stefan Küng wrote:
> On 23.12.2011 23:52, Branko Čibej wrote:
>> On 23.12.2011 18:49, Stefan Küng wrote:
>>> I have _repeatedly_ mentioned on this list that this kind of "error
>>> handling" (quotes intended) is bad. Sure, it won't affect the CL
>>> client that much, but for a library that's just not acceptable.
>>> But others have similar error handling implemented:
>>> http://thedailywtf.com/Articles/Serious-Failure.aspx
>>
>> Ranting is all very well, but I've yet to hear a suggestion from you
>> about how the libraries should handle unrecoverable errors. Like, for
>> example, the case where wc.db contains inconsistent and/or invalid data.
>
> Simple: return an error!
> That way the application can go on running, only the svn command is
> stopped.
> Sure, returning an error isn't always easy because it requires that
> you code a path to return to a known state. But for a library that's
> what has to be done.
In the case of Subversion, that would imply:
* second, removing all uses of SVN_ERR_MALFUNCTION* and SVN_ERR_ASSERT*;
* but first, making every library function return an svn_error_t.
I think that would be overkill. People tend to assume that any error
condition can be recovered from, but that's not the case at all. There
are many conditions where trying to recover from them does more damage
than hitting the big red button.
I could easily turn your reasoning around and simply state that you're
not using the libraries correctly, since you knew from day one that they
can "throw unrecoverable errors" (which is what assert() is all about,
after all), and therefore you should not be loading SVN libraries
directly into an Explorer process but should instead spawn them in a
separate, isolated daemon process and RPC that from your shell
extension. Sounds fun? No doubt. My guess is that it's less work than
making every possible error condition that the Subversion libraries can
encounter recoverable.
It's not even a valid argument to say that the libraries, such as they
are now, are not suitable for UI development, since not every UI is such
that a glitch in one of its plugins effectively brings down the whole
system. The Explorer extension environment is an exception. It's
basically your responsibility to take that into account, and whilst it's
reasonable to expect the SVN libraries to meet you halfway, it's quite
unreasonable to lay all the blame for your problems at their (rather,
our) doorstep.
> I think I've explained my reasons enough already, so I'll stop now.
The reasons are obvious, the method is what needs working on, that's all. :)
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Küng <to...@gmail.com>.
On 24.12.2011 11:37, Stefan Sperling wrote:
> On Sat, Dec 24, 2011 at 09:41:30AM +0100, Stefan Küng wrote:
>> Yes, I'm ranting. But I think I have a reason to. Because I'm pretty
>> sure that the asserts in the svn library will stay - you guys made
>> it clear every time I brought this up that you rather have the app
>> crash than not have a chance to get a useful stack trace from users.
>
> Oh, come on. We are making commits to fix these issues and there
> are and have been corresponding nominations in STATUS to get these
> fixes backported ASAP.
>
> I think you tend to misunderstand people's writing tone sometimes.
> That's understandable, but please try to stop being so defensive.
> You shouldn't assume people in this community are working against you.
I know that you're now trying to improve the situation. That was my
intention after all.
When you guys first started abusing asserts, all reports for those came
to the TSVN mailing list. I had to answer those reports, find out the
reason (which takes a lot of emails back and fourth with those users)
and then come up with a reproduction recipe so I could report it here.
Back then, my complaints about that were completely ignored here.
Now that I've implemented a dialog in TSVN that tells users to report
the issue directly to the svn devs, now you're starting to notice how
much work this causes and how annoying it is.
So for me this is now a moment of "I told you so".
But then I read some comments from you guys that it's still my fault,
that I have to either change back the dialogs (sorry, won't happen: I've
got enough work with TSVN as it is) or that I'm using the malfunction
handler wrong.
So please forgive me if I seem a little upset with this discussion.
> And don't forget that you also have commit access to Subversion itself
> which means your voice counts as much as any other committer's voice and
> you can also help out making happen changes which TSVN needs.
> For instance, there is this nomination in STATUS right now which could use
> your vote:
>
> * r1222521, r1222693
> Turn another wc-db assertion people are reporting into a normal error.
> Justification:
> An informative error message helps people more than an assert (which
> in case of TSVN can mean crashing the Windows Explorer).
> Reported several times on users@, see:
> http://svn.haxx.se/users/archive-2011-11/0527.shtml
> http://svn.haxx.se/users/archive-2011-12/0063.shtml
> http://svn.haxx.se/users/archive-2011-12/0299.shtml
> Votes:
> +1: stsp
Looks good. Voted.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Dec 24, 2011 at 09:41:30AM +0100, Stefan Küng wrote:
> Yes, I'm ranting. But I think I have a reason to. Because I'm pretty
> sure that the asserts in the svn library will stay - you guys made
> it clear every time I brought this up that you rather have the app
> crash than not have a chance to get a useful stack trace from users.
Oh, come on. We are making commits to fix these issues and there
are and have been corresponding nominations in STATUS to get these
fixes backported ASAP.
I think you tend to misunderstand people's writing tone sometimes.
That's understandable, but please try to stop being so defensive.
You shouldn't assume people in this community are working against you.
Your client is quite likely the most-used SVN client (at elego I've held
more workshop classes based on TSVN than any other workshop in our portfolio
combined) and it is of very high quality and we're very much interested
in it succeeding, just like you are.
And don't forget that you also have commit access to Subversion itself
which means your voice counts as much as any other committer's voice and
you can also help out making happen changes which TSVN needs.
For instance, there is this nomination in STATUS right now which could use
your vote:
* r1222521, r1222693
Turn another wc-db assertion people are reporting into a normal error.
Justification:
An informative error message helps people more than an assert (which
in case of TSVN can mean crashing the Windows Explorer).
Reported several times on users@, see:
http://svn.haxx.se/users/archive-2011-11/0527.shtml
http://svn.haxx.se/users/archive-2011-12/0063.shtml
http://svn.haxx.se/users/archive-2011-12/0299.shtml
Votes:
+1: stsp
You may feel that you're having a hard time convincing people but I
don't think that is what is actually happening. We're all acting as
equals here.
So, piece, and Merry Christmas!
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Küng <to...@gmail.com>.
On 23.12.2011 23:52, Branko Čibej wrote:
> On 23.12.2011 18:49, Stefan Küng wrote:
>> On 23.12.2011 15:18, Stefan Sperling wrote:
>>> On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
>>>> On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
>>>>> On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
>>>>>> On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
>>>>>>> On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
>>>>>>>> stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
>>>>>>>>> + An informative error message helps people more than an
>>>>>>>>> assert (which
>>>>>>>>> + in case of TSVN can mean crashing the Windows Explorer).
>>>>>>>>
>>>>>>>> I thought the svn_error_malfunction_handler_t stuff addressed
>>>>>>>> that issue?
>>>>>>>
>>>>>>> No, because TSVN didn't end up using it as intended.
>>>>>>> See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
>>>>>>
>>>>>> I don't see how that's relevant? That link discusses the error
>>>>>> dialogs.
>>>>>> What I'm asking is whether TSVN has implemented an
>>>>>> svn_error_malfunction_handler_t that replaces the default handler,
>>>>>> svn_error_abort_on_malfunction().
>>>>>
>>>>> Yes it does (called svn_error_handle_malfunction()) but it calls
>>>>> abort()
>>>>> which means the explorer crashes:
>>>>> http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
>>>>
>>>> And they can't remove the abort() call because...?
>>>
>>> As Stefan explained, he cannot rely on the internal state of the
>>> application if he gets an assertion thrown out of the svn libraries.
>>>
>>> And he has a point. He assumes that asserts are fatal and
>>> non-recoverable
>>> errors. What if Subversion asserts because it just trashed the entire
>>> stack and heap space of the windows explorer and happens to operate on
>>> garbaga data that triggers an assertion? It doesn't make any sense to
>>> try
>>> to continue.
>>>
>>> Now, we know that most of our assertions are due to bugs in our code
>>> where, for instance, invalid data entered wc.db. But an application
>>> using the svn libraries cannot safely rely on this knowledge.
>>> Changing all these bogus asserts that trigger just because of invalid
>>> working copy meta-data into proper errors is the right way to fix this.
>>
>> I think I have to explain something here:
>> First, the shell extension of TSVN does not even call
>> svn_error_set_malfunction_handler() to set its own handler. Since that
>> function clearly states that "This function must be called in a
>> single-threaded context", I can not initialize that in the shell
>> extension since the extension can get loaded in multi threaded
>> situations.
>> And the default handler asserts and calls abort(). So the whole shell
>> goes down in such situations.
>>
>> Then: the reason I call abort() in my own handler for TortoiseProc
>> (the UI part of TSVN) is that the SVN also clearly state that "If
>> can_return is false the method should never return. (The caller will
>> call abort())".
>> So what else than call abort() can I do? I can not return from that
>> function if can_return is false. No way to exit but to call abort().
>> So how is that "not implemented as intended"???
>> At least now in the UI part I can show a nasty dialog box telling the
>> users where to report the problem to. That way it's not me that gets
>> bothered with these since I can't do anything about them anyway.
>>
>> I have _repeatedly_ mentioned on this list that this kind of "error
>> handling" (quotes intended) is bad. Sure, it won't affect the CL
>> client that much, but for a library that's just not acceptable.
>> But others have similar error handling implemented:
>> http://thedailywtf.com/Articles/Serious-Failure.aspx
>
> Ranting is all very well, but I've yet to hear a suggestion from you
> about how the libraries should handle unrecoverable errors. Like, for
> example, the case where wc.db contains inconsistent and/or invalid data.
Simple: return an error!
That way the application can go on running, only the svn command is stopped.
Sure, returning an error isn't always easy because it requires that you
code a path to return to a known state. But for a library that's what
has to be done.
Just a real life example from a user who sent a very nasty email about
three weeks ago:
In case you didn't know, the Windows explorer is multi threaded. The
user started a backup copy operation of a 4GB directory with thousands
of files to an external drive (using explorer). Since this takes a long
time (user said about an hour) he kept working on his project while the
backup copy was in progress. About half through that copy operation, a
simple right-click on an svn working copy triggered an assertion (maybe
his working copy was corrupt?) and explorer went down.
Very nice error handling: he had to start with his backup all over again.
For me, such a behavior is not acceptable. And I've mentioned this
repeatedly here without much success.
Yes, I'm ranting. But I think I have a reason to. Because I'm pretty
sure that the asserts in the svn library will stay - you guys made it
clear every time I brought this up that you rather have the app crash
than not have a chance to get a useful stack trace from users.
I'm just wondering: how many times did you actually get a stack trace?
You're dealing with users here, not svn developers who have a debugger
installed and ready to use. And even if they have a debugger installed:
how many .NET, PHP, java, ... devs would even know how to get a stack
trace from an assert in a C app?
I think I've explained my reasons enough already, so I'll stop now.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
schamel23@spinor.com wrote on Sat, Dec 24, 2011 at 09:46:41 +0100:
> On 2011-12-23 23:52, Branko Čibej wrote:
> >On 23.12.2011 18:49, Stefan Küng wrote:
> >>On 23.12.2011 15:18, Stefan Sperling wrote:
> >>>On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
> >>>>On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
> >>>>>On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> >>>>>>On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> >>>>>>>On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> >>>>>>>>stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> >>>>>>>>>+ An informative error message helps people more than an
> >>>>>>>>>assert (which
> >>>>>>>>>+ in case of TSVN can mean crashing the Windows Explorer).
> >>>>>>>>
> >>>>>>>>I thought the svn_error_malfunction_handler_t stuff addressed
> >>>>>>>>that issue?
> >>>>>>>
> >>>>>>>No, because TSVN didn't end up using it as intended.
> >>>>>>>See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
> >>>>>>
> >>>>>>I don't see how that's relevant? That link discusses the error
> >>>>>>dialogs.
> >>>>>>What I'm asking is whether TSVN has implemented an
> >>>>>>svn_error_malfunction_handler_t that replaces the default handler,
> >>>>>>svn_error_abort_on_malfunction().
> >>>>>
> >>>>>Yes it does (called svn_error_handle_malfunction()) but it calls
> >>>>>abort()
> >>>>>which means the explorer crashes:
> >>>>>http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
> >>>>
> >>>>And they can't remove the abort() call because...?
> >>>
> >>>As Stefan explained, he cannot rely on the internal state of the
> >>>application if he gets an assertion thrown out of the svn libraries.
> >>>
> >>>And he has a point. He assumes that asserts are fatal and
> >>>non-recoverable
> >>>errors. What if Subversion asserts because it just trashed the entire
> >>>stack and heap space of the windows explorer and happens to operate on
> >>>garbaga data that triggers an assertion? It doesn't make any sense to
> >>>try
> >>>to continue.
> >>>
> >>>Now, we know that most of our assertions are due to bugs in our code
> >>>where, for instance, invalid data entered wc.db. But an application
> >>>using the svn libraries cannot safely rely on this knowledge.
> >>>Changing all these bogus asserts that trigger just because of invalid
> >>>working copy meta-data into proper errors is the right way to fix this.
> >>
> >>I think I have to explain something here:
> >>First, the shell extension of TSVN does not even call
> >>svn_error_set_malfunction_handler() to set its own handler. Since that
> >>function clearly states that "This function must be called in a
> >>single-threaded context", I can not initialize that in the shell
> >>extension since the extension can get loaded in multi threaded
> >>situations.
> >>And the default handler asserts and calls abort(). So the whole shell
> >>goes down in such situations.
> >>
> >>Then: the reason I call abort() in my own handler for TortoiseProc
> >>(the UI part of TSVN) is that the SVN also clearly state that "If
> >>can_return is false the method should never return. (The caller will
> >>call abort())".
> >>So what else than call abort() can I do? I can not return from that
> >>function if can_return is false. No way to exit but to call abort().
> >>So how is that "not implemented as intended"???
> >>At least now in the UI part I can show a nasty dialog box telling the
> >>users where to report the problem to. That way it's not me that gets
> >>bothered with these since I can't do anything about them anyway.
> >>
> >>I have _repeatedly_ mentioned on this list that this kind of "error
> >>handling" (quotes intended) is bad. Sure, it won't affect the CL
> >>client that much, but for a library that's just not acceptable.
> >>But others have similar error handling implemented:
> >>http://thedailywtf.com/Articles/Serious-Failure.aspx
> >
> >Ranting is all very well, but I've yet to hear a suggestion from you
> >about how the libraries should handle unrecoverable errors. Like, for
> >example, the case where wc.db contains inconsistent and/or invalid data.
>
> Simple answer:
> Return an normal error, giving the caller (e.g. TSVN shell extension)
> the chance for example to continue its work on other working copies.
> But definitely never just abort or assert (crash the Windows explorer).
>
How is calling svn_error_malfunction_handler_t(can_return=TRUE) not such
a mechanism?
When can_return=FALSE then the library itself abort()s if the callback
does return... but when can_return=TRUE, the library is supposed to
propagate the error all the way up[1]
[1] though, in practice, we may have places that do
if (err && err->apr_err == FOO)
/* .. */
else
svn_error_clear(err);
which would mask assertions. Perhaps we should make it a fatal error to
call svn_error_clear() on an error if SVN_ERROR_IN_CATEGORY(SVN_ERR_MALFUNC_CATEGORY_START) ?
> So there must be never "unrecoverable errors".
> There may be some unrecoverable data (e.g. a broken db/WC),
> but the caller must be able to continue work on other data
> (e.g. another WC).
>
+1
> Cheers,
> Folkr
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by sc...@spinor.com.
On 2011-12-23 23:52, Branko Čibej wrote:
> On 23.12.2011 18:49, Stefan Küng wrote:
>> On 23.12.2011 15:18, Stefan Sperling wrote:
>>> On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
>>>> On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
>>>>> On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
>>>>>> On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
>>>>>>> On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
>>>>>>>> stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
>>>>>>>>> + An informative error message helps people more than an
>>>>>>>>> assert (which
>>>>>>>>> + in case of TSVN can mean crashing the Windows Explorer).
>>>>>>>>
>>>>>>>> I thought the svn_error_malfunction_handler_t stuff addressed
>>>>>>>> that issue?
>>>>>>>
>>>>>>> No, because TSVN didn't end up using it as intended.
>>>>>>> See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
>>>>>>
>>>>>> I don't see how that's relevant? That link discusses the error
>>>>>> dialogs.
>>>>>> What I'm asking is whether TSVN has implemented an
>>>>>> svn_error_malfunction_handler_t that replaces the default handler,
>>>>>> svn_error_abort_on_malfunction().
>>>>>
>>>>> Yes it does (called svn_error_handle_malfunction()) but it calls
>>>>> abort()
>>>>> which means the explorer crashes:
>>>>> http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
>>>>
>>>> And they can't remove the abort() call because...?
>>>
>>> As Stefan explained, he cannot rely on the internal state of the
>>> application if he gets an assertion thrown out of the svn libraries.
>>>
>>> And he has a point. He assumes that asserts are fatal and
>>> non-recoverable
>>> errors. What if Subversion asserts because it just trashed the entire
>>> stack and heap space of the windows explorer and happens to operate on
>>> garbaga data that triggers an assertion? It doesn't make any sense to
>>> try
>>> to continue.
>>>
>>> Now, we know that most of our assertions are due to bugs in our code
>>> where, for instance, invalid data entered wc.db. But an application
>>> using the svn libraries cannot safely rely on this knowledge.
>>> Changing all these bogus asserts that trigger just because of invalid
>>> working copy meta-data into proper errors is the right way to fix this.
>>
>> I think I have to explain something here:
>> First, the shell extension of TSVN does not even call
>> svn_error_set_malfunction_handler() to set its own handler. Since that
>> function clearly states that "This function must be called in a
>> single-threaded context", I can not initialize that in the shell
>> extension since the extension can get loaded in multi threaded
>> situations.
>> And the default handler asserts and calls abort(). So the whole shell
>> goes down in such situations.
>>
>> Then: the reason I call abort() in my own handler for TortoiseProc
>> (the UI part of TSVN) is that the SVN also clearly state that "If
>> can_return is false the method should never return. (The caller will
>> call abort())".
>> So what else than call abort() can I do? I can not return from that
>> function if can_return is false. No way to exit but to call abort().
>> So how is that "not implemented as intended"???
>> At least now in the UI part I can show a nasty dialog box telling the
>> users where to report the problem to. That way it's not me that gets
>> bothered with these since I can't do anything about them anyway.
>>
>> I have _repeatedly_ mentioned on this list that this kind of "error
>> handling" (quotes intended) is bad. Sure, it won't affect the CL
>> client that much, but for a library that's just not acceptable.
>> But others have similar error handling implemented:
>> http://thedailywtf.com/Articles/Serious-Failure.aspx
>
> Ranting is all very well, but I've yet to hear a suggestion from you
> about how the libraries should handle unrecoverable errors. Like, for
> example, the case where wc.db contains inconsistent and/or invalid data.
Simple answer:
Return an normal error, giving the caller (e.g. TSVN shell extension)
the chance for example to continue its work on other working copies.
But definitely never just abort or assert (crash the Windows explorer).
This is standard in today's applications.
For example, when opening a corrupt document in OpenOffice,
in the worst case you may get an error "corrupt file",
but this does not crash OpenOffice itself,
but the user can choose to open another file.
So there must be never "unrecoverable errors".
There may be some unrecoverable data (e.g. a broken db/WC),
but the caller must be able to continue work on other data
(e.g. another WC).
Cheers,
Folkr
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Branko Čibej <br...@xbc.nu>.
On 23.12.2011 18:49, Stefan Küng wrote:
> On 23.12.2011 15:18, Stefan Sperling wrote:
>> On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
>>> On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
>>>> On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
>>>>> On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
>>>>>> On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
>>>>>>> stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
>>>>>>>> + An informative error message helps people more than an
>>>>>>>> assert (which
>>>>>>>> + in case of TSVN can mean crashing the Windows Explorer).
>>>>>>>
>>>>>>> I thought the svn_error_malfunction_handler_t stuff addressed
>>>>>>> that issue?
>>>>>>
>>>>>> No, because TSVN didn't end up using it as intended.
>>>>>> See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
>>>>>
>>>>> I don't see how that's relevant? That link discusses the error
>>>>> dialogs.
>>>>> What I'm asking is whether TSVN has implemented an
>>>>> svn_error_malfunction_handler_t that replaces the default handler,
>>>>> svn_error_abort_on_malfunction().
>>>>
>>>> Yes it does (called svn_error_handle_malfunction()) but it calls
>>>> abort()
>>>> which means the explorer crashes:
>>>> http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
>>>
>>> And they can't remove the abort() call because...?
>>
>> As Stefan explained, he cannot rely on the internal state of the
>> application if he gets an assertion thrown out of the svn libraries.
>>
>> And he has a point. He assumes that asserts are fatal and
>> non-recoverable
>> errors. What if Subversion asserts because it just trashed the entire
>> stack and heap space of the windows explorer and happens to operate on
>> garbaga data that triggers an assertion? It doesn't make any sense to
>> try
>> to continue.
>>
>> Now, we know that most of our assertions are due to bugs in our code
>> where, for instance, invalid data entered wc.db. But an application
>> using the svn libraries cannot safely rely on this knowledge.
>> Changing all these bogus asserts that trigger just because of invalid
>> working copy meta-data into proper errors is the right way to fix this.
>
> I think I have to explain something here:
> First, the shell extension of TSVN does not even call
> svn_error_set_malfunction_handler() to set its own handler. Since that
> function clearly states that "This function must be called in a
> single-threaded context", I can not initialize that in the shell
> extension since the extension can get loaded in multi threaded
> situations.
> And the default handler asserts and calls abort(). So the whole shell
> goes down in such situations.
>
> Then: the reason I call abort() in my own handler for TortoiseProc
> (the UI part of TSVN) is that the SVN also clearly state that "If
> can_return is false the method should never return. (The caller will
> call abort())".
> So what else than call abort() can I do? I can not return from that
> function if can_return is false. No way to exit but to call abort().
> So how is that "not implemented as intended"???
> At least now in the UI part I can show a nasty dialog box telling the
> users where to report the problem to. That way it's not me that gets
> bothered with these since I can't do anything about them anyway.
>
> I have _repeatedly_ mentioned on this list that this kind of "error
> handling" (quotes intended) is bad. Sure, it won't affect the CL
> client that much, but for a library that's just not acceptable.
> But others have similar error handling implemented:
> http://thedailywtf.com/Articles/Serious-Failure.aspx
Ranting is all very well, but I've yet to hear a suggestion from you
about how the libraries should handle unrecoverable errors. Like, for
example, the case where wc.db contains inconsistent and/or invalid data.
-- Brane
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Konstantin Kolinko <kn...@gmail.com>.
2011/12/23 Stefan Küng <to...@gmail.com>:
> On 23.12.2011 15:18, Stefan Sperling wrote:
>>
>> On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
>>>
>>> On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
>>>>
>>>> On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
>>>>>
>>>>> On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
>>>>>>
>>>>>> On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
>>>>>>>
>>>>>>> stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
>>>>>>>>
>>>>>>>> + An informative error message helps people more than an assert
>>>>>>>> (which
>>>>>>>> + in case of TSVN can mean crashing the Windows Explorer).
>>>>>>>
>>>>>>>
>>>>>>> I thought the svn_error_malfunction_handler_t stuff addressed that
>>>>>>> issue?
>>>>>>
>>>>>>
>>>>>> No, because TSVN didn't end up using it as intended.
>>>>>> See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
>>>>>
>>>>>
>>>>> I don't see how that's relevant? That link discusses the error
>>>>> dialogs.
>>>>> What I'm asking is whether TSVN has implemented an
>>>>> svn_error_malfunction_handler_t that replaces the default handler,
>>>>> svn_error_abort_on_malfunction().
>>>>
>>>>
>>>> Yes it does (called svn_error_handle_malfunction()) but it calls abort()
>>>> which means the explorer crashes:
>>>> http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
>>>
>>>
>>> And they can't remove the abort() call because...?
>>
>>
>> As Stefan explained, he cannot rely on the internal state of the
>> application if he gets an assertion thrown out of the svn libraries.
>>
>> And he has a point. He assumes that asserts are fatal and non-recoverable
>> errors. What if Subversion asserts because it just trashed the entire
>> stack and heap space of the windows explorer and happens to operate on
>> garbaga data that triggers an assertion? It doesn't make any sense to try
>> to continue.
>>
>> Now, we know that most of our assertions are due to bugs in our code
>> where, for instance, invalid data entered wc.db. But an application
>> using the svn libraries cannot safely rely on this knowledge.
>> Changing all these bogus asserts that trigger just because of invalid
>> working copy meta-data into proper errors is the right way to fix this.
>
>
> I think I have to explain something here:
> First, the shell extension of TSVN does not even call
> svn_error_set_malfunction_handler() to set its own handler. Since that
> function clearly states that "This function must be called in a
> single-threaded context", I can not initialize that in the shell extension
> since the extension can get loaded in multi threaded situations.
> And the default handler asserts and calls abort(). So the whole shell goes
> down in such situations.
>
Please excuse my naive questions, coming from outside
That "must be called in a single-threaded context" in the doc - is it
actually a usage constraint? Maybe it is just wrong documentation?
Looking at the code of that method in error.c, it just replaces a
value of a static variable with a new one and returns the old value.
Is that unsafe?
[1] http://svn.apache.org/viewvc/subversion/branches/1.7.x/subversion/libsvn_subr/error.c?view=markup#l656
Other question is what to do in such a malfunction handler method.
Can one use Windows exception handling [2][3] ? or C's longjmp?
[2] http://msdn.microsoft.com/en-us/library/swezty51%28v=vs.80%29.aspx
[3] http://msdn.microsoft.com/en-us/library/windows/desktop/ms680552%28v=vs.85%29.aspx
Does SVN library start its own threads for a single API call?
(Returning from the same thread is doable, but how does one return
from a different thread started from the main one?)
I suppose all of the above is not compatible with APR library, and I
would expect leaked resources, but it might be better than a crash.
>
> Then: the reason I call abort() in my own handler for TortoiseProc (the UI
> part of TSVN) is that the SVN also clearly state that "If can_return is
> false the method should never return. (The caller will call abort())".
> So what else than call abort() can I do? I can not return from that function
> if can_return is false. No way to exit but to call abort().
> So how is that "not implemented as intended"???
> At least now in the UI part I can show a nasty dialog box telling the users
> where to report the problem to. That way it's not me that gets bothered with
> these since I can't do anything about them anyway.
>
> I have _repeatedly_ mentioned on this list that this kind of "error
> handling" (quotes intended) is bad. Sure, it won't affect the CL client that
> much, but for a library that's just not acceptable.
> But others have similar error handling implemented:
> http://thedailywtf.com/Articles/Serious-Failure.aspx
>
>
Best regards,
Konstantin Kolinko
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Küng <to...@gmail.com>.
On 23.12.2011 15:18, Stefan Sperling wrote:
> On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
>> On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
>>> On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
>>>> On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
>>>>> On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
>>>>>> stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
>>>>>>> + An informative error message helps people more than an assert (which
>>>>>>> + in case of TSVN can mean crashing the Windows Explorer).
>>>>>>
>>>>>> I thought the svn_error_malfunction_handler_t stuff addressed that issue?
>>>>>
>>>>> No, because TSVN didn't end up using it as intended.
>>>>> See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
>>>>
>>>> I don't see how that's relevant? That link discusses the error dialogs.
>>>> What I'm asking is whether TSVN has implemented an
>>>> svn_error_malfunction_handler_t that replaces the default handler,
>>>> svn_error_abort_on_malfunction().
>>>
>>> Yes it does (called svn_error_handle_malfunction()) but it calls abort()
>>> which means the explorer crashes:
>>> http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
>>
>> And they can't remove the abort() call because...?
>
> As Stefan explained, he cannot rely on the internal state of the
> application if he gets an assertion thrown out of the svn libraries.
>
> And he has a point. He assumes that asserts are fatal and non-recoverable
> errors. What if Subversion asserts because it just trashed the entire
> stack and heap space of the windows explorer and happens to operate on
> garbaga data that triggers an assertion? It doesn't make any sense to try
> to continue.
>
> Now, we know that most of our assertions are due to bugs in our code
> where, for instance, invalid data entered wc.db. But an application
> using the svn libraries cannot safely rely on this knowledge.
> Changing all these bogus asserts that trigger just because of invalid
> working copy meta-data into proper errors is the right way to fix this.
I think I have to explain something here:
First, the shell extension of TSVN does not even call
svn_error_set_malfunction_handler() to set its own handler. Since that
function clearly states that "This function must be called in a
single-threaded context", I can not initialize that in the shell
extension since the extension can get loaded in multi threaded situations.
And the default handler asserts and calls abort(). So the whole shell
goes down in such situations.
Then: the reason I call abort() in my own handler for TortoiseProc (the
UI part of TSVN) is that the SVN also clearly state that "If can_return
is false the method should never return. (The caller will call abort())".
So what else than call abort() can I do? I can not return from that
function if can_return is false. No way to exit but to call abort().
So how is that "not implemented as intended"???
At least now in the UI part I can show a nasty dialog box telling the
users where to report the problem to. That way it's not me that gets
bothered with these since I can't do anything about them anyway.
I have _repeatedly_ mentioned on this list that this kind of "error
handling" (quotes intended) is bad. Sure, it won't affect the CL client
that much, but for a library that's just not acceptable.
But others have similar error handling implemented:
http://thedailywtf.com/Articles/Serious-Failure.aspx
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Fri, Dec 23, 2011 at 15:18:05 +0100:
> On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
> > On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
> > > On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> > > > On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> > > > > On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > > > > > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > > > > > + An informative error message helps people more than an assert (which
> > > > > > > + in case of TSVN can mean crashing the Windows Explorer).
> > > > > >
> > > > > > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
> > > > >
> > > > > No, because TSVN didn't end up using it as intended.
> > > > > See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
> > > >
> > > > I don't see how that's relevant? That link discusses the error dialogs.
> > > > What I'm asking is whether TSVN has implemented an
> > > > svn_error_malfunction_handler_t that replaces the default handler,
> > > > svn_error_abort_on_malfunction().
> > >
> > > Yes it does (called svn_error_handle_malfunction()) but it calls abort()
> > > which means the explorer crashes:
> > > http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
> >
> > And they can't remove the abort() call because...?
>
> As Stefan explained, he cannot rely on the internal state of the
> application if he gets an assertion thrown out of the svn libraries.
>
> And he has a point. He assumes that asserts are fatal and non-recoverable
> errors. What if Subversion asserts because it just trashed the entire
> stack and heap space of the windows explorer and happens to operate on
> garbaga data that triggers an assertion? It doesn't make any sense to try
> to continue.
Why do we ever set CAN_RETURN to TRUE when calling the handler, then?
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Fri, Dec 23, 2011 at 15:18:05 +0100:
> On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
> > On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
> > > On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> > > > On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> > > > > On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > > > > > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > > > > > + An informative error message helps people more than an assert (which
> > > > > > > + in case of TSVN can mean crashing the Windows Explorer).
> > > > > >
> > > > > > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
> > > > >
> > > > > No, because TSVN didn't end up using it as intended.
> > > > > See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
> > > >
> > > > I don't see how that's relevant? That link discusses the error dialogs.
> > > > What I'm asking is whether TSVN has implemented an
> > > > svn_error_malfunction_handler_t that replaces the default handler,
> > > > svn_error_abort_on_malfunction().
> > >
> > > Yes it does (called svn_error_handle_malfunction()) but it calls abort()
> > > which means the explorer crashes:
> > > http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
> >
> > And they can't remove the abort() call because...?
>
> As Stefan explained, he cannot rely on the internal state of the
> application if he gets an assertion thrown out of the svn libraries.
>
> And he has a point. He assumes that asserts are fatal and non-recoverable
> errors. What if Subversion asserts because it just trashed the entire
> stack and heap space of the windows explorer and happens to operate on
> garbaga data that triggers an assertion? It doesn't make any sense to try
> to continue.
Why do we ever set CAN_RETURN to TRUE when calling the handler, then?
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
> On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
> > On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> > > On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> > > > On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > > > > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > > > > + An informative error message helps people more than an assert (which
> > > > > > + in case of TSVN can mean crashing the Windows Explorer).
> > > > >
> > > > > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
> > > >
> > > > No, because TSVN didn't end up using it as intended.
> > > > See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
> > >
> > > I don't see how that's relevant? That link discusses the error dialogs.
> > > What I'm asking is whether TSVN has implemented an
> > > svn_error_malfunction_handler_t that replaces the default handler,
> > > svn_error_abort_on_malfunction().
> >
> > Yes it does (called svn_error_handle_malfunction()) but it calls abort()
> > which means the explorer crashes:
> > http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
>
> And they can't remove the abort() call because...?
As Stefan explained, he cannot rely on the internal state of the
application if he gets an assertion thrown out of the svn libraries.
And he has a point. He assumes that asserts are fatal and non-recoverable
errors. What if Subversion asserts because it just trashed the entire
stack and heap space of the windows explorer and happens to operate on
garbaga data that triggers an assertion? It doesn't make any sense to try
to continue.
Now, we know that most of our assertions are due to bugs in our code
where, for instance, invalid data entered wc.db. But an application
using the svn libraries cannot safely rely on this knowledge.
Changing all these bogus asserts that trigger just because of invalid
working copy meta-data into proper errors is the right way to fix this.
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Dec 23, 2011 at 04:10:07PM +0200, Daniel Shahaf wrote:
> On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
> > On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> > > On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> > > > On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > > > > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > > > > + An informative error message helps people more than an assert (which
> > > > > > + in case of TSVN can mean crashing the Windows Explorer).
> > > > >
> > > > > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
> > > >
> > > > No, because TSVN didn't end up using it as intended.
> > > > See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
> > >
> > > I don't see how that's relevant? That link discusses the error dialogs.
> > > What I'm asking is whether TSVN has implemented an
> > > svn_error_malfunction_handler_t that replaces the default handler,
> > > svn_error_abort_on_malfunction().
> >
> > Yes it does (called svn_error_handle_malfunction()) but it calls abort()
> > which means the explorer crashes:
> > http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
>
> And they can't remove the abort() call because...?
As Stefan explained, he cannot rely on the internal state of the
application if he gets an assertion thrown out of the svn libraries.
And he has a point. He assumes that asserts are fatal and non-recoverable
errors. What if Subversion asserts because it just trashed the entire
stack and heap space of the windows explorer and happens to operate on
garbaga data that triggers an assertion? It doesn't make any sense to try
to continue.
Now, we know that most of our assertions are due to bugs in our code
where, for instance, invalid data entered wc.db. But an application
using the svn libraries cannot safely rely on this knowledge.
Changing all these bogus asserts that trigger just because of invalid
working copy meta-data into proper errors is the right way to fix this.
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
> On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> > On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> > > On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > > > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > > > + An informative error message helps people more than an assert (which
> > > > > + in case of TSVN can mean crashing the Windows Explorer).
> > > >
> > > > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
> > >
> > > No, because TSVN didn't end up using it as intended.
> > > See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
> >
> > I don't see how that's relevant? That link discusses the error dialogs.
> > What I'm asking is whether TSVN has implemented an
> > svn_error_malfunction_handler_t that replaces the default handler,
> > svn_error_abort_on_malfunction().
>
> Yes it does (called svn_error_handle_malfunction()) but it calls abort()
> which means the explorer crashes:
> http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
And they can't remove the abort() call because...?
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Fri, Dec 23, 2011, at 14:50, Stefan Sperling wrote:
> On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> > On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> > > On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > > > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > > > + An informative error message helps people more than an assert (which
> > > > > + in case of TSVN can mean crashing the Windows Explorer).
> > > >
> > > > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
> > >
> > > No, because TSVN didn't end up using it as intended.
> > > See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
> >
> > I don't see how that's relevant? That link discusses the error dialogs.
> > What I'm asking is whether TSVN has implemented an
> > svn_error_malfunction_handler_t that replaces the default handler,
> > svn_error_abort_on_malfunction().
>
> Yes it does (called svn_error_handle_malfunction()) but it calls abort()
> which means the explorer crashes:
> http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
And they can't remove the abort() call because...?
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> > On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > > + An informative error message helps people more than an assert (which
> > > > + in case of TSVN can mean crashing the Windows Explorer).
> > >
> > > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
> >
> > No, because TSVN didn't end up using it as intended.
> > See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
>
> I don't see how that's relevant? That link discusses the error dialogs.
> What I'm asking is whether TSVN has implemented an
> svn_error_malfunction_handler_t that replaces the default handler,
> svn_error_abort_on_malfunction().
Yes it does (called svn_error_handle_malfunction()) but it calls abort()
which means the explorer crashes:
http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Dec 23, 2011 at 02:26:40PM +0200, Daniel Shahaf wrote:
> On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> > On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > > + An informative error message helps people more than an assert (which
> > > > + in case of TSVN can mean crashing the Windows Explorer).
> > >
> > > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
> >
> > No, because TSVN didn't end up using it as intended.
> > See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
>
> I don't see how that's relevant? That link discusses the error dialogs.
> What I'm asking is whether TSVN has implemented an
> svn_error_malfunction_handler_t that replaces the default handler,
> svn_error_abort_on_malfunction().
Yes it does (called svn_error_handle_malfunction()) but it calls abort()
which means the explorer crashes:
http://tortoisesvn.googlecode.com/svn/trunk/src/SVN/SVN.cpp
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > + An informative error message helps people more than an assert (which
> > > + in case of TSVN can mean crashing the Windows Explorer).
> >
> > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
>
> No, because TSVN didn't end up using it as intended.
> See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
I don't see how that's relevant? That link discusses the error dialogs.
What I'm asking is whether TSVN has implemented an
svn_error_malfunction_handler_t that replaces the default handler,
svn_error_abort_on_malfunction().
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
On Fri, Dec 23, 2011, at 13:01, Stefan Sperling wrote:
> On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> > stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > > + An informative error message helps people more than an assert (which
> > > + in case of TSVN can mean crashing the Windows Explorer).
> >
> > I thought the svn_error_malfunction_handler_t stuff addressed that issue?
>
> No, because TSVN didn't end up using it as intended.
> See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
I don't see how that's relevant? That link discusses the error dialogs.
What I'm asking is whether TSVN has implemented an
svn_error_malfunction_handler_t that replaces the default handler,
svn_error_abort_on_malfunction().
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > + An informative error message helps people more than an assert (which
> > + in case of TSVN can mean crashing the Windows Explorer).
>
> I thought the svn_error_malfunction_handler_t stuff addressed that issue?
No, because TSVN didn't end up using it as intended.
See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Dec 23, 2011 at 01:48:18PM +0200, Daniel Shahaf wrote:
> stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> > + An informative error message helps people more than an assert (which
> > + in case of TSVN can mean crashing the Windows Explorer).
>
> I thought the svn_error_malfunction_handler_t stuff addressed that issue?
No, because TSVN didn't end up using it as intended.
See bottom of http://svn.haxx.se/users/archive-2011-10/0378.shtml
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> + An informative error message helps people more than an assert (which
> + in case of TSVN can mean crashing the Windows Explorer).
I thought the svn_error_malfunction_handler_t stuff addressed that issue?
Re: svn commit: r1222522 - /subversion/branches/1.7.x/STATUS
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
stsp@apache.org wrote on Fri, Dec 23, 2011 at 00:35:16 -0000:
> + An informative error message helps people more than an assert (which
> + in case of TSVN can mean crashing the Windows Explorer).
I thought the svn_error_malfunction_handler_t stuff addressed that issue?