On Tue, Apr 26, 2011 at 5:38 PM, <rhuijben_at_apache.org> wrote:
> Author: rhuijben
> Date: Tue Apr 26 21:38:34 2011
> New Revision: 1096921
>
> URL: http://svn.apache.org/viewvc?rev=1096921&view=rev
> Log:
> Remove the last bit of working copy obstruction detection from the repository
> diff editor.
>
> This patch makes the merge editor aware of the list of added directories to
> allow dry run property merges to directories.
>
> * subversion/libsvn_client/merge.c
> (merge_cmd_baton_t): Add hashtable of merge added paths.
> (dry_run_added_p): New function.
> (perform_obstruction_check): Specialize handling of merge-added paths.
> (merge_dir_props_changed): Don't perform actual prop merging for added
> directories when doing a dry-run merge.
> (merge_file_added,
> merge_dir_added,
> merge_dir_deleted,
> merge_dir_closed): Don't set optional output arguments to their default.
>
> * subversion/libsvn_client/repos_diff.c
> (close_directory): When an obstruction is noticed, update the content state
> and use updated notifications.
>
> * subversion/tests/cmdline/merge_tree_conflict_tests.py
> (tree_conflicts_merge_edit_onto_missing,
> tree_conflicts_merge_del_onto_missing): Expect more skip notifications.
>
> * subversion/tests/cmdline/tree_conflict_tests.py
> (at_directory_external): Mark as XFail again. This change moves the failure
> to where we step into the external for the first time. The merge code
> detects this properly now, but wants to apply mergeinfo on the external
> to notice that it wasn't merged. But it can't and shouldn't store this
> information.
>
> Modified:
> subversion/trunk/subversion/libsvn_client/merge.c
> subversion/trunk/subversion/libsvn_client/repos_diff.c
> subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py
> subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_client/merge.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1096921&r1=1096920&r2=1096921&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/merge.c (original)
> +++ subversion/trunk/subversion/libsvn_client/merge.c Tue Apr 26 21:38:34 2011
> @@ -276,6 +276,10 @@ typedef struct merge_cmd_baton_t {
> dry_run mode. */
> apr_hash_t *dry_run_deletions;
>
> + /* The list of paths for entries we've added, used only when in
> + dry_run mode. */
> + apr_hash_t *dry_run_added;
> +
> /* The list of any paths which remained in conflict after a
> resolution attempt was made. We track this in-memory, rather
> than just using WC entry state, since the latter doesn't help us
> @@ -353,6 +357,20 @@ dry_run_deleted_p(const merge_cmd_baton_
> APR_HASH_KEY_STRING) != NULL);
> }
>
> +/* Return true iff we're in dry-run mode and WCPATH would have been
> + added by now if we weren't in dry-run mode.
> + Used to avoid spurious notifications (e.g. conflicts) from a merge
> + attempt into an existing target which would have been deleted if we
> + weren't in dry_run mode (issue #2584). Assumes that WCPATH is
> + still versioned (e.g. has an associated entry). */
> +static APR_INLINE svn_boolean_t
> +dry_run_added_p(const merge_cmd_baton_t *merge_b, const char *wcpath)
> +{
> + return (merge_b->dry_run &&
> + apr_hash_get(merge_b->dry_run_added, wcpath,
> + APR_HASH_KEY_STRING) != NULL);
> +}
> +
> /* Return whether any WC path was put in conflict by the merge
> operation corresponding to MERGE_B. */
> static APR_INLINE svn_boolean_t
> @@ -408,20 +426,36 @@ perform_obstruction_check(svn_wc_notify_
> *kind = svn_node_none;
>
> /* In a dry run, make as if nodes "deleted" by the dry run appear so. */
> - if (merge_b->dry_run && dry_run_deleted_p(merge_b, local_abspath))
> + if (merge_b->dry_run)
> {
> - *obstruction_state = svn_wc_notify_state_inapplicable;
> + if (dry_run_deleted_p(merge_b, local_abspath))
> + {
> + *obstruction_state = svn_wc_notify_state_inapplicable;
> +
> + if (versioned)
> + *versioned = TRUE;
> + if (deleted)
> + *deleted = TRUE;
> +
> + if (expected_kind != svn_node_unknown
> + && expected_kind != svn_node_none)
> + *obstruction_state = svn_wc_notify_state_obstructed;
> + return SVN_NO_ERROR;
> + }
> + else if (dry_run_added_p(merge_b, local_abspath))
> + {
> + *obstruction_state = svn_wc_notify_state_inapplicable;
>
> - if (versioned)
> - *versioned = TRUE;
> - if (deleted)
> - *deleted = TRUE;
> -
> - if (expected_kind != svn_node_unknown
> - && expected_kind != svn_node_none)
> - *obstruction_state = svn_wc_notify_state_obstructed;
> - return SVN_NO_ERROR;
> - }
> + if (versioned)
> + *versioned = TRUE;
> + if (added)
> + *added = TRUE;
> + if (kind)
> + *kind = svn_node_dir; /* Currently only used for dirs */
> +
> + return SVN_NO_ERROR;
> + }
> + }
>
> if (kind == NULL)
> kind = &wc_kind;
> @@ -1224,6 +1258,13 @@ merge_dir_props_changed(svn_wc_notify_st
> return SVN_NO_ERROR;
> }
>
> + if (dir_was_added
> + && merge_b->dry_run
> + && dry_run_added_p(merge_b, local_abspath))
> + {
> + return SVN_NO_ERROR; /* We can't do a real prop merge for added dirs */
> + }
> +
> return svn_error_return(merge_props_changed(state,
> tree_conflicted,
> local_abspath,
> @@ -1544,8 +1585,6 @@ merge_file_added(svn_wc_notify_state_t *
> *content_state = svn_wc_notify_state_unchanged;
> if (prop_state)
> *prop_state = svn_wc_notify_state_unchanged;
> - if (tree_conflicted)
> - *tree_conflicted = FALSE;
> return SVN_NO_ERROR;
> }
>
> @@ -1555,9 +1594,6 @@ merge_file_added(svn_wc_notify_state_t *
> if (prop_state)
> *prop_state = svn_wc_notify_state_unknown;
>
> - if (tree_conflicted)
> - *tree_conflicted = FALSE;
> -
> /* Apply the prop changes to a new hash table. */
> file_props = apr_hash_copy(subpool, original_props);
> for (i = 0; i < prop_changes->nelts; ++i)
> @@ -1996,8 +2032,6 @@ merge_dir_added(svn_wc_notify_state_t *s
> svn_pool_destroy(subpool);
> if (state)
> *state = svn_wc_notify_state_unchanged;
> - if (tree_conflicted)
> - *tree_conflicted = FALSE;
> return SVN_NO_ERROR;
> }
>
> @@ -2069,7 +2103,11 @@ merge_dir_added(svn_wc_notify_state_t *s
> case svn_node_none:
> /* Unversioned or schedule-delete */
> if (merge_b->dry_run)
> - merge_b->added_path = apr_pstrdup(merge_b->pool, local_abspath);
> + {
> + merge_b->added_path = apr_pstrdup(merge_b->pool, local_abspath);
> + apr_hash_set(merge_b->dry_run_added, merge_b->added_path,
> + APR_HASH_KEY_STRING, merge_b->added_path);
> + }
> else
> {
> SVN_ERR(svn_io_dir_make(local_abspath, APR_OS_DEFAULT, subpool));
> @@ -2183,12 +2221,8 @@ merge_dir_deleted(svn_wc_notify_state_t
> svn_pool_destroy(subpool);
> if (state)
> *state = svn_wc_notify_state_unchanged;
> - if (tree_conflicted)
> - *tree_conflicted = FALSE;
> return SVN_NO_ERROR;
> }
> - if (tree_conflicted)
> - *tree_conflicted = FALSE;
>
> /* Check for an obstructed or missing node on disk. */
> {
> @@ -2434,12 +2468,6 @@ merge_dir_closed(svn_wc_notify_state_t *
> apr_pool_t *scratch_pool)
> {
> merge_cmd_baton_t *merge_b = baton;
> - if (contentstate)
> - *contentstate = svn_wc_notify_state_unknown;
> - if (propstate)
> - *propstate = svn_wc_notify_state_unknown;
> - if (tree_conflicted)
> - *tree_conflicted = FALSE;
>
> if (merge_b->dry_run)
> svn_hash__clear(merge_b->dry_run_deletions, scratch_pool);
> @@ -8654,6 +8682,8 @@ do_merge(apr_hash_t **modified_subtrees,
> merge_cmd_baton.add_necessitated_merge = FALSE;
> merge_cmd_baton.dry_run_deletions =
> dry_run ? apr_hash_make(subpool) : NULL;
> + merge_cmd_baton.dry_run_added =
> + dry_run ? apr_hash_make(subpool) : NULL;
> merge_cmd_baton.conflicted_paths = NULL;
> merge_cmd_baton.paths_with_new_mergeinfo = NULL;
> merge_cmd_baton.paths_with_deleted_mergeinfo = NULL;
>
> Modified: subversion/trunk/subversion/libsvn_client/repos_diff.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/repos_diff.c?rev=1096921&r1=1096920&r2=1096921&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/repos_diff.c (original)
> +++ subversion/trunk/subversion/libsvn_client/repos_diff.c Tue Apr 26 21:38:34 2011
> @@ -983,40 +983,12 @@ close_directory(void *dir_baton,
> struct edit_baton *eb = b->edit_baton;
> svn_wc_notify_state_t content_state = svn_wc_notify_state_unknown;
> svn_wc_notify_state_t prop_state = svn_wc_notify_state_unknown;
> - svn_node_kind_t kind;
> - const char *local_dir_abspath;
> + svn_boolean_t skipped = FALSE;
>
> /* Skip *everything* within a newly tree-conflicted directory. */
> if (b->skip)
> return SVN_NO_ERROR;
>
> - if (eb->wc_ctx)
> - SVN_ERR(svn_wc_read_kind(&kind, eb->wc_ctx, b->wcpath, FALSE, pool));
> - else
> - kind = svn_node_dir;
> -
> - if (kind != svn_node_dir)
> - {
> - /* ### maybe try to stat the local b->wcpath? */
> - /* If the path doesn't exist, then send a 'skipped' notification.
> - Don't notify added directories as they triggered notification
> - in add_directory. */
> - if (! b->added && eb->notify_func)
> - {
> - svn_wc_notify_t *notify
> - = svn_wc_create_notify(b->wcpath,
> - b->tree_conflicted
> - ? svn_wc_notify_tree_conflict
> - : svn_wc_notify_skip,
> - pool);
> - notify->kind = svn_node_dir;
> - notify->content_state = notify->prop_state
> - = svn_wc_notify_state_missing;
> - (*eb->notify_func)(eb->notify_baton, notify, pool);
> - }
> - return SVN_NO_ERROR;
> - }
> -
> /* Don't do the props_changed stuff if this is a dry_run and we don't
> have an access baton, since in that case the directory will already
> have been recognised as added, in which case they cannot conflict. */
> @@ -1030,23 +1002,30 @@ close_directory(void *dir_baton,
> b->edit_baton->diff_cmd_baton, pool));
> if (tree_conflicted)
> b->tree_conflicted = TRUE;
> +
> + if (prop_state == svn_wc_notify_state_obstructed
> + || prop_state == svn_wc_notify_state_missing)
> + {
> + content_state = prop_state;
> + skipped = TRUE;
> + }
> }
>
> - SVN_ERR(eb->diff_callbacks->dir_closed(
> - NULL, NULL, NULL,
> - b->wcpath, b->added,
> - b->edit_baton->diff_cmd_baton, pool));
> + SVN_ERR(eb->diff_callbacks->dir_closed(NULL, NULL, NULL,
> + b->wcpath, b->added,
> + b->edit_baton->diff_cmd_baton,
> + pool));
>
> /* Don't notify added directories as they triggered notification
> in add_directory. */
> - if (!b->added && eb->notify_func)
> + if (!skipped && !b->added && eb->notify_func)
> {
> - svn_wc_notify_t *notify;
> apr_hash_index_t *hi;
>
> for (hi = apr_hash_first(pool, eb->deleted_paths); hi;
> hi = apr_hash_next(hi))
> {
> + svn_wc_notify_t *notify;
> const char *deleted_path = svn__apr_hash_index_key(hi);
> deleted_path_notify_t *dpn = svn__apr_hash_index_val(hi);
>
> @@ -1058,12 +1037,21 @@ close_directory(void *dir_baton,
> apr_hash_set(eb->deleted_paths, deleted_path,
> APR_HASH_KEY_STRING, NULL);
> }
> + }
> +
> + if (!b->added && eb->notify_func)
> + {
> + svn_wc_notify_t *notify;
> + svn_wc_notify_action_t action;
> +
> + if (b->tree_conflicted)
> + action = svn_wc_notify_tree_conflict;
> + else if (skipped)
> + action = svn_wc_notify_skip;
> + else
> + action = svn_wc_notify_update_update;
>
> - notify = svn_wc_create_notify(b->wcpath,
> - b->tree_conflicted
> - ? svn_wc_notify_tree_conflict
> - : svn_wc_notify_update_update,
> - pool);
> + notify = svn_wc_create_notify(b->wcpath, action, pool);
> notify->kind = svn_node_dir;
>
> /* In case of a tree conflict during merge, the diff callback
>
> Modified: subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py?rev=1096921&r1=1096920&r2=1096921&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/merge_tree_conflict_tests.py Tue Apr 26 21:38:34 2011
> @@ -1348,6 +1348,13 @@ def tree_conflicts_merge_edit_onto_missi
>
> expected_skip = svntest.wc.State('', {
> 'F/alpha' : Item(),
> + # BH: After fixing several issues in the obstruction handling
> + # I get the following Skip notifications. Please review!
> + 'D/D1' : Item(),
> + 'DD/D1' : Item(),
> + 'DF/D1' : Item(),
> + 'DDD/D1' : Item(),
> + 'DDF/D1' : Item(),
> })
Hi Bert,
Short answer: I think your change to this test's expectations are correct.
Long answer:
So this merge tries to apply diffs to subtrees which are missing due
to an OS-level deletion. In 1.6 this would have produced tree
conflicts, but in r946186 stsp adjusted the merge behavior to skip
obstructed and missing directories during a merge (missing/obstructed
files were already skipped). The log for that change explains the
reasoning for this, so I won't rehash it here except to note that
since that change was made we now prevent merge-tracking aware merges
when subtrees are missing. So the bit in the log about
non-inheritable mergeinfo no longer applies.
Anyway, assuming r946186 is the desired behavior (I think it is) then
your changes in r1096921 actually fix a bug we missed:
Prior to r1096921we only issued notifications for missing *files*:
trunk_at_1096920>svn st
! local_tree_missing_incoming_leaf_edit\local\D\D1
! local_tree_missing_incoming_leaf_edit\local\F\alpha
! local_tree_missing_incoming_leaf_edit\local\DD\D1
! local_tree_missing_incoming_leaf_edit\local\DD\D1\D2
! local_tree_missing_incoming_leaf_edit\local\DF\D1
! local_tree_missing_incoming_leaf_edit\local\DF\D1\beta
! local_tree_missing_incoming_leaf_edit\local\DDD\D1
! local_tree_missing_incoming_leaf_edit\local\DDD\D1\D2
! local_tree_missing_incoming_leaf_edit\local\DDD\D1\D2\D3
! local_tree_missing_incoming_leaf_edit\local\DDF\D1
! local_tree_missing_incoming_leaf_edit\local\DDF\D1\D2
! local_tree_missing_incoming_leaf_edit\local\DDF\D1\D2\gamma
trunk_at_1096920>svn merge ^^/local_tree_missing_incoming_leaf_edit/incoming
local_tree_missing_incoming_leaf_edit\local --ignore-ancestry
Skipped missing target: 'local_tree_missing_incoming_leaf_edit\local\F\alpha'
Summary of conflicts:
Skipped paths: 1
Uh, that seems quite wrong since the incoming change touches a lot
more than alpha:
trunk_at_1096920>svn log -v -r4 ^^/local_tree_missing_incoming_leaf_edit/incoming
------------------------------------------------------------------------
r4 | jrandom | 2011-04-28 09:26:12 -0400 (Thu, 28 Apr 2011) | 1 line
Changed paths:
M /local_tree_missing_incoming_leaf_edit/incoming/D/D1
A /local_tree_missing_incoming_leaf_edit/incoming/D/D1/delta
M /local_tree_missing_incoming_leaf_edit/incoming/DD/D1/D2
A /local_tree_missing_incoming_leaf_edit/incoming/DD/D1/D2/epsilon
M /local_tree_missing_incoming_leaf_edit/incoming/DDD/D1/D2/D3
A /local_tree_missing_incoming_leaf_edit/incoming/DDD/D1/D2/D3/zeta
M /local_tree_missing_incoming_leaf_edit/incoming/DDF/D1/D2/gamma
M /local_tree_missing_incoming_leaf_edit/incoming/DF/D1/beta
M /local_tree_missing_incoming_leaf_edit/incoming/F/alpha
Committing incoming actions
------------------------------------------------------------------------
trunk_at_1096921>svn merge ^^/local_tree_missing_incoming_leaf_edit/incoming
local_tree_missing_incoming_leaf_edit\local --ignore-ancestry --dry-run
Skipped missing target: 'local_tree_missing_incoming_leaf_edit\local\D\D1'
Skipped missing target: 'local_tree_missing_incoming_leaf_edit\local\F\alpha'
Skipped missing target: 'local_tree_missing_incoming_leaf_edit\local\DD\D1'
Skipped missing target: 'local_tree_missing_incoming_leaf_edit\local\DF\D1'
Skipped missing target: 'local_tree_missing_incoming_leaf_edit\local\DDD\D1'
Skipped missing target: 'local_tree_missing_incoming_leaf_edit\local\DDF\D1'
Summary of conflicts:
Skipped paths: 6
Now we see a notification for the root of each missing subtree in
which we are attempting to apply a diff. No longer are those five
directories silently skipped.
> @@ -1424,6 +1431,13 @@ def tree_conflicts_merge_del_onto_missin
> expected_skip = svntest.wc.State('', {
> 'F/alpha' : Item(),
> 'D/D1' : Item(),
> + # BH: After fixing several issues in the obstruction handling
> + # I get the following Skip notifications. Please review!
> + 'D/D1' : Item(),
> + 'DD/D1' : Item(),
> + 'DF/D1' : Item(),
> + 'DDD/D1' : Item(),
> + 'DDF/D1' : Item(),
> })
Pretty much the same thing as above except prior to r1096921 we did
get a notification that the *directory* D/D1 was skipped. This was
because of yet another inconsistency in reporting skips: An incoming
delete on the root of a missing subtree produced a notification, but
not an incoming delete below the root of a missing subtree.
Fortunately you took care to avoid duplicate notifications in this
case.
Paul
> svntest.actions.deep_trees_run_tests_scheme_for_merge(sbox,
>
> Modified: subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py?rev=1096921&r1=1096920&r2=1096921&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py Tue Apr 26 21:38:34 2011
> @@ -1084,6 +1084,7 @@ def lock_update_only(sbox):
> # subversion/libsvn_wc/lock.c:1437: (apr_err=155005)
> # svn: E155005: No write-lock in '/.../svn-test-work/working_copies/tree_conflict_tests-22/E'
> @Issue(3469)
> +@XFail()
> def at_directory_external(sbox):
> "tree conflict at directory external"
Received on 2011-04-28 17:41:11 CEST