On Saturday 06 December 2003 21:21, Julian Foad wrote:
> John Szakmeister wrote:
> > On Tuesday 02 December 2003 22:20, Julian Foad wrote:
> >>It does catch a few other instances of using an adm_access baton after it
> >>was closed. The attached patch fixes all but one tricky case, marked
> >> with "###". Do you fancy trying to see how to deal with that tricky
> >> case?
> >
> > Well, I spent some time trying to fix the tricky case in
> > svn_wc_process_committed(). I've managed to get nearly everything
> > working with the attached patch. Only copy test #9 and svnlook test #2
> > fail and I think for the same reason. It appears that this line is no
> > longer filtering out the already committed file "A/B2/E/alpha" in
> > svn_client_commit(), and tries to run svn_wc_process_committed() on it
> > again, causing it to fail.
> >
> > if (! entry
> > && have_processed_parent (commit_items, i, item->path, subpool))
> > /* This happens when the item is a file that is deleted, and it
> > has been processed as a child of an earlier item. */
> > continue;
>
> Hmm, I haven't seen a problem with this bit of code. I will look for the
> effect you describe later, but in the meantime I've just concentrated on
> the body of svn_wc_process_committed().
The problem was that it was acting on the the committed item first before
acting on it's children. So, I figured we should change the approach and
take care of the children first, then we can act on the item and not have to
worry about whether or not it closes adm_access.
> I see in your version of svn_wc_process_committed that you swap the order
> of processing and recursing. I'm afraid I am not competent to comment on
> that. I have had another look and I have a simple idea that works: don't
> recurse if the adm_access baton has been closed.
>
> /* Recurse if required, unless we have just removed the entire directory.
> */ if (recurse && svn_wc_adm_locked (adm_access))
>
> This works because if the directory was removed,
> svn_wc_remove_from_revision_control->svn_wc__adm_destroy->svn_wc_adm_close
> will have marked the access baton as not having a write lock. (Actually,
> the status is explicitly set to "closed" but my zeroing it out interferes
> with that and changes the status to "unlocked". Either way is sufficient
> for this to work, but it is unclean.)
Good idea!
> I think this patch now fixes the used-after-closed errors: all tests pass
> on Linux over RA-local and RA-svn. The only question I have is whether we
> want the baton-invalidation step to be checked in. If so, should it set
> various fields individually instead of doing a "memset"?
Oof, good question. I personally like it because it forces us as developers
to adhere to the API. I'm not sure it useful outside of that though. I'm in
favor of the memset() since if the baton changes for some reason, it's less
code to update.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Dec 7 04:34:56 2003