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