You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/08/28 16:02:49 UTC
Re: svn commit: r32790 - in trunk/subversion: libsvn_wc tests/cmdline
danielsh@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 <da...@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.
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?
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.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32790 - in trunk/subversion: libsvn_wc tests/cmdline
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Shahaf <d....@daniel.shahaf.name> writes:
>> > "clear" not "clean" :-)
>> >
>>
>> r32796.
>>
>
> And r32798.
:-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Re: Re: svn commit: r32790 - in trunk/subversion: libsvn_wc
tests/cmdline
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
> > "clear" not "clean" :-)
> >
>
> r32796.
>
And r32798.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32790 - in trunk/subversion: libsvn_wc tests/cmdline
Posted by Karl Fogel <kf...@red-bean.com>.
danielsh@tigris.org writes:
> Karl Fogel wrote on Thu, 28 Aug 2008 at 13:15 -0400:
>> danielsh@tigris.org writes:
>> > 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,
>> >
>> > ?
>>
>> "clear" not "clean" :-)
>>
>
> r32796.
But doesn't r32796 still have the "clear" vs "clean" typo?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Re: svn commit: r32790 - in trunk/subversion: libsvn_wc
tests/cmdline
Posted by da...@tigris.org.
Karl Fogel wrote on Thu, 28 Aug 2008 at 13:15 -0400:
> danielsh@tigris.org writes:
> > 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,
> >
> > ?
>
> "clear" not "clean" :-)
>
r32796.
Thanks,
Daniel
> >> 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:
>
> Okay. I guess that means when error A is a child of error B, then
> A->pool == B->pool. Then the implementation makes sense.
>
> -K
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32790 - in trunk/subversion: libsvn_wc tests/cmdline
Posted by Karl Fogel <kf...@red-bean.com>.
danielsh@tigris.org writes:
> 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,
>
> ?
"clear" not "clean" :-)
>> 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:
Okay. I guess that means when error A is a child of error B, then
A->pool == B->pool. Then the implementation makes sense.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32790 - in trunk/subversion: libsvn_wc
tests/cmdline
Posted by da...@tigris.org.
Karl Fogel wrote on Thu, 28 Aug 2008 at 12:02 -0400:
> danielsh@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 <da...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org