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()?
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.
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.
void
svn_error_clear(svn_error_t *err)
{
if (err)
{
#if defined(SVN_DEBUG)
while (err->child)
err = err->child;
apr_pool_cleanup_kill(err->pool, err, err_abort);
#endif
svn_pool_destroy(err->pool);
}
}
Blair
