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

Re: svn commit: r32790 - in trunk/subversion: libsvn_wc tests/cmdline

From: <danielsh_at_tigris.org>
Date: Thu, 28 Aug 2008 19:40:37 +0300 (IDT)

Karl Fogel wrote on Thu, 28 Aug 2008 at 12:02 -0400:
> danielsh_at_tigris.org writes:
> > Log:
> > Fix issue #2505: make switch continue after deleting locally modified
> > directories, as it update and merge do.
> >
> > Patch by: Danil Shopyrin <danil.shopyrin_at_gmail.com>
> > (Log message tweaked by me.)
> >
> > * subversion/libsvn_wc/update_editor.c
> > (leftmod_error_chain): Trap and ignore SVN_ERR_WC_LEFT_LOCAL_MOD, fixing
> > a potential error leak. Don't delete logfile in that case.
> > (do_entry_deletion): Call svn_wc_remove_from_revision_control()
> > with instant_error = FALSE.
> >
> > * subversion/tests/cmdline/switch_tests.py
> > (tolerate_local_mods): New test.
> > (test_list): Run the new test.
>
> Sorry not to have offered the review below before the patch was
> applied -- I didn't get a chance to look until now.
>
> > --- trunk/subversion/libsvn_wc/update_editor.c
> > +++ trunk/subversion/libsvn_wc/update_editor.c
> > @@ -1092,19 +1092,16 @@ leftmod_error_chain(svn_error_t *err,
> > if (tmp_err->apr_err == SVN_ERR_WC_LEFT_LOCAL_MOD)
> > break;
> >
> > - /* If we found a "left a local mod" error, wrap and return it.
> > - Otherwise, we just return our top-most error. */
> > + /* If we found a "left a local mod" error, tolerate it
> > + and clear the whole error. In that case we continue with
> > + modified files left on the disk. */
> > if (tmp_err)
> > {
> > - /* Remove the LOGFILE (and eat up errors from this process). */
> > - svn_error_clear(svn_io_remove_file(logfile, pool));
> > -
> > - return svn_error_createf
> > - (SVN_ERR_WC_OBSTRUCTED_UPDATE, tmp_err,
> > - _("Won't delete locally modified directory '%s'"),
> > - svn_path_local_style(path, pool));
> > + svn_error_clear(err);
> > + return SVN_NO_ERROR;
> > }
> >
> > + /* Otherwise, we just return our top-most error. */
> > return err;
> > }
>
> The doc string for leftmod_error_chain() should explain that it can
> result in the 'err' argument being cleared.
>

And it also needs to say that LOGFILE won't be cleared. Suggested
patch:

Index: subversion/libsvn_wc/update_editor.c
===================================================================
--- subversion/libsvn_wc/update_editor.c (revision 32790)
+++ subversion/libsvn_wc/update_editor.c (working copy)
@@ -1069,11 +1069,10 @@ open_root(void *edit_baton,
 }
 
 
-/* Helper for delete_entry().
+/* Helper for delete_entry() and do_entry_deletion().
 
- Search an error chain (ERR) for evidence that a local mod was left.
- If so, cleanup LOGFILE and return an appropriate error. Otherwise,
- just return the original error chain.
+ If the error chain ERR contains evidence that a local mod was left
+ (an SVN_ERR_WC_LEFT_LOCAL_MOD error), clean ERR. Otherwise, return ERR.
 */
 static svn_error_t *
 leftmod_error_chain(svn_error_t *err,

?

> But also, 'err' is just the top of a possibly multi-error chain. Here
> we're clear 'err', but not any of the other down errors (which could be
> before or after 'err' in the chain). Shouldn't we clear everyone in the
> chain?
>

The docstring of svn_error_clear() says it clears everyone in the chain:

        /** Free the memory used by @a error, as well as all ancestors and
         * descendants of @a error.
         *
         * Unlike other Subversion objects, errors are managed explicitly; you
         * MUST clear an error if you are ignoring it, or you are leaking memory.
         * For convenience, @a error may be @c NULL, in which case this function does
         * nothing; thus, svn_error_clear(svn_foo(...)) works as an idiom to
         * ignore errors.
         */
        void
        svn_error_clear(svn_error_t *err);

> See the definition of svn_error_clear(), by the way, which behaves very
> differently when SVN_DEBUG is defined, for reasons I don't understand.
>

I think it has to do with the maintainer-mode logic to abort if an error
is leaked. Someone who knows that area better can clarify.

For reference:

        void
        svn_error_clear(svn_error_t *err)
        {
          if (err)
                {
        #if defined(SVN_DEBUG)
                  while (err->child)
                        err = err->child;
                  apr_pool_cleanup_kill(err->pool, err, err_abort);
        #endif
                  svn_pool_destroy(err->pool);
                }
        }

> -Karl
>

Thanks for the review!

Daniel

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-28 18:40:05 CEST

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