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:
>> 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().
>>
>
> Agreed. As to the 'lost' links (those not pointed to any more),
> I believe they're allocated from the same pool as every other link in
> the chain, so they'll be freed when that chain is cleared.
Yes.
>>>> 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().
>>>>> 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
Received on 2010-12-23 07:26:59 CET