You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2011/08/12 18:05:15 UTC
svn commit: r1157172 - in /subversion/trunk/subversion:
libsvn_client/revert.c tests/cmdline/copy_tests.py
Author: stsp
Date: Fri Aug 12 16:05:15 2011
New Revision: 1157172
URL: http://svn.apache.org/viewvc?rev=1157172&view=rev
Log:
Make 'svn revert' error out if only one side of a move is reverted.
For now, 'revert' and always refuses to incompletely revert a move.
This will later be extended so that both sides of a move are reverted
if no other local mods affect the moved nodes.
* subversion/libsvn_client/revert.c
(path_is_in_target_list): New helper. Figures out of a given node is
within the revert target list.
(check_moves): New helper. Checks for moved-away and moved-here nodes
within the to-be-reverted subtree, and throws an error if only one half
of a move is present.
(svn_client_revert2): Run check_moves() if we're not reverting the entire WC.
* subversion/tests/cmdline/copy_tests.py
(mv_and_revert_directory): Revert both sides of move to avoid test failure.
Modified:
subversion/trunk/subversion/libsvn_client/revert.c
subversion/trunk/subversion/tests/cmdline/copy_tests.py
Modified: subversion/trunk/subversion/libsvn_client/revert.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/revert.c?rev=1157172&r1=1157171&r2=1157172&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/revert.c (original)
+++ subversion/trunk/subversion/libsvn_client/revert.c Fri Aug 12 16:05:15 2011
@@ -109,6 +109,124 @@ revert(void *baton, apr_pool_t *result_p
return SVN_NO_ERROR;
}
+/* Set *IS_IN_TARGET_LIST to TRUE if LOCAL_ABSPATH is an element
+ * or a child of an element in TARGET_LIST. Else return FALSE. */
+static svn_error_t *
+path_is_in_target_list(svn_boolean_t *is_in_target_list,
+ const char *local_abspath,
+ const apr_array_header_t *target_list,
+ apr_pool_t *scratch_pool)
+{
+ int i;
+
+ for (i = 0; i < target_list->nelts; i++)
+ {
+ const char *target = APR_ARRAY_IDX(target_list, i, const char *);
+ const char *target_abspath;
+
+ SVN_ERR(svn_dirent_get_absolute(&target_abspath, target, scratch_pool));
+ *is_in_target_list = (svn_dirent_skip_ancestor(target_abspath,
+ local_abspath) != NULL);
+ if (*is_in_target_list)
+ return SVN_NO_ERROR;
+ }
+
+ return SVN_NO_ERROR;
+}
+
+/* If any children of LOCAL_ABSPATH have been moved-here
+ * or moved-away, verify that both halfs of each move are
+ * in the revert target list. */
+static svn_error_t *
+check_moves(const char *local_abspath,
+ const apr_array_header_t *target_list,
+ svn_wc_context_t *wc_ctx,
+ apr_pool_t *scratch_pool)
+{
+ const char *moved_to_abspath;
+ const char *copy_op_root_abspath;
+ const char *moved_from_abspath;
+ const char *delete_op_root_abspath;
+ svn_error_t *err;
+
+ err = svn_wc__node_was_moved_away(&moved_to_abspath,
+ ©_op_root_abspath,
+ wc_ctx, local_abspath,
+ scratch_pool, scratch_pool);
+ if (err)
+ {
+ if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
+ {
+ svn_error_clear(err);
+ moved_to_abspath = NULL;
+ copy_op_root_abspath = NULL;
+ }
+ else
+ return svn_error_trace(err);
+ }
+
+ if (moved_to_abspath && copy_op_root_abspath &&
+ strcmp(moved_to_abspath, copy_op_root_abspath) == 0)
+ {
+ svn_boolean_t is_in_target_list;
+
+ SVN_ERR(path_is_in_target_list(&is_in_target_list,
+ moved_to_abspath, target_list,
+ scratch_pool));
+ if (!is_in_target_list)
+ {
+ /* TODO: If the moved-away node has no post-move mods
+ * we can just add it to the target list. */
+ return svn_error_createf(
+ SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Cannot revert '%s' because it was moved to '%s' "
+ "which is not part of the revert; both sides of "
+ "the move must be reverted together"),
+ svn_dirent_local_style(local_abspath, scratch_pool),
+ svn_dirent_local_style(moved_to_abspath, scratch_pool));
+ }
+ }
+
+ err = svn_wc__node_was_moved_here(&moved_from_abspath,
+ &delete_op_root_abspath,
+ wc_ctx, local_abspath,
+ scratch_pool, scratch_pool);
+ if (err)
+ {
+ if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
+ {
+ svn_error_clear(err);
+ moved_from_abspath = NULL;
+ delete_op_root_abspath = NULL;
+ }
+ else
+ return svn_error_trace(err);
+ }
+
+ if (moved_from_abspath && delete_op_root_abspath &&
+ strcmp(moved_from_abspath, delete_op_root_abspath) == 0)
+ {
+ svn_boolean_t is_in_target_list;
+
+ SVN_ERR(path_is_in_target_list(&is_in_target_list,
+ moved_from_abspath, target_list,
+ scratch_pool));
+ if (!is_in_target_list)
+ {
+ /* TODO: If the moved-here node has no post-move mods
+ * we can just add it to the target list. */
+ return svn_error_createf(
+ SVN_ERR_ILLEGAL_TARGET, NULL,
+ _("Cannot revert '%s' because it was moved from '%s' "
+ "which is not part of the revert; both sides of "
+ "the move must be reverted together"),
+ svn_dirent_local_style(local_abspath, scratch_pool),
+ svn_dirent_local_style(moved_from_abspath, scratch_pool));
+ }
+ }
+
+ return SVN_NO_ERROR;
+}
svn_error_t *
svn_client_revert2(const apr_array_header_t *paths,
@@ -170,6 +288,13 @@ svn_client_revert2(const apr_array_heade
local_abspath, pool));
lock_target = wc_root ? local_abspath
: svn_dirent_dirname(local_abspath, pool);
+ if (!wc_root)
+ {
+ err = check_moves(local_abspath, paths, ctx->wc_ctx, subpool);
+ if (err)
+ goto errorful;
+ }
+
err = svn_wc__call_with_write_lock(revert, &baton, ctx->wc_ctx,
lock_target, FALSE, pool, pool);
if (err)
Modified: subversion/trunk/subversion/tests/cmdline/copy_tests.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/copy_tests.py?rev=1157172&r1=1157171&r2=1157172&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/copy_tests.py (original)
+++ subversion/trunk/subversion/tests/cmdline/copy_tests.py Fri Aug 12 16:05:15 2011
@@ -777,8 +777,9 @@ def mv_and_revert_directory(sbox):
# Issue 932: revert failed to lock the parent directory
svntest.actions.run_and_verify_svn(None, None, [], 'revert', '--recursive',
- new_E_path)
+ E_path, new_E_path)
expected_status.remove('A/B/F/E', 'A/B/F/E/alpha', 'A/B/F/E/beta')
+ expected_status.tweak('A/B/E', 'A/B/E/alpha', 'A/B/E/beta', status=' ')
svntest.actions.run_and_verify_status(wc_dir, expected_status)
Re: svn commit: r1157172 - in /subversion/trunk/subversion:
libsvn_client/revert.c tests/cmdline/copy_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 12, 2011 at 09:37:57PM +0200, Stefan Sperling wrote:
> However, clearing move-info if one half is reverted is simpler to
> implement because we don't have to deal with cases where we cannot
> revert the other half without destroying other local changes (e.g. a
> file replacing the delete half of a move).
> The behaviour is also much easier to explain because there are no
> special cases.
>
> Hmmm... I'll think about it. Thanks for raising this concern.
I've decided to go for this approach. See r1157246.
Re: svn commit: r1157172 - in /subversion/trunk/subversion:
libsvn_client/revert.c tests/cmdline/copy_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 12, 2011 at 11:39:17AM -0700, Blair Zajac wrote:
>
> On Aug 12, 2011, at 9:05 AM, stsp@apache.org wrote:
>
> > Author: stsp
> > Date: Fri Aug 12 16:05:15 2011
> > New Revision: 1157172
> >
> > URL: http://svn.apache.org/viewvc?rev=1157172&view=rev
> > Log:
> > Make 'svn revert' error out if only one side of a move is reverted.
>
> I actually find this useful sometimes, to revert one part of the move.
We could make the working copy forget that a move took place if
only one half of a move is reverted.
The current plan is to revert both sides of the move if no
other local changes prevent this.
The rationale is as follows:
What if people accidentally revert one half, commit the other half,
and later realise their mistake? The move is now a copy+delete that
spans multiple revisions.
I anticipate that eventually the server will communicate moves to the client.
If, say, the client asks for delta -r10:40, and a sequence of moves
A->B->C->D has happened within this range, the server will tell the
client "A was moved to D". If the client selects a smaller range,
the server might instead say "A was moved to C".
Now, if any copy+delete didn't happen within a single revision there are
ranges where one half of the move appears but the other does not.
This is can be irritating and prevent some kinds of tree-conflicts to
be handled automatically depending on merges are driven (i.e. how
merge-tracking splits up revision ranges). So I think we should try
to prevent a move from spreading across multiple revisions.
However, clearing move-info if one half is reverted is simpler to
implement because we don't have to deal with cases where we cannot
revert the other half without destroying other local changes (e.g. a
file replacing the delete half of a move).
The behaviour is also much easier to explain because there are no
special cases.
Hmmm... I'll think about it. Thanks for raising this concern.
> > For now, 'revert' and always refuses to incompletely revert a move.
> > This will later be extended so that both sides of a move are reverted
> > if no other local mods affect the moved nodes.
>
> Later is by 1.8?
Well before then. By 1.8 I hope to be done with all subcommands that
can or need to be changed to take advantage of support for local moves.
Re: svn commit: r1157172 - in /subversion/trunk/subversion: libsvn_client/revert.c tests/cmdline/copy_tests.py
Posted by Blair Zajac <bl...@orcaware.com>.
On Aug 12, 2011, at 9:05 AM, stsp@apache.org wrote:
> Author: stsp
> Date: Fri Aug 12 16:05:15 2011
> New Revision: 1157172
>
> URL: http://svn.apache.org/viewvc?rev=1157172&view=rev
> Log:
> Make 'svn revert' error out if only one side of a move is reverted.
I actually find this useful sometimes, to revert one part of the move.
> For now, 'revert' and always refuses to incompletely revert a move.
> This will later be extended so that both sides of a move are reverted
> if no other local mods affect the moved nodes.
Later is by 1.8?
Blair