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 22:49:10 UTC
svn commit: r1157246 - in /subversion/trunk/subversion/libsvn_wc:
wc-queries.sql wc_db.c
Author: stsp
Date: Fri Aug 12 20:49:09 2011
New Revision: 1157246
URL: http://svn.apache.org/viewvc?rev=1157246&view=rev
Log:
When reverting one half of a move, transform the other half into a
plain addition/copy.
* subversion/libsvn_wc/wc-queries.sql
(STMT_SELECT_NODE_INFO): Return the moved_here column.
(STMT_SELECT_NODE_INFO_WITH_LOCK): Same here, just for consistency.
* subversion/libsvn_wc/wc_db.c
(clear_moved_to): New helper for op_revert_txn and op_revert_recursive_txn.
(op_revert_txn, op_revert_recursive_txn): Clear the moved-to relpath at
the delete-half of a move for any reverted moved-here node.
Modified:
subversion/trunk/subversion/libsvn_wc/wc-queries.sql
subversion/trunk/subversion/libsvn_wc/wc_db.c
Modified: subversion/trunk/subversion/libsvn_wc/wc-queries.sql
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc-queries.sql?rev=1157246&r1=1157245&r2=1157246&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc-queries.sql (original)
+++ subversion/trunk/subversion/libsvn_wc/wc-queries.sql Fri Aug 12 20:49:09 2011
@@ -29,7 +29,7 @@
-- STMT_SELECT_NODE_INFO
SELECT op_depth, repos_id, repos_path, presence, kind, revision, checksum,
translated_size, changed_revision, changed_date, changed_author, depth,
- symlink_target, last_mod_time, properties
+ symlink_target, last_mod_time, properties, moved_here
FROM nodes
WHERE wc_id = ?1 AND local_relpath = ?2
ORDER BY op_depth DESC
@@ -38,7 +38,7 @@ ORDER BY op_depth DESC
SELECT op_depth, nodes.repos_id, nodes.repos_path, presence, kind, revision,
checksum, translated_size, changed_revision, changed_date, changed_author,
depth, symlink_target, last_mod_time, properties, lock_token, lock_owner,
- lock_comment, lock_date
+ lock_comment, lock_date, moved_here
FROM nodes
LEFT OUTER JOIN lock ON nodes.repos_id = lock.repos_id
AND nodes.repos_path = lock.repos_relpath
Modified: subversion/trunk/subversion/libsvn_wc/wc_db.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db.c?rev=1157246&r1=1157245&r2=1157246&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/wc_db.c (original)
+++ subversion/trunk/subversion/libsvn_wc/wc_db.c Fri Aug 12 20:49:09 2011
@@ -5159,6 +5159,39 @@ svn_wc__db_op_set_tree_conflict(svn_wc__
}
+/* Clear moved-to information at the delete-half of the move which
+ * moved LOCAL_RELPATH here. This transforms the move into a simple delete. */
+static svn_error_t *
+clear_moved_to(const char *local_relpath,
+ svn_wc__db_wcroot_t *wcroot,
+ apr_pool_t *scratch_pool)
+{
+ svn_sqlite__stmt_t *stmt;
+ svn_boolean_t have_row;
+ const char *moved_from_relpath;
+
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_SELECT_MOVED_FROM_RELPATH));
+ SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id, local_relpath));
+ SVN_ERR(svn_sqlite__step(&have_row, stmt));
+ if (!have_row)
+ {
+ SVN_ERR(svn_sqlite__reset(stmt));
+ return SVN_NO_ERROR;
+ }
+
+ moved_from_relpath = svn_sqlite__column_text(stmt, 0, scratch_pool);
+ SVN_ERR(svn_sqlite__reset(stmt));
+
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_CLEAR_MOVED_TO_RELPATH));
+ SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id,
+ moved_from_relpath));
+ SVN_ERR(svn_sqlite__step_done(stmt));
+
+ return SVN_NO_ERROR;
+}
+
/* This implements svn_wc__db_txn_callback_t */
static svn_error_t *
op_revert_txn(void *baton,
@@ -5169,6 +5202,7 @@ op_revert_txn(void *baton,
svn_sqlite__stmt_t *stmt;
svn_boolean_t have_row;
apr_int64_t op_depth;
+ svn_boolean_t moved_here;
int affected_rows;
/* ### Similar structure to op_revert_recursive_txn, should they be
@@ -5214,6 +5248,7 @@ op_revert_txn(void *baton,
}
op_depth = svn_sqlite__column_int64(stmt, 0);
+ moved_here = svn_sqlite__column_boolean(stmt, 15);
SVN_ERR(svn_sqlite__reset(stmt));
if (op_depth > 0 && op_depth == relpath_depth(local_relpath))
@@ -5253,6 +5288,9 @@ op_revert_txn(void *baton,
SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id, local_relpath));
SVN_ERR(svn_sqlite__step_done(stmt));
+ if (moved_here)
+ SVN_ERR(clear_moved_to(local_relpath, wcroot, scratch_pool));
+
/* Clear the moved-to path of the BASE node. */
SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
STMT_CLEAR_MOVED_TO_RELPATH));
@@ -5288,6 +5326,7 @@ op_revert_recursive_txn(void *baton,
svn_boolean_t have_row;
apr_int64_t op_depth;
int affected_rows;
+ apr_pool_t *iterpool;
/* ### Similar structure to op_revert_txn, should they be
combined? */
@@ -5355,6 +5394,25 @@ op_revert_recursive_txn(void *baton,
local_relpath));
SVN_ERR(svn_sqlite__step_done(stmt));
+ SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
+ STMT_SELECT_MOVED_HERE_CHILDREN));
+ SVN_ERR(svn_sqlite__bindf(stmt, "is", wcroot->wc_id, local_relpath));
+
+ SVN_ERR(svn_sqlite__step(&have_row, stmt));
+
+ iterpool = svn_pool_create(scratch_pool);
+ while (have_row)
+ {
+ const char *moved_here_child_relpath;
+
+ svn_pool_clear(iterpool);
+
+ moved_here_child_relpath = svn_sqlite__column_text(stmt, 0, iterpool);
+ SVN_ERR(clear_moved_to(moved_here_child_relpath, wcroot, iterpool));
+ SVN_ERR(svn_sqlite__step(&have_row, stmt));
+ }
+ svn_pool_destroy(iterpool);
+
/* Clear any moved-to paths of the BASE nodes. */
SVN_ERR(svn_sqlite__get_statement(&stmt, wcroot->sdb,
STMT_CLEAR_MOVED_TO_RELPATH_RECURSIVE));
Re: svn commit: r1157246 - in
/subversion/trunk/subversion/libsvn_wc: wc-queries.sql wc_db.c
Posted by Stefan Sperling <st...@apache.org>.
On Thu, Aug 18, 2011 at 11:22:38AM +0200, Bert Huijben wrote:
> > -----Original Message-----
> > From: stsp@apache.org [mailto:stsp@apache.org]
> > Sent: vrijdag 12 augustus 2011 22:49
> > To: commits@subversion.apache.org
> > Subject: svn commit: r1157246 - in /subversion/trunk/subversion/libsvn_wc:
> > wc-queries.sql wc_db.c
> >
> > Author: stsp
> > Date: Fri Aug 12 20:49:09 2011
> > New Revision: 1157246
>
> Reviewing an old patch.
>
> >
> > URL: http://svn.apache.org/viewvc?rev=1157246&view=rev
> > Log:
> > When reverting one half of a move, transform the other half into a
> > plain addition/copy.
> >
> > * subversion/libsvn_wc/wc-queries.sql
> > (STMT_SELECT_NODE_INFO): Return the moved_here column.
> > (STMT_SELECT_NODE_INFO_WITH_LOCK): Same here, just for consistency.
> >
> > * subversion/libsvn_wc/wc_db.c
> > (clear_moved_to): New helper for op_revert_txn and
> > op_revert_recursive_txn.
> > (op_revert_txn, op_revert_recursive_txn): Clear the moved-to relpath at
> > the delete-half of a move for any reverted moved-here node.
>
> This doesn't look right.
> STMT_SELECT_NODE_INFO and STMT_SELECT_NODE_INFO_WITH_LOCK have exactly the same data in the same column numbers, yet the users of STMT_SELECT_NODE_INFO_WITH_LOCK
> (read: read_info and read_info_children) weren't updated.
Not sure why you mention read_info_children().
It uses STMT_SELECT_NODE_CHILDREN_INFO, not STMT_SELECT_NODE_INFO_WITH_LOCK.
> The new column that was queried should be at the same position in both queries and the offset of the lock data updated.
Does this patch do what you have in mind?
Index: subversion/libsvn_wc/wc-queries.sql
===================================================================
--- subversion/libsvn_wc/wc-queries.sql (revision 1158975)
+++ subversion/libsvn_wc/wc-queries.sql (working copy)
@@ -37,8 +37,8 @@ ORDER BY op_depth DESC
-- STMT_SELECT_NODE_INFO_WITH_LOCK
SELECT op_depth, nodes.repos_id, nodes.repos_path, presence, kind, revision,
checksum, translated_size, changed_revision, changed_date, changed_author,
- depth, symlink_target, last_mod_time, properties, lock_token, lock_owner,
- lock_comment, lock_date, moved_here
+ depth, symlink_target, last_mod_time, properties, moved_here, lock_token,
+ lock_owner, lock_comment, lock_date
FROM nodes
LEFT OUTER JOIN lock ON nodes.repos_id = lock.repos_id
AND nodes.repos_path = lock.repos_relpath
Index: subversion/libsvn_wc/wc_db.c
===================================================================
--- subversion/libsvn_wc/wc_db.c (revision 1158975)
+++ subversion/libsvn_wc/wc_db.c (working copy)
@@ -6776,7 +6776,7 @@ read_info(svn_wc__db_status_t *status,
if (op_depth != 0)
*lock = NULL;
else
- *lock = lock_from_columns(stmt_info, 15, 16, 17, 18, result_pool);
+ *lock = lock_from_columns(stmt_info, 16, 17, 18, 19, result_pool);
}
if (have_work)
RE: svn commit: r1157246 - in /subversion/trunk/subversion/libsvn_wc: wc-queries.sql wc_db.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: stsp@apache.org [mailto:stsp@apache.org]
> Sent: vrijdag 12 augustus 2011 22:49
> To: commits@subversion.apache.org
> Subject: svn commit: r1157246 - in /subversion/trunk/subversion/libsvn_wc:
> wc-queries.sql wc_db.c
>
> Author: stsp
> Date: Fri Aug 12 20:49:09 2011
> New Revision: 1157246
Reviewing an old patch.
>
> URL: http://svn.apache.org/viewvc?rev=1157246&view=rev
> Log:
> When reverting one half of a move, transform the other half into a
> plain addition/copy.
>
> * subversion/libsvn_wc/wc-queries.sql
> (STMT_SELECT_NODE_INFO): Return the moved_here column.
> (STMT_SELECT_NODE_INFO_WITH_LOCK): Same here, just for consistency.
>
> * subversion/libsvn_wc/wc_db.c
> (clear_moved_to): New helper for op_revert_txn and
> op_revert_recursive_txn.
> (op_revert_txn, op_revert_recursive_txn): Clear the moved-to relpath at
> the delete-half of a move for any reverted moved-here node.
This doesn't look right.
STMT_SELECT_NODE_INFO and STMT_SELECT_NODE_INFO_WITH_LOCK have exactly the same data in the same column numbers, yet the users of STMT_SELECT_NODE_INFO_WITH_LOCK
(read: read_info and read_info_children) weren't updated.
The new column that was queried should be at the same position in both queries and the offset of the lock data updated.
Bert
Re: svn commit: r1157246 - in
/subversion/trunk/subversion/libsvn_wc: wc-queries.sql wc_db.c
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Aug 13, 2011 at 10:41:11AM +0400, Ivan Zhakov wrote:
> On Sat, Aug 13, 2011 at 00:49, <st...@apache.org> wrote:
> > Author: stsp
> > Date: Fri Aug 12 20:49:09 2011
> > New Revision: 1157246
> >
> > URL: http://svn.apache.org/viewvc?rev=1157246&view=rev
> > Log:
> > When reverting one half of a move, transform the other half into a
> > plain addition/copy.
> >
> I think that reverting part of move is bad idea in general, but I
> understand that it's needed in some cases. So ideal solution would be
> block reverting half of move, but adding some kind of '--force' option
> to revert it and translate to delete/copy. What do you think?
Let me explain why I think the current behaviour is best.
If we want to revert both halves we have to deal with special cases where
we cannot revert both halves by default (i.e. the following applies
even if no --force switch was necessary to revert both halves):
1) The user wants to revert copied-half, and the delete-half of the move
has been replaced. In this case we cannot revert both halves of the
move without also reverting the node replacing the delete-half.
2) The user wants to revert the delete-half, and the copied-half of the
move has been modified post-move. In this case we cannot revert the
copied-half without also reverting the post-move textual mods.
We would have to force the user to also explicitly revert the other half
of the move. This can get very tedious if multiple moves are involved.
And we would need to document these special cases. I would expect
this would span more than one paragraph in the book.
Whereas the current behaviour is easy to document in a single sentence:
"If only one half of a move is reverted, the other half will be
transformed into a normal copy or delete."
If the new --force flag always causes a revert of both halves of the move
we cannot tell if there are post-move local mods the user wants to keep.
Consider what the new --force flag should mean in these cases:
What if the user wants to replace the node instead of moving it?
Should the user run revert --force (the replacing node is reverted) and
then run rm/add? Right now, a simple 'svn revert' is enough.
What if there are post-move modes in the copied-half the user does not
want to move the node after all but wants to create a modified copy of it?
Should the user save local mods, run revert --force (reverting the
post-move mods along with the copied-half of the move), and then run 'svn
copy' and modify the copied file? Right now, a simple 'svn revert' is enough.
Also, consider this case:
svn mv A/f B/
svn revert -R A/
The user is reverting subtree A of the working copy and may not expect
B to be touched by the revert operation at all. But B/f would also be
reverted (except in special case 2 above). This is a big change from
previous behaviour of recursive revert of a subtree.
Last but not least, trying to revert both halves of a move involves a
lot more code because of the special cases we have to handle. The current
approach requries only minimal changes to the existing revert code.
So it is not only simpler to document but also much simpler to implement.
The only gain we get from the additional complexity is preventing users
from shooting themselves in the foot by reverting only one half of a move
even though they meant to revert both halves, and committing the result
without double-checking their commit for correctness.
I don't think this gain is worth the extra effort in both the implementation
and documentation.
Re: svn commit: r1157246 - in /subversion/trunk/subversion/libsvn_wc:
wc-queries.sql wc_db.c
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Sat, Aug 13, 2011 at 00:49, <st...@apache.org> wrote:
> Author: stsp
> Date: Fri Aug 12 20:49:09 2011
> New Revision: 1157246
>
> URL: http://svn.apache.org/viewvc?rev=1157246&view=rev
> Log:
> When reverting one half of a move, transform the other half into a
> plain addition/copy.
>
I think that reverting part of move is bad idea in general, but I
understand that it's needed in some cases. So ideal solution would be
block reverting half of move, but adding some kind of '--force' option
to revert it and translate to delete/copy. What do you think?
--
Ivan Zhakov
Re: svn commit: r1157246 - When reverting one half of a move ...
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2011-08-12 at 20:49 +0000, stsp@apache.org wrote:
> Author: stsp
> Date: Fri Aug 12 20:49:09 2011
> New Revision: 1157246
>
> URL: http://svn.apache.org/viewvc?rev=1157246&view=rev
> Log:
> When reverting one half of a move, transform the other half into a
> plain addition/copy.
(Just a late observation on this old log msg.)
Should that say "into a plain addition/copy or delete, depending on
which half was reverted."? Or did that commit only handle one half?
- Julian