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

Re: Problem in copy_versioned_files()?

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2003-12-02 23:45:49 CET

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

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.