Blair Zajac wrote on Wed, Dec 22, 2010 at 22:26:15 -0800:
> On 12/22/10 4:38 PM, Daniel Shahaf wrote:
>> Blair Zajac wrote on Wed, Dec 22, 2010 at 13:22:11 -0800:
>>> On 12/22/10 10:56 AM, Daniel Shahaf wrote:
>>>> Blair Zajac wrote on Wed, Dec 22, 2010 at 10:45:12 -0800:
>>>>> 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.
>>>
>>
>> IOW, duplicating in maintainer mode forces duplicating in non-maintainer
>> mode too, making the common case costlier. True.
>>
>> However, we could have a macro that causes the dup() operation to only
>> be done in maintainer mode:
>>
>> void svn_error_purge_tracing_shell(svn_error_t **err_p)
>> {
>> #ifdef SVN_ERR__TRACING
>> svn_error_t *err = svn_error_purge_tracing(*err_p);
>> svn_error_clear(*err_p);
>> *err_p = err;
>> #endif /* SVN_ERR__TRACING */
>> }
>>
>>> 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.
>>>
>>
>> IOW, change its promise such that it's the passed-in error, not the
>> returned one, which should be cleared? And have it leave the passed-in
>> chain untouched?
>>
>> It makes sense on a first scan, but at 3am it's too late for a full +1.
>
> I've attached a patch to review. It fixes both svn_error_purge_tracing()
> and the the behavior of svn_repos__post_commit_error_str().
>
Review below.
>>>>>> 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?
>>>
>>
>> Won't the inner while() loop would skip over them?
>
> Yes, I think it would. We should probably write a unit test for
> svn_error_purge_tracing().
>
> Blair
I've made a quick sketch --- looks good?
(I'll fix the "###" comment before committing.)
[[[
Index: subversion/tests/libsvn_subr/error-test.c
===================================================================
--- subversion/tests/libsvn_subr/error-test.c (revision 1052215)
+++ subversion/tests/libsvn_subr/error-test.c (working copy)
@@ -78,6 +78,34 @@ test_error_root_cause(apr_pool_t *pool)
return SVN_NO_ERROR;
}
+/* ### just make ../../libsvn_subr/error.c:is_tracing_link() semi-public */
+#ifdef SVN_ERR__TRACING
+#define is_tracing_link(err) \
+ ((err) && (err)->message && !strcmp((err)->message,
SVN_ERR__TRACED))
+#else
+#define is_tracing_link(err) FALSE
+#endif
+
+static svn_error_t *
+test_error_purge_tracing(apr_pool_t *pool)
+{
+ svn_error_t *err, *err2, *child;
+
+ err = svn_error_quick_wrap(svn_error_create(SVN_ERR_BASE, NULL, "root
error"),
+ "wrapped");
+ err = svn_error_quick_wrap(svn_error_create(SVN_ERR_BASE, err, "other
error"),
+ "re-wrapped");
+
+ err2 = svn_error_purge_tracing(err);
+ for (child = err2; child; child = child->child)
+ if (is_tracing_link(child))
+ return svn_error_create(SVN_ERR_TEST_FAILED, err,
+ "Tracing link found after purging the following
chain:");
+
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+}
+
/* The test table. */
@@ -86,5 +114,7 @@ struct svn_test_descriptor_t test_funcs[] =
SVN_TEST_NULL,
SVN_TEST_PASS2(test_error_root_cause,
"test svn_error_root_cause"),
+ SVN_TEST_PASS2(test_error_purge_tracing,
+ "test svn_error_purge_tracing"),
SVN_TEST_NULL
};
]]]
> Index: subversion/libsvn_subr/error.c
> ===================================================================
> --- subversion/libsvn_subr/error.c (revision 1052167)
> +++ subversion/libsvn_subr/error.c (working copy)
> @@ -349,39 +349,47 @@
> svn_error_purge_tracing(svn_error_t *err)
> {
> #ifdef SVN_ERR__TRACING
...
> return new_err;
> #else /* SVN_ERR__TRACING */
> return err;
+1
> Index: subversion/mod_dav_svn/deadprops.c
> ===================================================================
> --- subversion/mod_dav_svn/deadprops.c (revision 1052167)
> +++ subversion/mod_dav_svn/deadprops.c (working copy)
> @@ -222,18 +222,22 @@
> + purged_serr->message = apr_xml_quote_string
> + (purged_serr->pool,
Our current policy is to have the '(' on the same line as the function name.
> Index: subversion/mod_dav_svn/version.c
> ===================================================================
> --- subversion/mod_dav_svn/version.c (revision 1052167)
> +++ subversion/mod_dav_svn/version.c (working copy)
> @@ -922,16 +922,16 @@
> {
> if (serr)
> {
> - const char *post_commit_err;
> - apr_err = serr->apr_err;
> - post_commit_err = svn_repos__post_commit_error_str
> - (serr, resource->pool);
> - serr = SVN_NO_ERROR;
> - ap_log_perror(APLOG_MARK, APLOG_ERR, apr_err, resource->pool,
> + const char *post_commit_err = svn_repos__post_commit_error_str
> + (serr, resource->pool);
> + ap_log_perror(APLOG_MARK, APLOG_ERR, serr->apr_err,
> + resource->pool,
The rest looks good. (I am assuming --- didn't check --- that you
updated all callers.)
