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, <gstein_at_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
>
>
Received on 2012-05-04 03:47:17 CEST