If I ask a WC API for "the children" of a working directory, I would
expect to get just the children that belong to it, not including any
paths that were children of an underlying directory that has been
locally replaced.
The attached patch starts this by creating a test and defining a new
version of one of the APIs. (Only the API description is different, the
signature is the same.)
Sanity checks appreciated, especially of the test. I'll describe the
test.
I start with a repo and clean WC at revision 1 containing just the
following directories (no files):
A
A/F
A/G
A/H
X
X/G
X/H
X/I
and then (in Subversion) I replace A with a copy of X:
rm A
cp X A
and then delete the child 'H' and add a new child 'J':
rm A/G
mkdir A/J
I expect to the "working view" of A to consist of
A - copied, replacing an old 'A'
A/G - scheduled for delete (was a copied child of 'A')
A/H - copied (as a child of 'A')
A/I - copied (as a child of 'A')
A/J - scheduled for add
Note that I don't expect to hear anything about the path 'A/F'.
Currently, 'F' is included in the results returned by
svn_wc__db_read_children() and svn_wc__db_read_children_info().
On the right track?
- Julian
p.s. The last part of the test - calling svn_wc__node_get_children() -
isn't working right yet. It returns abspaths where the test expects
relpaths.
w|loc|o|p|r|rep|r|presence|||kind||depth ||| | | |
1|A |0| |1|A |1|normal |||dir ||infinity|||1|1299779887071167|||||
1|A/F|0|A|1|A/F|1|normal |||dir ||infinity|||1|1299779887071167|||||
1|A/G|0|A|1|A/G|1|normal |||dir ||infinity|||1|1299779887071167|||||
1|A/H|0|A|1|A/H|1|normal |||dir ||infinity|||1|1299779887071167|||||
1|A |1| |1|X |1|normal |||dir ||infinity|||1|1299779887071167|||||
1|A/F|1|A|-|- |-|base-del|||dir ||infinity|||1|1299779887071167|||||
1|A/G|1|A|1|X/G|1|normal |||dir ||infinity|||1|1299779887071167|||||
1|A/H|1|A|1|X/H|1|normal |||dir ||infinity|||1|1299779887071167|||||
1|A/I|1|A|1|X/I|1|normal |||dir ||infinity|||1|1299779887071167|||||
1|A/H|2|A|-|- |-|base-del|||dir ||infinity|||1|1299779887071167|||||
1|A/J|2|A|-|- |-|normal |||dir ||infinity|||-|0|||||
### Patch in progress.
Work towards sane behaviour for reading the set of children of a replaced
directory: we would expect children of any underlying (replaced) directories
to be ignored. Add a new WC test for that behaviour. Define a new version
of the svn_wc__db_read_children() API that promises that behaviour.
* subversion/libsvn_wc/wc_db.h
(svn_wc__db_read_children_info): Add a note about unexpected behaviour.
(svn_wc__db_read_children): Deprecate. Add a note about unexpected behaviour.
### TODO: Either add some deprecation mark-up or update all callers to
use the new version and don't rev. its name.
(svn_wc__db_read_children2): New function.
* subversion/tests/libsvn_wc/op-depth-test.c
(check_hash_keys, check_array_strings): New functions.
(CHECK_HASH, CHECK_ARRAY): New macros.
(test_children_of_replace): New test function.
(test_funcs): Add the new test.
--This line, and those below, will be ignored--
Index: subversion/libsvn_wc/wc_db.h
===================================================================
--- subversion/libsvn_wc/wc_db.h (revision 1080298)
+++ subversion/libsvn_wc/wc_db.h (working copy)
@@ -1509,7 +1509,13 @@ struct svn_wc__db_info_t {
/* Return in *NODES a hash mapping name->struct svn_wc__db_info_t for
the children of DIR_ABSPATH, and in *CONFLICTS a hash of names in
- conflict. */
+ conflict.
+
+ ### The implementation currently returns all children that exist in the
+ working view and all children that are recorded at any op-depth -- in
+ other words, even children of replaced directories. This is not
+ generally what we want.
+ */
svn_error_t *
svn_wc__db_read_children_info(apr_hash_t **nodes,
apr_hash_t **conflicts,
@@ -1635,8 +1641,8 @@ svn_wc__db_read_pristine_props(apr_hash_
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
-/* Read into CHILDREN the basenames of the immediate children of
- LOCAL_ABSPATH in DB.
+/* Set *CHILDREN to a new array containing the (const char *) basenames of
+ the immediate children of LOCAL_ABSPATH in DB.
Allocate *CHILDREN in RESULT_POOL and do temporary allocations in
SCRATCH_POOL.
@@ -1649,6 +1655,18 @@ svn_wc__db_read_pristine_props(apr_hash_
### computing all info.
*/
svn_error_t *
+svn_wc__db_read_children2(const apr_array_header_t **children,
+ svn_wc__db_t *db,
+ const char *local_abspath,
+ apr_pool_t *result_pool,
+ apr_pool_t *scratch_pool);
+
+/* Similar to svn_wc__db_read_children2(), except it returns not only
+ children in the working view but children that are recorded at any
+ op-depth, which includes children of replaced directories.
+ (This is not generally what we want.)
+*/
+svn_error_t *
svn_wc__db_read_children(const apr_array_header_t **children,
svn_wc__db_t *db,
const char *local_abspath,
Index: subversion/tests/libsvn_wc/op-depth-test.c
===================================================================
--- subversion/tests/libsvn_wc/op-depth-test.c (revision 1080270)
+++ subversion/tests/libsvn_wc/op-depth-test.c (working copy)
@@ -2552,6 +2552,131 @@ test_op_revert_changelist(const svn_test
return SVN_NO_ERROR;
}
+/* Check that the (const char *) keys of HASH are exactly the
+ * EXPECTED_NUM strings in EXPECTED_STRINGS. Return an error if not. */
+static svn_error_t *
+check_hash_keys(apr_hash_t *hash,
+ int expected_num,
+ const char **expected_strings,
+ apr_pool_t *scratch_pool)
+{
+ int i;
+
+ SVN_TEST_ASSERT(apr_hash_count(hash) == expected_num);
+ for (i = 0; i < expected_num; i++)
+ {
+ const char *name = expected_strings[i];
+
+ SVN_TEST_ASSERT(apr_hash_get(hash, name, APR_HASH_KEY_STRING));
+ apr_hash_set(hash, name, APR_HASH_KEY_STRING, NULL);
+ }
+ SVN_TEST_ASSERT(apr_hash_count(hash) == 0);
+ return SVN_NO_ERROR;
+}
+
+/* Check that the (const char *) keys of APR_HASH are exactly the
+ * strings in (const char *[]) C_ARRAY. Return an error if not. */
+#define CHECK_HASH(apr_hash, c_array, scratch_pool) \
+ check_hash_keys(apr_hash, sizeof(c_array) / sizeof(c_array[0]), \
+ c_array, scratch_pool)
+
+/* Check that the (const char *) elements of ARRAY are exactly the
+ * EXPECTED_NUM strings in EXPECTED_STRINGS. Return an error if not. */
+static svn_error_t *
+check_array_strings(const apr_array_header_t *array,
+ int expected_num,
+ const char **expected_strings,
+ apr_pool_t *scratch_pool)
+{
+ int i;
+ apr_hash_t *hash = apr_hash_make(scratch_pool);
+
+ for (i = 0; i < array->nelts; i++)
+ {
+ const char *name = APR_ARRAY_IDX(array, i, const char *);
+
+ apr_hash_set(hash, name, APR_HASH_KEY_STRING, "");
+ }
+
+ return check_hash_keys(hash, expected_num, expected_strings, scratch_pool);
+}
+
+/* Check that the (const char *) elements of APR_ARRAY are exactly the
+ * strings in (const char *[]) C_ARRAY. Return an error if not. */
+#define CHECK_ARRAY(apr_array, c_array, scratch_pool) \
+ check_array_strings(apr_array, sizeof(c_array) / sizeof(c_array[0]), \
+ c_array, scratch_pool)
+
+
+static svn_error_t *
+test_children_of_replace(const svn_test_opts_t *opts, apr_pool_t *pool)
+{
+ svn_test__sandbox_t b;
+ const apr_array_header_t *children_array;
+ apr_hash_t *children_hash, *conflicts_hash;
+ const char *A_abspath;
+ const char *base_children[] = { "F", "G", "H" };
+ const char *visible_children[] = { "H", "I", "J" };
+ const char *working_children_incl_del[] = { "G", "H", "I", "J" };
+ /*
+ * F - base only
+ * G - base, working (from copy of X), schedule-delete
+ * H - base, working (from copy of X)
+ * I - working only (from copy of X)
+ * J - working only, schedule-add
+ */
+
+ SVN_ERR(svn_test__sandbox_create(&b, "children_of_replace", opts, pool));
+ A_abspath = svn_dirent_join(b.wc_abspath, "A", pool);
+
+ /* Set up the base state as revision 1. */
+ SVN_ERR(wc_mkdir(&b, "A"));
+ SVN_ERR(wc_mkdir(&b, "A/F"));
+ SVN_ERR(wc_mkdir(&b, "A/G"));
+ SVN_ERR(wc_mkdir(&b, "A/H"));
+ SVN_ERR(wc_mkdir(&b, "X"));
+ SVN_ERR(wc_mkdir(&b, "X/G"));
+ SVN_ERR(wc_mkdir(&b, "X/H"));
+ SVN_ERR(wc_mkdir(&b, "X/I"));
+ SVN_ERR(wc_commit(&b, ""));
+ SVN_ERR(wc_update(&b, "", 1));
+
+ /* Replace A with a copy of X. */
+ SVN_ERR(wc_delete(&b, "A"));
+ SVN_ERR(wc_copy(&b, "X", "A"));
+
+ /* Make other local mods. */
+ SVN_ERR(wc_delete(&b, "A/G"));
+ SVN_ERR(wc_mkdir(&b, "A/J"));
+
+ /* Test several variants of "list the children of 'A'". */
+
+ SVN_ERR(svn_wc__db_base_get_children(&children_array,
+ b.wc_ctx->db, A_abspath,
+ pool, pool));
+ SVN_ERR(CHECK_ARRAY(children_array, base_children, pool));
+
+ SVN_ERR(svn_wc__db_read_children2(&children_array,
+ b.wc_ctx->db, A_abspath,
+ pool, pool));
+ SVN_ERR(CHECK_ARRAY(children_array, working_children_incl_del, pool));
+
+ SVN_ERR(svn_wc__db_read_children_info(&children_hash, &conflicts_hash,
+ b.wc_ctx->db, A_abspath,
+ pool, pool));
+ SVN_ERR(CHECK_HASH(children_hash, working_children_incl_del, pool));
+
+ SVN_ERR(svn_wc__node_get_children(&children_array, b.wc_ctx, A_abspath,
+ FALSE /* show_hidden */, pool, pool));
+ SVN_ERR(CHECK_ARRAY(children_array, visible_children, pool));
+
+ SVN_ERR(svn_wc__node_get_children(&children_array, b.wc_ctx, A_abspath,
+ TRUE /* show_hidden */, pool, pool));
+ SVN_ERR(CHECK_ARRAY(children_array, working_children_incl_del, pool));
+
+ return SVN_NO_ERROR;
+}
+
/* ---------------------------------------------------------------------- */
/* The list of test functions */
@@ -2592,5 +2717,7 @@ struct svn_test_descriptor_t test_funcs[
"test_op_revert"),
SVN_TEST_OPTS_PASS(test_op_revert_changelist,
"test_op_revert_changelist"),
+ SVN_TEST_OPTS_PASS(test_children_of_replace,
+ "test_children_of_replace"),
SVN_TEST_NULL
};
