Philip or anyone: my main concern here is whether I'm taking a sensible
approach here in throwing away the new all-in-one copy and going back to
the old code here. Any comments? Anyway here's today's iteration of
this patch.
More below...
On Mon, 2010-11-22, Julian Foad wrote:
> lock_tests.py 40 fails because my WC-to-WC copy code doesn't reset files
> to read-write on disk when it copies them if svn:needs-lock was set on
> the source, which the old code did; instead, it just copies exactly
> what's there.
>
> The attached patch is where I have got to today, attacking that from the
> angle of abandoning my all-in-one approach now that Philip's made the
> WC-to-WC code handle op-depth automatically. I try to extend that to
> cover this case.
>
> As it says in the log msg, it fixes lock_tests.py 40 but breaks
> copy_tests.py 47. I haven't time today to go further with seeing why.
The attached version 2 of this patch doesn't break copy_tests 47 or
anything else, AFICT; still running tests now.
In patch version 1, db_op_copy() was inserting 'incomplete' nodes for
children of a copied added dir, even though its children were only local
adds, so that was wrong. Instead I am making it use a new function
"gather_repo_children()" which only reads children at the same op-depth
as the specified parent dir. I *think* that would give the correct set
of children that need to be marked as incomplete. At least it returns
no children when there are only local adds, which is the case in
lock_tests 40.
I want to split this patch up and commit the refactoring of
gather_children() as a separate patch first, then the behaviour changes.
- Julian
Fix the op-depth code for copies. This makes lock_tests.py 40 PASS,
when compiled with SVN_WC__OP_DEPTH defined.
* subversion/libsvn_wc/copy.c
(svn_wc_copy3): Always use the old one-node-at-a-time code.
* subversion/libsvn_wc/wc_db.c
(add_children_to_hash): Remove.
(gather_repo_children): New function.
(gather_children): Remove the 'base_only' parameter, as that case can now
be handled by gather_repo_children(op_depth=0). Re-write, bringing the
guts of add_children_to_hash() in-line here.
(svn_wc__db_base_get_children, make_copy_txn): Replace
gather_children(base_only=TRUE) with gather_repo_children(op_depth=0).
(db_op_copy, svn_wc__db_op_copy): Calculate the correct op depth
internally, instead of using what the caller provides. By using
gather_repo_children(), don't add 'incomplete' rows for all children but
only those children that belong to the node in the repository.
(svn_wc__db_read_children): Adjust the gather_children() call.
* subversion/libsvn_wc/wc-queries.sql
(STMT_SELECT_BASE_NODE_CHILDREN): Remove.
(STMT_SELECT_OP_DEPTH_CHILDREN): New query.
--This line, and those below, will be ignored--
* subversion/libsvn_wc/copy.c
(svn_wc_copy3):
* subversion/libsvn_wc/wc_db.c
* subversion/libsvn_wc/wc-queries.sql
(VALUES):
--This line, and those below, will be ignored--
Index: subversion/libsvn_wc/copy.c
===================================================================
--- subversion/libsvn_wc/copy.c (revision 1038202)
+++ subversion/libsvn_wc/copy.c (working copy)
@@ -723,7 +723,7 @@ svn_wc_copy3(svn_wc_context_t *wc_ctx,
scratch_pool));
}
-#ifdef SVN_WC__OP_DEPTH
+#if 0 /* was: #ifdef SVN_WC__OP_DEPTH */
if (svn_wc__db_same_db(db, src_abspath, dst_abspath, scratch_pool))
{
SVN_ERR(copy_versioned_tree(db, src_abspath, dst_abspath,
Index: subversion/libsvn_wc/wc_db.c
===================================================================
--- subversion/libsvn_wc/wc_db.c (revision 1038202)
+++ subversion/libsvn_wc/wc_db.c (working copy)
@@ -1035,43 +1035,48 @@ insert_working_node(void *baton,
}
-/* Each name is allocated in RESULT_POOL and stored into CHILDREN as a key
- pointed to the same name. */
+/* Set *CHILDREN to a new array of (const char *) names of the repository
+ children of the directory PDH:LOCAL_RELPATH - that is, the children at
+ the same op-depth. */
static svn_error_t *
-add_children_to_hash(apr_hash_t *children,
- int stmt_idx,
- svn_sqlite__db_t *sdb,
- apr_int64_t wc_id,
- const char *parent_relpath,
- apr_pool_t *result_pool)
+gather_repo_children(const apr_array_header_t **children,
+ svn_wc__db_pdh_t *pdh,
+ const char *local_relpath,
+ apr_int64_t op_depth,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool)
{
+ apr_array_header_t *result
+ = apr_array_make(result_pool, 0, sizeof(const char *));
svn_sqlite__stmt_t *stmt;
svn_boolean_t have_row;
- SVN_ERR(svn_sqlite__get_statement(&stmt, sdb, stmt_idx));
- SVN_ERR(svn_sqlite__bindf(stmt, "is", wc_id, parent_relpath));
+ SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->wcroot->sdb,
+ STMT_SELECT_OP_DEPTH_CHILDREN));
+ SVN_ERR(svn_sqlite__bindf(stmt, "isi", pdh->wcroot->wc_id, local_relpath,
+ op_depth));
SVN_ERR(svn_sqlite__step(&have_row, stmt));
while (have_row)
{
const char *child_relpath = svn_sqlite__column_text(stmt, 0, NULL);
- const char *name = svn_relpath_basename(child_relpath, result_pool);
- apr_hash_set(children, name, APR_HASH_KEY_STRING, name);
+ /* Allocate the name in RESULT_POOL so we won't have to copy it. */
+ APR_ARRAY_PUSH(result, const char *)
+ = svn_relpath_basename(child_relpath, result_pool);
SVN_ERR(svn_sqlite__step(&have_row, stmt));
}
+ SVN_ERR(svn_sqlite__reset(stmt));
- return svn_sqlite__reset(stmt);
+ *children = result;
+ return SVN_NO_ERROR;
}
-/* Return in *CHILDREN all of the children of the directory LOCAL_RELPATH.
- If BASE_ONLY is true, then *only* the children from BASE_NODE are
- returned (those in WORKING_NODE are ignored). The result children are
- allocated in RESULT_POOL. */
+/* Return in *CHILDREN all of the children of the directory LOCAL_RELPATH,
+ of any status, in all op-depths in the NODES table. */
static svn_error_t *
gather_children(const apr_array_header_t **children,
- svn_boolean_t base_only,
svn_wc__db_pdh_t *pdh,
const char *local_relpath,
apr_pool_t *result_pool,
@@ -1080,17 +1085,25 @@ gather_children(const apr_array_header_t
apr_hash_t *names_hash = apr_hash_make(scratch_pool);
apr_array_header_t *names_array;
- /* All of the names get allocated in RESULT_POOL. For !base_only it
+ /* All of the names get allocated in RESULT_POOL. It
appears to be faster to use the hash to remove duplicates than to
use DISTINCT in the SQL query. */
- if (base_only)
- SVN_ERR(add_children_to_hash(names_hash, STMT_SELECT_BASE_NODE_CHILDREN,
- pdh->wcroot->sdb, pdh->wcroot->wc_id,
- local_relpath, result_pool));
- else
- SVN_ERR(add_children_to_hash(names_hash, STMT_SELECT_NODE_CHILDREN,
- pdh->wcroot->sdb, pdh->wcroot->wc_id,
- local_relpath, result_pool));
+ svn_sqlite__stmt_t *stmt;
+ svn_boolean_t have_row;
+
+ SVN_ERR(svn_sqlite__get_statement(&stmt, pdh->wcroot->sdb,
+ STMT_SELECT_NODE_CHILDREN));
+ SVN_ERR(svn_sqlite__bindf(stmt, "is", pdh->wcroot->wc_id, local_relpath));
+ SVN_ERR(svn_sqlite__step(&have_row, stmt));
+ while (have_row)
+ {
+ const char *child_relpath = svn_sqlite__column_text(stmt, 0, NULL);
+ const char *name = svn_relpath_basename(child_relpath, result_pool);
+
+ apr_hash_set(names_hash, name, APR_HASH_KEY_STRING, name);
+
+ SVN_ERR(svn_sqlite__step(&have_row, stmt));
+ }
SVN_ERR(svn_hash_keys(&names_array, names_hash, result_pool));
*children = names_array;
@@ -2166,8 +2179,8 @@ svn_wc__db_base_get_children(const apr_a
scratch_pool, scratch_pool));
VERIFY_USABLE_PDH(pdh);
- return gather_children(children, TRUE,
- pdh, local_relpath, result_pool, scratch_pool);
+ return gather_repo_children(children, pdh, local_relpath, 0,
+ result_pool, scratch_pool);
}
@@ -2975,6 +2988,15 @@ get_info_for_copy(apr_int64_t *copyfrom_
return SVN_NO_ERROR;
}
+static svn_error_t *
+op_depth_for_copy(apr_int64_t *op_depth,
+ apr_int64_t copyfrom_repos_id,
+ const char *copyfrom_relpath,
+ svn_revnum_t copyfrom_revision,
+ svn_wc__db_pdh_t *pdh,
+ const char *local_relpath,
+ apr_pool_t *scratch_pool);
+
/* Like svn_wc__db_op_copy(), but with PDH+LOCAL_RELPATH instead of
* DB+LOCAL_ABSPATH. */
static svn_error_t *
@@ -2982,7 +3004,6 @@ db_op_copy(svn_wc__db_pdh_t *src_pdh,
const char *src_relpath,
svn_wc__db_pdh_t *dst_pdh,
const char *dst_relpath,
- apr_int64_t dst_op_depth,
const svn_skel_t *work_items,
apr_pool_t *scratch_pool)
{
@@ -2991,6 +3012,7 @@ db_op_copy(svn_wc__db_pdh_t *src_pdh,
svn_wc__db_status_t status, dst_status;
svn_boolean_t have_work;
apr_int64_t copyfrom_id;
+ apr_int64_t dst_op_depth;
svn_wc__db_kind_t kind;
const apr_array_header_t *children;
@@ -2998,6 +3020,10 @@ db_op_copy(svn_wc__db_pdh_t *src_pdh,
&status, &kind, &have_work,
src_pdh, src_relpath, scratch_pool, scratch_pool));
+ SVN_ERR(op_depth_for_copy(&dst_op_depth, copyfrom_id,
+ copyfrom_relpath, copyfrom_rev,
+ dst_pdh, dst_relpath, scratch_pool));
+
SVN_ERR_ASSERT(kind == svn_wc__db_kind_file || kind == svn_wc__db_kind_dir);
/* ### New status, not finished, see notes/wc-ng/copying */
@@ -3031,8 +3057,8 @@ db_op_copy(svn_wc__db_pdh_t *src_pdh,
}
if (kind == svn_wc__db_kind_dir)
- SVN_ERR(gather_children(&children, FALSE, src_pdh, src_relpath,
- scratch_pool, scratch_pool));
+ SVN_ERR(gather_repo_children(&children, src_pdh, src_relpath, dst_op_depth,
+ scratch_pool, scratch_pool));
else
children = NULL;
@@ -3072,6 +3098,10 @@ db_op_copy(svn_wc__db_pdh_t *src_pdh,
dst_relpath, dst_parent_relpath));
SVN_ERR(svn_sqlite__step_done(stmt));
+ /* Insert incomplete children, if relevant.
+ The children are part of the same op and so have the same op_depth.
+ (The only time we'd want a different depth is during a recursive
+ simple add, but we never insert children here during a simple add.) */
if (kind == svn_wc__db_kind_dir)
SVN_ERR(insert_incomplete_children(dst_pdh->wcroot->sdb,
dst_pdh->wcroot->wc_id,
@@ -3112,10 +3142,6 @@ svn_wc__db_op_copy(svn_wc__db_t *db,
{
svn_wc__db_pdh_t *src_pdh, *dst_pdh;
const char *src_relpath, *dst_relpath;
-#ifdef SVN_WC__OP_DEPTH
- const char *dst_op_root_relpath;
-#endif
- apr_int64_t dst_op_depth;
SVN_ERR_ASSERT(svn_dirent_is_absolute(src_abspath));
SVN_ERR_ASSERT(svn_dirent_is_absolute(dst_abspath));
@@ -3132,19 +3158,9 @@ svn_wc__db_op_copy(svn_wc__db_t *db,
scratch_pool, scratch_pool));
VERIFY_USABLE_PDH(dst_pdh);
-#ifdef SVN_WC__OP_DEPTH
- SVN_ERR(svn_wc__db_pdh_parse_local_abspath(&dst_pdh, &dst_op_root_relpath,
- db, dst_op_root_abspath,
- svn_sqlite__mode_readwrite,
- scratch_pool, scratch_pool));
- dst_op_depth = relpath_depth(dst_op_root_relpath);
-#else
- dst_op_depth = 2; /* ### temporary op_depth */
-#endif
-
/* ### This should all happen in one transaction. */
SVN_ERR(db_op_copy(src_pdh, src_relpath, dst_pdh, dst_relpath,
- dst_op_depth, work_items, scratch_pool));
+ work_items, scratch_pool));
return SVN_NO_ERROR;
}
@@ -5844,7 +5860,7 @@ svn_wc__db_read_children(const apr_array
scratch_pool, scratch_pool));
VERIFY_USABLE_PDH(pdh);
- return gather_children(children, FALSE,
+ return gather_children(children,
pdh, local_relpath, result_pool, scratch_pool);
}
@@ -8907,8 +8923,8 @@ make_copy_txn(void *baton,
SVN_ERR(svn_sqlite__reset(stmt));
/* Get the BASE children, as WORKING children don't need modifications */
- SVN_ERR(gather_children(&children, TRUE, mcb->pdh, mcb->local_relpath,
- scratch_pool, iterpool));
+ SVN_ERR(gather_repo_children(&children, mcb->pdh, mcb->local_relpath, 0,
+ scratch_pool, iterpool));
for (i = 0; i < children->nelts; i++)
{
Index: subversion/libsvn_wc/wc-queries.sql
===================================================================
--- subversion/libsvn_wc/wc-queries.sql (revision 1038202)
+++ subversion/libsvn_wc/wc-queries.sql (working copy)
@@ -132,9 +132,9 @@ INSERT OR REPLACE INTO nodes (
VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13, ?14,
?15, ?16, ?17, ?18, ?19);
--- STMT_SELECT_BASE_NODE_CHILDREN
+-- STMT_SELECT_OP_DEPTH_CHILDREN
SELECT local_relpath FROM nodes
-WHERE wc_id = ?1 AND parent_relpath = ?2 AND op_depth = 0;
+WHERE wc_id = ?1 AND parent_relpath = ?2 AND op_depth = ?3;
-- STMT_SELECT_NODE_CHILDREN
SELECT local_relpath FROM nodes
