[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1096921 - in /subversion/trunk/subversion: libsvn_client/merge.c libsvn_client/repos_diff.c tests/cmdline/merge_tree_conflict_tests.py tests/cmdline/tree_conflict_tests.py

From: Paul Burba <ptburba_at_gmail.com>
Date: Thu, 28 Apr 2011 11:37:51 -0400

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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.