John Szakmeister wrote:
> On Sunday 30 November 2003 12:55, Philip Martin wrote:
>
>>> err = svn_wc_entry (&entry, from, adm_access, FALSE, subpool);
>>> SVN_ERR (svn_wc_adm_close (adm_access));
>>> if (err)
>>> {
>>> if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
>>> return err;
>>> else
>>> svn_error_clear (err);
>>> }
[...]
>>> err = svn_wc_entry (&entry, copy_from, adm_access, FALSE,
>>> subpool);
>>>
>>>Is this safe? According to svn_wc_adm_close's docstring, "Any physical
>>>locks will be removed from the working copy. Lock removal is
>>>unconditional, there is no check to determine if cleanup is required."
>>>This sounds to me like we shouldn't be accessing the entry information
>>>after we've closed the WC.
>>
>>With the current code the entry information will remain until the pool
>>is cleared. I'm not sure that's something we want to guarantee, but I
>>suspect it's not going to change. It probably would be better if we
>>didn't access entry information after a baton is closed.
>
> I figured it would sit in the pool (otherwise I would have seen problems when
> the local export stuff), but it seems that this function is making use of an
> implementation detail that isn't explicitly documented, which is why I had
> cause for concern.
>
> I'll look at getting this all squared away so that we no longer do that in
> this function.
Is my patch that I have just sent in thread "Sub-pools for recursion" OK for this purpose? It also fixes the handling of errors which, in the original code above, is wrong in the case where svn_wc_adm_close returns an error.
I considered putting an "svn_wc_adm_close" in the error return path ("if (err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)"), but I don't think that's necessary.
I am also playing with the following patch:
Index: subversion/libsvn_wc/lock.c
===================================================================
--- subversion/libsvn_wc/lock.c (revision 7899)
+++ subversion/libsvn_wc/lock.c (working copy)
@@ -738,6 +738,9 @@
assert (! adm_access->set_owner || apr_hash_count (adm_access->set) == 0);
}
+ /* Invalidate the baton so that attempts to use it will be caught. */
+ memset (adm_access, 0, sizeof(*adm_access));
+
return SVN_NO_ERROR;
}
It catches the error that is the subject of this message, but makes lots of other tests fail too. I'm still investigating whether it is finding real bugs or is a wrong thing to do.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 2 23:43:36 2003