On 12/22/10 10:56 AM, Daniel Shahaf wrote:
Blair Zajac wrote on Wed, Dec 22, 2010 at 10:45:12 -0800:
On 12/22/10 10:27 AM, Daniel Shahaf wrote:
Blair Zajac wrote on Wed, Dec 22, 2010 at 09:02:34 -0800:
On 12/22/10 5:47 AM, Daniel Shahaf wrote:
bl...@apache.org wrote on Wed, Dec 22, 2010 at 05:46:45 -0000:
Author: blair
Date: Wed Dec 22 05:46:45 2010
New Revision: 1051763
URL: http://svn.apache.org/viewvc?rev=1051763&view=rev
Log:
Modified: subversion/trunk/subversion/libsvn_repos/commit.c
URL:
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1051763&r1=1051762&r2=1051763&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_repos/commit.c (original)
+++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Dec 22 05:46:45 2010
@@ -644,7 +644,56 @@ change_dir_prop(void *dir_baton,
name, value, pool);
}
+const char *
+svn_repos__post_commit_error_str(svn_error_t *err,
+ apr_pool_t *pool)
+{
+ svn_error_t *hook_err1, *hook_err2;
+
+ if (! err)
+ return "(no error)";
+
+ err = svn_error_purge_tracing(err);
This modifies the provided error. Either the doc string should warn
that this is being done (i.e., the provided ERR is not safe for use
after calling this helper), or this call should be removed and another
means used to locate the first non-tracing error message.
Where do you clear ERR? You must do so in this function, since the
caller's ERR may not be cleared (according to svn_error_purge_tracing()'s
promise).
Is this promise a defensive promise, as the original err still looks
usable by reviewing svn_error_purge_tracing()'s implementation? It looks
like only one of the errors should ever be cleared and it doesn't matter
which one. True, it's conceptually easier to remember that one should
only clear the error returned from svn_error_purge_tracing().
I hadn't actually examined the implementation at the time I wrote that
email, so now I'm confused.
svn_error_purge_tracing() returns a chain which is a subset of the
original chain. (The subset need not start at the same place as the
original chain, eg in the common case that the first entry in the
original chain is a tracing link.)
The reason for clearing errors is to free() the memory and deinstall the
err_abort() pool cleanup callback. Since svn_error_purge_tracing()
doesn't free() or deinstall the callback, it would seem that one has to
clear the ORIGINAL (passed-in) error chain, not the returned one!
Even this isn't guarenteed to work, if the error at the end of the chain
is a tracing link, right, since it can be removed by
svn_error_purge_tracing()?
Ah, right. Once you call svn_error_purge_tracing(), if the chain
contains a tracing link at any place other than the first position, then
all pointers to that link would be lost.
(But if this is indeed a bug, then how come it hasn't been noticed yet?)
I think it only matters if the last error in the chain is a tracing error,
because the removal of err_abort() from the pool has to use the exact values
that were used to register err_abort(), which is the last error in the chain.
Since probably 100% of the time the last error in a chain is not a tracing
error, then err_abort() will be properly deregistered by svn_error_clear().
It seems the best thing is to have svn_error_purge_tracing() return a
brand new chain that shares no links with the original chain, and then
both need to be freed.
That makes sense; if we'll work with complete chains, there's a smaller
chance of more edge-case bugs.
Thinking on this a little more, since most people won't use debugging, it'll be
wasteful to have svn_error_purge_tracing() make a duplicate chain that will have
no tracing errors in it.
So maybe we should have svn_error_purge_tracing() be related to the input chain
and share what it can, but not be an error chain one should use
svn_error_clear() on.
Separately, it seems that svn_error_dup() installs a pool cleanup
callback only on one link in the duplicate chain, rather than on every
link. (whereas make_error_internal() causes every link of a 'normal'
chain to have a cleanup attached)
make_error_internal() only installs a callback on the last error in the chain,
that is if "child" is NULL.
For example, shouldn't the following code trigger err_abort() on the
outer (wrapper) link?
svn_error_t *in = svn_error_create(...);
svn_error_t *original = svn_error_quick_wrap(in, SVN_ERR__TRACED);
svn_error_t *purged = svn_error_purge_tracing(original);
svn_error_clear(purged);
exit(0);
Am I making sense?
I don't think so, because if SVN_DEBUG is defined, then it finds the
error at the end of the chain, which in this example is a non-tracing
error, and it'll properly remove the err_abort.
You're right, I'll modify my example to this:
SVN_ERR__TRACED /* start of chain */
SVN_ERR__TRACED
"non-tracing link" /* end of chain */
In this case, the middle link would be lost. Right?
Both SVN_ERR__TRACED would be lost by svn_error_purge_tracing(), right?
Blair
