[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn_error_purge_tracing() and clearing the errors Re: svn commit: r1051763 - in /subversion/trunk/subversion: include/private/svn_repos_private.h libsvn_repos/commit.c mod_dav_svn/version.c

From: Blair Zajac <blair_at_orcaware.com>
Date: Wed, 22 Dec 2010 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:
>> 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

This is an archived mail posted to the Subversion Dev mailing list.