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

Re: r1661179 - avoiding revision gaps when committing simple moves

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Fri, 20 Feb 2015 21:40:54 +0000

Bert proposed this for backport to 1.9.0.

> URL: http://svn.apache.org/r1661179
> Log:
> Start using our move information in our commit processing to avoid
> unneeded revision gaps when committing simple moves.
>
> This handles the simple case where (during commit) we know that a
> node didn't change in a specific revision range. As this is a
> client side fixup this can't resolve the hard cases, but at
> least it is safe and helps removing many gaps.

I'd love us to be better at committing moves without a gap, but I'd like to get a bit more visibility and thought about how we do it. It just feels to me that this may not be as obvious as it at first looks.

- Julian

> * subversion/include/private/svn_wc_private.h
>   (svn_wc__node_get_deleted_ancestor): Remove unneeded function.
>
> * subversion/libsvn_client/commit.c
>   (svn_client_commit6): Attempt to fix-up the copy-from revision of
>     moved nodes, by fixing up the known safe cases. Trust that the
>     delete_op_root is really the delete op_root.
>
> * subversion/libsvn_wc/node.c
>   (svn_wc__node_get_deleted_ancestor): Remove function.
>
> * subversion/tests/cmdline/log_tests.py
>   (log_revision_move_copy): Update test expectations. The client
>     side commit is now smart enough to avoid the revision gap, for
>     direct moves from BASE.

> Modified: subversion/trunk/subversion/libsvn_client/commit.c
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_client/commit.c Fri Feb 20 18:26:04 2015
> @@ -537,6 +537,7 @@ svn_client_commit6(const apr_array_heade
>    const char *current_abspath;
>    const char *notify_prefix;
>    int depth_empty_after = -1;
> +  apr_hash_t *move_youngest = NULL;
>    int i;
>
>    SVN_ERR_ASSERT(depth != svn_depth_unknown && depth !=
> svn_depth_exclude);
> @@ -707,62 +708,12 @@ svn_client_commit6(const apr_array_heade
>            if (cmt_err)
>              goto cleanup;
>
> -          if (moved_from_abspath && delete_op_root_abspath &&
> -              strcmp(moved_from_abspath, delete_op_root_abspath) == 0)
> -
> +          if (moved_from_abspath && delete_op_root_abspath)
>              {
> -              svn_boolean_t found_delete_half =
> -                (svn_hash_gets(committables->by_path,
> delete_op_root_abspath)
> -                 != NULL);
> +              svn_client_commit_item3_t *delete_half =
> +                svn_hash_gets(committables->by_path,
> delete_op_root_abspath);
>
> -              if (!found_delete_half)
> -                {
> -                  const char *delete_half_parent_abspath;
> -
> -                  /* The delete-half isn't in the commit target list.
> -                   * However, it might itself be the child of a deleted node,
> -                   * either because of another move or a deletion.
> -                   *
> -                   * For example, consider: mv A/B B; mv B/C C; commit;
> -                   * C's moved-from A/B/C is a child of the deleted A/B.
> -                   * A/B/C does not appear in the commit target list, but
> -                   * A/B does appear.
> -                   * (Note that moved-from information is always stored
> -                   * relative to the BASE tree, so we have 'C moved-from
> -                   * A/B/C', not 'C moved-from B/C'.)
> -                   *
> -                   * An example involving a move and a delete would be:
> -                   * mv A/B C; rm A; commit;
> -                   * Now C is moved-from A/B which does not appear in the
> -                   * commit target list, but A does appear.
> -                   */
> -
> -                  /* Scan upwards for a deletion op-root from the
> -                   * delete-half's parent directory. */
> -                  delete_half_parent_abspath =
> -                    svn_dirent_dirname(delete_op_root_abspath, iterpool);
> -                  if (strcmp(delete_op_root_abspath,
> -                             delete_half_parent_abspath) != 0)
> -                    {
> -                      const char *parent_delete_op_root_abspath;
> -
> -                      cmt_err = svn_error_trace(
> -                                  svn_wc__node_get_deleted_ancestor(
> -                                    &parent_delete_op_root_abspath,
> -                                    ctx->wc_ctx, delete_half_parent_abspath,
> -                                    iterpool, iterpool));
> -                      if (cmt_err)
> -                        goto cleanup;
> -
> -                      if (parent_delete_op_root_abspath)
> -                        found_delete_half =
> -                          (svn_hash_gets(committables->by_path,
> -                                         parent_delete_op_root_abspath)
> -                           != NULL);
> -                    }
> -                }
> -
> -              if (!found_delete_half)
> +              if (!delete_half)
>                  {
>                    cmt_err = svn_error_createf(
>                                SVN_ERR_ILLEGAL_TARGET, NULL,
> @@ -787,6 +738,17 @@ svn_client_commit6(const apr_array_heade
>
>                    goto cleanup;
>                  }
> +              else if (delete_half->revision == item->copyfrom_rev)
> +                {
> +                  /* Ok, now we know that we perform an out-of-date check
> +                     on the copyfrom location. Remember this for a fixup
> +                     round right before committing. */
> +
> +                  if (!move_youngest)
> +                    move_youngest = apr_hash_make(pool);
> +
> +                  svn_hash_sets(move_youngest, item->path, item);
> +                }
>              }
>          }
>
> @@ -885,6 +847,37 @@ svn_client_commit6(const apr_array_heade
>    if (cmt_err)
>      goto cleanup;
>
> +  if (move_youngest != NULL)
> +    {
> +      apr_hash_index_t *hi;
> +      svn_revnum_t youngest;
> +
> +      SVN_ERR(svn_ra_get_latest_revnum(ra_session, &youngest, pool));
> +
> +      for (hi = apr_hash_first(iterpool, move_youngest);
> +           hi;
> +           hi = apr_hash_next(hi))
> +        {
> +          svn_client_commit_item3_t *item = apr_hash_this_val(hi);
> +
> +          /* We delete the original side with its original revision and will
> +             receive an out-of-date error if that node changed since that
> +             revision.
> +
> +             The copy is of that same revision and we know that this revision
> +             didn't change between this revision and youngest. So we can
> just
> +             as well commit a copy from youngest.
> +
> +            Note that it is still possible to see gaps between the delete and
> +            copy revisions as the repository might handle multiple commits
> +            at the same time (or when an out of date proxy is involved), but
> +            in general it should decrease the number of gaps. */
> +
> +          if (item->copyfrom_rev < youngest)
> +            item->copyfrom_rev = youngest;
> +        }
> +    }
> +
>    cmt_err = svn_error_trace(
>                get_ra_editor(&editor, &edit_baton, ra_session, ctx,
>                              log_msg, commit_items, revprop_table,
>

> Modified: subversion/trunk/subversion/tests/cmdline/log_tests.py
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/log_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/log_tests.py Fri Feb 20 18:26:04
> 2015
> @@ -2627,7 +2627,10 @@ def log_revision_move_copy(sbox):
>    sbox.simple_append('iotb', 'new line\n')
>
>    sbox.simple_copy('A/mu', 'mutb')
> -  sbox.simple_append('iotb', 'mutb\n')
> +  sbox.simple_append('mutb', 'mutb\n')
> +
> +  sbox.simple_move('A/B/E', 'E')
> +  sbox.simple_move('E/alpha', 'alpha')
>
>    #r2
>    svntest.actions.run_and_verify_svn(None, [],
> @@ -2639,10 +2642,12 @@ def log_revision_move_copy(sbox):
>    # of these nodes behaves in r2.
>
>    # This one might change behavior once we improve move handling
> -  expected_output = []
> -  expected_err = '.*E195012: Unable to find repository location.*'
> +  expected_output = [
> +   
> '------------------------------------------------------------------------\n'
> +  ]
> +  expected_err = []
>    svntest.actions.run_and_verify_svn(expected_output, expected_err,
> -                                     'log', '-v',
> sbox.ospath('iotb'),
> +                                     'log',
> '-v',sbox.ospath('iotb'),
>                                       '-r2')
>
>    # While this one
> @@ -2653,8 +2658,10 @@ def log_revision_move_copy(sbox):
>                                       '-r2')
>
>    # And just for fun, do the same thing for blame
> -  expected_output = None
> -  expected_err = '.*E195012: Unable to find repository location.*'
> +  expected_output = [
> +    '     1    jrandom This is the file
> \'iota\'.\n'
> +  ]
> +  expected_err = []
>    svntest.actions.run_and_verify_svn(expected_output, expected_err,
>                                       'blame',
> sbox.ospath('iotb'),
>                                       '-r2')
> @@ -2662,9 +2669,26 @@ def log_revision_move_copy(sbox):
>    expected_output = None
>    expected_err = '.*E195012: Unable to find repository location.*'
>    svntest.actions.run_and_verify_svn(expected_output, expected_err,
> -                                     'log', '-v',
> sbox.ospath('mutb'),
> +                                     'blame',
> sbox.ospath('mutb'),
>                                       '-r2')
>
> +  expected_output = svntest.verify.RegexListOutput([
> +    '-+\\n',
> +    'r3\ .*\n',
> +    re.escape('Changed paths:\n'),
> +    re.escape('   D /A/B/E\n'),
> +    re.escape('   A /E (from /A/B/E:2)\n'), # Patched - Direct move
> +    re.escape('   D /E/alpha\n'),
> +    re.escape('   A /alpha (from /A/B/E/alpha:1)\n'), # Indirect
> move - Not patched
> +    re.escape('   D /iota\n'),
> +    re.escape('   A /iotb (from /iota:2)\n'), # Patched - Direct
> move
> +    re.escape('   A /mutb (from /A/mu:1)\n'), # Copy (always r1)
> +    '-+\\n'
> +  ])
> +  svntest.actions.run_and_verify_svn(expected_output, [],
> +                                     'log', '-v', '-q',
> sbox.wc_dir,
> +                                     '-c3')
> +
>
> ########################################################################
> # Run the tests
>
Received on 2015-02-20 22:43:57 CET

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