You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2012/05/04 03:46:42 UTC
Re: svn commit: r1333739 - /subversion/trunk/subversion/libsvn_delta/editor.c
Hyrum,
switch 29 and update 44 are failing, but that was due to my work in
r1332881. I need to get back and figure those out (first saw it on
ra_serf, but didn't realize until today it also affected ra_local).
The revision below does *not* yet verify driver-ordering of deletes vs
adds. I might need to collect more data for that. Dunno. The below was
just more assertions that I thought up given the existing data.
One fallout that I need to document: alter_*() should be called on
child nodes at the destination of a copy/delete. It is now "illegal"
to modify a child, then move its parent elsewhere. The proper order
is: move parent, then edit the child. (strictly speaking, it doesn't
check the full ancestry... just the immediate parent, but whatever...
this is still debug code to help us rather than full hard-core
verification)
(it is already illegal to edit a file and then move it; I just added a
check on the parent too)
Cheers,
-g
On Thu, May 3, 2012 at 9:39 PM, <gs...@apache.org> wrote:
> Author: gstein
> Date: Fri May 4 01:39:23 2012
> New Revision: 1333739
>
> URL: http://svn.apache.org/viewvc?rev=1333739&view=rev
> Log:
> Ev2 shims:
>
> The editor collects a bunch of data while in debug mode, to verify a
> bunch of constraints. There are a number of other constraints we can
> look for, so code up the assertions...
>
> * subversion/libsvn_delta/editor.c:
> (MARK_PARENT_STABLE): used to mark the parent of a node as needing
> to stay where it is (no delets, move, replace).
> (VERIFY_PARENT_MAY_EXIST): there are cases wen we know a parent
> doesn't exist (thus an operation on a child is not possible), and
> this macro can catch those.
> (CHILD_DELETIONS_ALLOWED): added directories, and directories moved
> away cannot possibly have children which can be deleted. catch
> these situations.
> (mark_parent_stable): helper function for the above macro
> (svn_editor_add_directory, svn_editor_add_file,
> svn_editor_add_symlink, svn_editor_add_absent,
> svn_editor_alter_directory, svn_editor_alter_file,
> svn_editor_alter_symlink): use VERIFY_PARENT_MAY_EXIST() to
> eliminate definitely-missing parents. use MARK_PARENT_STABLE() to
> deny structural changes to the parent, now that we've performed an
> operation on a child.
> (svn_editor_delete): ensure a parent might exist, and that deletions
> are allowed in this directory. use MARK_PARENT_STABLE() now that
> we've munged a child.
> (svn_editor_copy): verify the parents of the source and destination
> might exist. stabilize the parent of the destination.
> (svn_editor_move): verify the parents of the source and destination,
> and that deletions are allowed in the source directory. mark both
> source and destination as needing to remain stable.
> (svn_editor_create): verify all parents exist and that deletions are
> allowed. mark all new parents as needing to be stable.
>
> Modified:
> subversion/trunk/subversion/libsvn_delta/editor.c
>
> Modified: subversion/trunk/subversion/libsvn_delta/editor.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_delta/editor.c?rev=1333739&r1=1333738&r2=1333739&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_delta/editor.c (original)
> +++ subversion/trunk/subversion/libsvn_delta/editor.c Fri May 4 01:39:23 2012
> @@ -129,6 +129,36 @@ static const int marker_added_dir;
> #define CHECK_UNKNOWN_CHILD(editor, relpath) \
> SVN_ERR_ASSERT(check_unknown_child(editor, relpath))
>
> +/* When a child is changed in some way, mark the parent directory as needing
> + to be "stable" (no future structural changes). IOW, only allow "alter" on
> + the parent. Prevents parent-add/delete/move after any child operation. */
> +#define MARK_PARENT_STABLE(editor, relpath) \
> + mark_parent_stable(editor, relpath)
> +
> +/* If the parent is MARKER_ALLOW_ADD, then it has been moved-away, and we
> + know it does not exist. All other cases: it might exist. */
> +#define VERIFY_PARENT_MAY_EXIST(editor, relpath) \
> + SVN_ERR_ASSERT(apr_hash_get((editor)->completed_nodes, \
> + svn_relpath_dirname(relpath, \
> + (editor)->scratch_pool), \
> + APR_HASH_KEY_STRING) != MARKER_ALLOW_ADD)
> +
> +/* If the parent is MARKER_ADDED_DIR, then we should not be deleting
> + children(*). If the parent is MARKER_ALLOW_ADD, then it has been
> + moved-away, so children cannot exist. That leaves MARKER_DONE,
> + MARKER_ALLOW_ALTER, and NULL as possible values. Just assert that
> + we didn't get either of the bad ones.
> +
> + (*) if the child as added via add_*(), then it would have been marked
> + as completed and delete/move-away already test against completed nodes.
> + This test is to beware of trying to delete "children" that are not
> + actually (and can't possibly be) present. */
> +#define CHILD_DELETIONS_ALLOWED(editor, relpath) \
> + SVN_ERR_ASSERT(!allow_either(editor, \
> + svn_relpath_dirname(relpath, \
> + (editor)->scratch_pool), \
> + MARKER_ADDED_DIR, MARKER_ALLOW_ADD))
> +
> static svn_boolean_t
> allow_either(const svn_editor_t *editor,
> const char *relpath,
> @@ -166,6 +196,31 @@ check_unknown_child(const svn_editor_t *
> return TRUE;
> }
>
> +static void
> +mark_parent_stable(const svn_editor_t *editor,
> + const char *relpath)
> +{
> + const char *parent = svn_relpath_dirname(relpath, editor->scratch_pool);
> + const void *marker = apr_hash_get(editor->completed_nodes,
> + parent, APR_HASH_KEY_STRING);
> +
> + /* If RELPATH has already been marked (to disallow adds, or that it
> + has been fully-completed), then do nothing. */
> + if (marker == MARKER_ALLOW_ALTER
> + || marker == MARKER_DONE
> + || marker == MARKER_ADDED_DIR)
> + return;
> +
> + /* If the marker is MARKER_ALLOW_ADD, then that means the parent was
> + moved away. There is no way to work on a child. That should have
> + been tested before we got here by VERIFY_PARENT_MAY_EXIST(). */
> + SVN_ERR_ASSERT_NO_RETURN(marker != MARKER_ALLOW_ADD);
> +
> + /* MARKER is NULL. Upgrade it to MARKER_ALLOW_ALTER. */
> + apr_hash_set(editor->completed_nodes, parent, APR_HASH_KEY_STRING,
> + MARKER_ALLOW_ALTER);
> +}
> +
> #else
>
> /* Be wary with the definition of these macros so that we don't
> @@ -192,6 +247,10 @@ check_unknown_child(const svn_editor_t *
> #define MARK_ADDED_DIR(editor, relpath) /* empty */
> #define CHECK_UNKNOWN_CHILD(editor, relpath) /* empty */
>
> +#define MARK_PARENT_STABLE(editor, relpath) /* empty */
> +#define VERIFY_PARENT_MAY_EXIST(editor, relpath) /* empty */
> +#define CHILD_DELETIONS_ALLOWED(editor, relpath) /* empty */
> +
> #endif /* ENABLE_ORDERING_CHECK */
>
>
> @@ -416,6 +475,7 @@ svn_editor_add_directory(svn_editor_t *e
> /* ### validate children are just basenames? */
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_ALLOW_ADD(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
> CHECK_UNKNOWN_CHILD(editor, relpath);
>
> SVN_ERR(check_cancel(editor));
> @@ -430,6 +490,7 @@ svn_editor_add_directory(svn_editor_t *e
> }
>
> MARK_ADDED_DIR(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
> CLEAR_INCOMPLETE(editor, relpath);
>
> #ifdef ENABLE_ORDERING_CHECK
> @@ -469,6 +530,7 @@ svn_editor_add_file(svn_editor_t *editor
> SVN_ERR_ASSERT(props != NULL);
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_ALLOW_ADD(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
> CHECK_UNKNOWN_CHILD(editor, relpath);
>
> SVN_ERR(check_cancel(editor));
> @@ -483,6 +545,7 @@ svn_editor_add_file(svn_editor_t *editor
> }
>
> MARK_COMPLETED(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
> CLEAR_INCOMPLETE(editor, relpath);
>
> svn_pool_clear(editor->scratch_pool);
> @@ -503,6 +566,7 @@ svn_editor_add_symlink(svn_editor_t *edi
> SVN_ERR_ASSERT(props != NULL);
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_ALLOW_ADD(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
> CHECK_UNKNOWN_CHILD(editor, relpath);
>
> SVN_ERR(check_cancel(editor));
> @@ -516,6 +580,7 @@ svn_editor_add_symlink(svn_editor_t *edi
> }
>
> MARK_COMPLETED(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
> CLEAR_INCOMPLETE(editor, relpath);
>
> svn_pool_clear(editor->scratch_pool);
> @@ -534,6 +599,7 @@ svn_editor_add_absent(svn_editor_t *edit
> SVN_ERR_ASSERT(svn_relpath_is_canonical(relpath));
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_ALLOW_ADD(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
> CHECK_UNKNOWN_CHILD(editor, relpath);
>
> SVN_ERR(check_cancel(editor));
> @@ -547,6 +613,7 @@ svn_editor_add_absent(svn_editor_t *edit
> }
>
> MARK_COMPLETED(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
> CLEAR_INCOMPLETE(editor, relpath);
>
> svn_pool_clear(editor->scratch_pool);
> @@ -566,6 +633,7 @@ svn_editor_alter_directory(svn_editor_t
> SVN_ERR_ASSERT(props != NULL);
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_ALLOW_ALTER(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
>
> SVN_ERR(check_cancel(editor));
>
> @@ -579,6 +647,7 @@ svn_editor_alter_directory(svn_editor_t
> }
>
> MARK_COMPLETED(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
>
> svn_pool_clear(editor->scratch_pool);
> return svn_error_trace(err);
> @@ -603,6 +672,7 @@ svn_editor_alter_file(svn_editor_t *edit
> SVN_ERR_ASSERT(checksum->kind == SVN_EDITOR_CHECKSUM_KIND);
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_ALLOW_ALTER(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
>
> SVN_ERR(check_cancel(editor));
>
> @@ -617,6 +687,7 @@ svn_editor_alter_file(svn_editor_t *edit
> }
>
> MARK_COMPLETED(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
>
> svn_pool_clear(editor->scratch_pool);
> return svn_error_trace(err);
> @@ -636,6 +707,7 @@ svn_editor_alter_symlink(svn_editor_t *e
> SVN_ERR_ASSERT(props != NULL || target != NULL);
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_ALLOW_ALTER(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
>
> SVN_ERR(check_cancel(editor));
>
> @@ -650,6 +722,7 @@ svn_editor_alter_symlink(svn_editor_t *e
> }
>
> MARK_COMPLETED(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
>
> svn_pool_clear(editor->scratch_pool);
> return svn_error_trace(err);
> @@ -666,6 +739,8 @@ svn_editor_delete(svn_editor_t *editor,
> SVN_ERR_ASSERT(svn_relpath_is_canonical(relpath));
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_NOT_BE_COMPLETED(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
> + CHILD_DELETIONS_ALLOWED(editor, relpath);
>
> SVN_ERR(check_cancel(editor));
>
> @@ -678,6 +753,7 @@ svn_editor_delete(svn_editor_t *editor,
> }
>
> MARK_COMPLETED(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
>
> svn_pool_clear(editor->scratch_pool);
> return svn_error_trace(err);
> @@ -697,6 +773,8 @@ svn_editor_copy(svn_editor_t *editor,
> SVN_ERR_ASSERT(svn_relpath_is_canonical(dst_relpath));
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_ALLOW_ADD(editor, dst_relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, src_relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, dst_relpath);
>
> SVN_ERR(check_cancel(editor));
>
> @@ -710,6 +788,7 @@ svn_editor_copy(svn_editor_t *editor,
> }
>
> MARK_ALLOW_ALTER(editor, dst_relpath);
> + MARK_PARENT_STABLE(editor, dst_relpath);
> CLEAR_INCOMPLETE(editor, dst_relpath);
>
> svn_pool_clear(editor->scratch_pool);
> @@ -731,6 +810,9 @@ svn_editor_move(svn_editor_t *editor,
> SHOULD_NOT_BE_FINISHED(editor);
> SHOULD_NOT_BE_COMPLETED(editor, src_relpath);
> SHOULD_ALLOW_ADD(editor, dst_relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, src_relpath);
> + CHILD_DELETIONS_ALLOWED(editor, src_relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, dst_relpath);
>
> SVN_ERR(check_cancel(editor));
>
> @@ -744,7 +826,9 @@ svn_editor_move(svn_editor_t *editor,
> }
>
> MARK_ALLOW_ADD(editor, src_relpath);
> + MARK_PARENT_STABLE(editor, src_relpath);
> MARK_ALLOW_ALTER(editor, dst_relpath);
> + MARK_PARENT_STABLE(editor, dst_relpath);
> CLEAR_INCOMPLETE(editor, dst_relpath);
>
> svn_pool_clear(editor->scratch_pool);
> @@ -769,6 +853,8 @@ svn_editor_rotate(svn_editor_t *editor,
>
> SVN_ERR_ASSERT(svn_relpath_is_canonical(relpath));
> SHOULD_NOT_BE_COMPLETED(editor, relpath);
> + VERIFY_PARENT_MAY_EXIST(editor, relpath);
> + CHILD_DELETIONS_ALLOWED(editor, relpath);
> }
> }
> #endif
> @@ -790,6 +876,7 @@ svn_editor_rotate(svn_editor_t *editor,
> {
> const char *relpath = APR_ARRAY_IDX(relpaths, i, const char *);
> MARK_ALLOW_ALTER(editor, relpath);
> + MARK_PARENT_STABLE(editor, relpath);
> }
> }
> #endif
>
>
Re: svn commit: r1333739 - /subversion/trunk/subversion/libsvn_delta/editor.c
Posted by Greg Stein <gs...@gmail.com>.
On May 4, 2012 6:49 AM, "Philip Martin" <ph...@wandisco.com> wrote:
>
> Greg Stein <gs...@gmail.com> writes:
>
> > One fallout that I need to document: alter_*() should be called on
> > child nodes at the destination of a copy/delete. It is now "illegal"
> > to modify a child, then move its parent elsewhere. The proper order
> > is: move parent, then edit the child. (strictly speaking, it doesn't
> > check the full ancestry... just the immediate parent, but whatever...
> > this is still debug code to help us rather than full hard-core
> > verification)
>
> Does this restriction also apply to adding/deleting/moving/rotating all
> children before calling alter_directory on the parent?
Only structural changes are denied (if they weren't already through prior
edits on the parent). alter_directory is still allowed after the child op,
presuming you haven't called it already.
Also note that if the parent was created with add_directory, then
alter_directory is denied (you should have created it with the correct
props).
And yes: you can call alter_directory on the parent before these various
child ops.
The goal is to avoid (say) altering a child, then deleting it by deleting
the parent.
> In an earlier thread I was asking how alter_directory could apply
> properties without knowing the new children:
>
> http://svn.haxx.se/dev/archive-2012-04/0206.shtml
>
http://mail-archives.apache.org/mod_mbox/subversion-dev/201204.mbox/%3C878vi12d0z.fsf@stat.home.lan%3E
>
> When updating the working copy we cannot change the directory properties
> without changing the revision and we cannot change the revision without
> either already having all the children at the new revision or marking
> the directory incomplete.
Oh! I missed that thread. I was off in Kentucky...
You're right. I'll make the necessary changes. I'll respond on that thread
for future readers, too.
Thanks,
-g
Re: svn commit: r1333739 - /subversion/trunk/subversion/libsvn_delta/editor.c
Posted by Philip Martin <ph...@wandisco.com>.
Greg Stein <gs...@gmail.com> writes:
> One fallout that I need to document: alter_*() should be called on
> child nodes at the destination of a copy/delete. It is now "illegal"
> to modify a child, then move its parent elsewhere. The proper order
> is: move parent, then edit the child. (strictly speaking, it doesn't
> check the full ancestry... just the immediate parent, but whatever...
> this is still debug code to help us rather than full hard-core
> verification)
Does this restriction also apply to adding/deleting/moving/rotating all
children before calling alter_directory on the parent?
In an earlier thread I was asking how alter_directory could apply
properties without knowing the new children:
http://svn.haxx.se/dev/archive-2012-04/0206.shtml
http://mail-archives.apache.org/mod_mbox/subversion-dev/201204.mbox/%3C878vi12d0z.fsf@stat.home.lan%3E
When updating the working copy we cannot change the directory properties
without changing the revision and we cannot change the revision without
either already having all the children at the new revision or marking
the directory incomplete.
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com