You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2014/01/14 23:39:10 UTC
RE: svn commit: r1558224 - in
/subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c
tests/libsvn_fs/fs-test.c
How do we now check that property changes don't conflict?
Before this patch we assumed that the client code would do the verification work. Are property changes now just overwritten?
Or is your patch limited to one round of property changes?
I think changes like this need more documentation than just mentioning that it is a scalability issue that can be fixed 'easily'. There were quite a few design decisions behind this limitation when I looked at it a few years ago.
This might break some of the promises that make the 'incomplete' handling capable of resuming an update. Both the status of the properties and entries of the directory are tied to that.
Bert
-----Original Message-----
From: "stefan2@apache.org" <st...@apache.org>
Sent: 14-1-2014 23:09
To: "commits@subversion.apache.org" <co...@subversion.apache.org>
Subject: svn commit: r1558224 - in /subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c
Author: stefan2
Date: Tue Jan 14 22:09:01 2014
New Revision: 1558224
URL: http://svn.apache.org/r1558224
Log:
For FSFS and FSX, fix a major merge scalability issue described here:
http://svn.haxx.se/dev/archive-2014-01/0057.shtml
The bottom line is that we allow incoming prop changes on directories
that have no addition nor deletion of entries relative to the TXN
base revision. This patch updates existing tests and provided a
specific new one.
* subversion/libsvn_fs_fs/tree.c
(compare_dir_structure): A new utility function checking that all
dirents are still the same, although
possibly at different revisions.
(merge): An dir prop change in a txn should not conflict with
simply node upgrades within that directory.
* subversion/libsvn_fs_x/tree.c
(compare_dir_structure,
merge): Similar implementation as for FSFS with a slight adaptation
due to different directory container types.
* subversion/tests/libsvn_fs/fs-test.c
(unordered_txn_dirprops): Update expected results.
(dir_prop_merge): New test.
(test_compat_version): Register the new test.
Modified:
subversion/trunk/subversion/libsvn_fs_fs/tree.c
subversion/trunk/subversion/libsvn_fs_x/tree.c
subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Tue Jan 14 22:09:01 2014
@@ -1654,6 +1654,53 @@ conflict_err(svn_stringbuf_t *conflict_p
_("Conflict at '%s'"), path);
}
+/* Compare the directory representations at nodes LHS and RHS and set
+ * *CHANGED to TRUE, if at least one entry has been added or removed them.
+ * Use POOL for temporary allocations.
+ */
+static svn_error_t *
+compare_dir_structure(svn_boolean_t *changed,
+ dag_node_t *lhs,
+ dag_node_t *rhs,
+ apr_pool_t *pool)
+{
+ apr_array_header_t *lhs_entries;
+ apr_array_header_t *rhs_entries;
+ int i;
+
+ SVN_ERR(svn_fs_fs__dag_dir_entries(&lhs_entries, lhs, pool));
+ SVN_ERR(svn_fs_fs__dag_dir_entries(&rhs_entries, rhs, pool));
+
+ /* different number of entries -> some addition / removal */
+ if (lhs_entries->nelts != rhs_entries->nelts)
+ {
+ *changed = TRUE;
+ return SVN_NO_ERROR;
+ }
+
+ /* Since directories are sorted by name, we can simply compare their
+ entries one-by-one without binary lookup etc. */
+ for (i = 0; i < lhs_entries->nelts; ++i)
+ {
+ svn_fs_dirent_t *lhs_entry
+ = APR_ARRAY_IDX(lhs_entries, i, svn_fs_dirent_t *);
+ svn_fs_dirent_t *rhs_entry
+ = APR_ARRAY_IDX(rhs_entries, i, svn_fs_dirent_t *);
+
+ if (strcmp(lhs_entry->name, rhs_entry->name)
+ || !svn_fs_fs__id_part_eq(svn_fs_fs__id_node_id(lhs_entry->id),
+ svn_fs_fs__id_node_id(rhs_entry->id))
+ || !svn_fs_fs__id_part_eq(svn_fs_fs__id_copy_id(lhs_entry->id),
+ svn_fs_fs__id_copy_id(rhs_entry->id)))
+ {
+ *changed = TRUE;
+ return SVN_NO_ERROR;
+ }
+ }
+
+ *changed = FALSE;
+ return SVN_NO_ERROR;
+}
/* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
* and TARGET must be distinct node revisions. TARGET_PATH should
@@ -1828,10 +1875,23 @@ merge(svn_stringbuf_t *conflict_p,
it doesn't do a brute-force comparison on textual contents, so
it won't do that here either. Checking to see if the propkey
atoms are `equal' is enough. */
- if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
- return conflict_err(conflict_p, target_path);
if (! svn_fs_fs__noderev_same_rep_key(src_nr->prop_rep, anc_nr->prop_rep))
return conflict_err(conflict_p, target_path);
+
+ /* The directory entries got changed in the repository but the directory
+ properties did not. */
+ if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
+ {
+ /* There is an incoming prop change for this directory.
+ We will accept it only if the directory changes were mere updates
+ to its entries, i.e. there were no additions or removals.
+ Those could cause update problems to the working copy. */
+ svn_boolean_t changed;
+ SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
+
+ if (changed)
+ return conflict_err(conflict_p, target_path);
+ }
}
/* ### todo: it would be more efficient to simply check for a NULL
Modified: subversion/trunk/subversion/libsvn_fs_x/tree.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_x/tree.c (original)
+++ subversion/trunk/subversion/libsvn_fs_x/tree.c Tue Jan 14 22:09:01 2014
@@ -1630,6 +1630,56 @@ conflict_err(svn_stringbuf_t *conflict_p
_("Conflict at '%s'"), path);
}
+/* Compare the directory representations at nodes LHS and RHS and set
+ * *CHANGED to TRUE, if at least one entry has been added or removed them.
+ * Use POOL for temporary allocations.
+ */
+static svn_error_t *
+compare_dir_structure(svn_boolean_t *changed,
+ dag_node_t *lhs,
+ dag_node_t *rhs,
+ apr_pool_t *pool)
+{
+ apr_hash_t *lhs_entries;
+ apr_hash_t *rhs_entries;
+ apr_hash_index_t *hi;
+
+ SVN_ERR(svn_fs_x__dag_dir_entries(&lhs_entries, lhs, pool));
+ SVN_ERR(svn_fs_x__dag_dir_entries(&rhs_entries, rhs, pool));
+
+ /* different number of entries -> some addition / removal */
+ if (apr_hash_count(lhs_entries) != apr_hash_count(rhs_entries))
+ {
+ *changed = TRUE;
+ return SVN_NO_ERROR;
+ }
+
+ /* Since the number of dirents is the same, we simply need to do a
+ one-sided comparison. */
+ for (hi = apr_hash_first(pool, lhs_entries); hi; hi = apr_hash_next(hi))
+ {
+ svn_fs_dirent_t *lhs_entry;
+ svn_fs_dirent_t *rhs_entry;
+ const char *name;
+ apr_ssize_t klen;
+
+ apr_hash_this(hi, (const void **)&name, &klen, (void **)&lhs_entry);
+ rhs_entry = apr_hash_get(rhs_entries, name, klen);
+
+ if (!rhs_entry
+ || !svn_fs_x__id_part_eq(svn_fs_x__id_node_id(lhs_entry->id),
+ svn_fs_x__id_node_id(rhs_entry->id))
+ || !svn_fs_x__id_part_eq(svn_fs_x__id_copy_id(lhs_entry->id),
+ svn_fs_x__id_copy_id(rhs_entry->id)))
+ {
+ *changed = TRUE;
+ return SVN_NO_ERROR;
+ }
+ }
+
+ *changed = FALSE;
+ return SVN_NO_ERROR;
+}
/* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
* and TARGET must be distinct node revisions. TARGET_PATH should
@@ -1803,10 +1853,23 @@ merge(svn_stringbuf_t *conflict_p,
it doesn't do a brute-force comparison on textual contents, so
it won't do that here either. Checking to see if the propkey
atoms are `equal' is enough. */
- if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
- return conflict_err(conflict_p, target_path);
if (! svn_fs_x__noderev_same_rep_key(src_nr->prop_rep, anc_nr->prop_rep))
return conflict_err(conflict_p, target_path);
+
+ /* The directory entries got changed in the repository but the directory
+ properties did not. */
+ if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep, anc_nr->prop_rep))
+ {
+ /* There is an incoming prop change for this directory.
+ We will accept it only if the directory changes were mere updates
+ to its entries, i.e. there were no additions or removals.
+ Those could cause update problems to the working copy. */
+ svn_boolean_t changed;
+ SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
+
+ if (changed)
+ return conflict_err(conflict_p, target_path);
+ }
}
/* ### todo: it would be more efficient to simply check for a NULL
Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1558224&r1=1558223&r2=1558224&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue Jan 14 22:09:01 2014
@@ -4582,6 +4582,7 @@ unordered_txn_dirprops(const svn_test_op
svn_fs_root_t *txn_root, *txn_root2;
svn_string_t pval;
svn_revnum_t new_rev, not_rev;
+ svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
/* This is a regression test for issue #2751. */
@@ -4638,10 +4639,21 @@ unordered_txn_dirprops(const svn_test_op
/* Commit the first one first. */
SVN_ERR(test_commit_txn(&new_rev, txn, NULL, pool));
- /* Then commit the second -- but expect an conflict because the
- directory wasn't up-to-date, which is required for propchanges. */
- SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
- SVN_ERR(svn_fs_abort_txn(txn2, pool));
+ /* Some backends are clever then others. */
+ if (is_bdb)
+ {
+ /* Then commit the second -- but expect an conflict because the
+ directory wasn't up-to-date, which is required for propchanges. */
+ SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
+ SVN_ERR(svn_fs_abort_txn(txn2, pool));
+ }
+ else
+ {
+ /* Then commit the second -- there will be no conflict despite the
+ directory being out-of-data because the properties as well as the
+ directory structure (list of nodes) was up-to-date. */
+ SVN_ERR(test_commit_txn(¬_rev, txn2, NULL, pool));
+ }
return SVN_NO_ERROR;
}
@@ -5222,6 +5234,80 @@ test_compat_version(const svn_test_opts_
return SVN_NO_ERROR;
}
+static svn_error_t *
+dir_prop_merge(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_revnum_t head_rev;
+ svn_fs_root_t *root;
+ svn_fs_txn_t *txn, *mid_txn, *top_txn, *sub_txn, *c_txn;
+ svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
+
+ /* Create test repository. */
+ SVN_ERR(svn_test__create_fs(&fs, "test-dir_prop-merge", opts, pool));
+
+ SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, txn, pool));
+
+ /* Create and verify the greek tree. */
+ SVN_ERR(svn_test__create_greek_tree(root, pool));
+ SVN_ERR(test_commit_txn(&head_rev, txn, NULL, pool));
+
+ /* Start concurrent transactions */
+
+ /* 1st: modify a mid-level directory */
+ SVN_ERR(svn_fs_begin_txn2(&mid_txn, fs, head_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, mid_txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
+ svn_string_create("val1", pool), pool));
+ svn_fs_close_root(root);
+
+ /* 2st: modify a top-level directory */
+ SVN_ERR(svn_fs_begin_txn2(&top_txn, fs, head_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, top_txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "A", "test-prop",
+ svn_string_create("val2", pool), pool));
+ svn_fs_close_root(root);
+
+ SVN_ERR(svn_fs_begin_txn2(&sub_txn, fs, head_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, sub_txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "A/D/G", "test-prop",
+ svn_string_create("val3", pool), pool));
+ svn_fs_close_root(root);
+
+ /* 3rd: modify a conflicting change to the mid-level directory */
+ SVN_ERR(svn_fs_begin_txn2(&c_txn, fs, head_rev, 0, pool));
+ SVN_ERR(svn_fs_txn_root(&root, c_txn, pool));
+ SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
+ svn_string_create("valX", pool), pool));
+ svn_fs_close_root(root);
+
+ /* Prop changes to the same node should conflict */
+ SVN_ERR(test_commit_txn(&head_rev, mid_txn, NULL, pool));
+ SVN_ERR(test_commit_txn(&head_rev, c_txn, "/A/D", pool));
+ SVN_ERR(svn_fs_abort_txn(c_txn, pool));
+
+ /* Changes in a sub-tree should not conflict with prop changes to some
+ parent directory but some backends are clever then others. */
+ if (is_bdb)
+ {
+ SVN_ERR(test_commit_txn(&head_rev, top_txn, "/A", pool));
+ SVN_ERR(svn_fs_abort_txn(top_txn, pool));
+ }
+ else
+ {
+ SVN_ERR(test_commit_txn(&head_rev, top_txn, NULL, pool));
+ }
+
+ /* The inverted case is not that trivial to handle. Hence, conflict.
+ Depending on the checking order, the reported conflict path differs. */
+ SVN_ERR(test_commit_txn(&head_rev, sub_txn, is_bdb ? "/A/D" : "/A", pool));
+ SVN_ERR(svn_fs_abort_txn(sub_txn, pool));
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
@@ -5315,5 +5401,7 @@ struct svn_test_descriptor_t test_funcs[
"commit timestamp"),
SVN_TEST_OPTS_PASS(test_compat_version,
"test svn_fs__compatible_version"),
+ SVN_TEST_OPTS_PASS(dir_prop_merge,
+ "test merge directory properties"),
SVN_TEST_NULL
};
Re: svn commit: r1558224 - in /subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@wandisco.com>.
Stefan Fuhrmann <st...@wandisco.com> writes:
> As you already figured out, there is an explicit check in
> the repos layer that prevents property changes on dirs
> that are not at the txn base revision, i.e. node-HEAD at the
> time of the begin of that txns. The reason is that the API
> does not actually have a "change prop" but only a "set
> prop" function. Prop changes to the same node between
> wc BASE and txn base would simply be lost.
The repos/RA layers could implement the more relaxed check of the FS
layer but it is probably not a trivial change, and it would not match
the BDB backend.
> So, we still have a theoretical scalability issue but now
> it is the time a "svn up --depth=empty" at the root node
> (in theory a constant value) that should be less than the
> average time between commits.
Agreed, the current code does improve concurrency. In my example:
$ svn st wc1
M wc1/f
$ svn st wc2
M wc2
M wc2/g
With 1.8 the commit of wc2 fails if wc1 is committed first. With trunk
wc2 still fails if wc1 finishes before wc2 starts. The new behaviour is
that if wc1 and wc2 start in parallel, so that wc2 passes the repos/RA
checks before wc1 finishes, then wc1 can finish before wc2 and wc2 still
succeeds.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1558224 - in /subversion/trunk/subversion:libsvn_fs_fs/tree.c
libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jan 15, 2014 at 12:07 PM, Philip Martin
<ph...@wandisco.com>wrote:
> This change doesn't work over RA the way I would expect. I'd expect
> these two commits to work:
>
> svnadmin create repo
> svn import -mm repo/format file://`pwd`/repo/A/f
> svn import -mm repo/format file://`pwd`/repo/A/g
> svn co http://localhost:8888/obj/repo/A wc1
> echo 1 >> wc1/f
> svn co http://localhost:8888/obj/repo/A wc2
> svn ps p v wc2
> echo 2 >> wc2/g
>
> $ svn st wc1
> M wc1/f
>
> $ svn st wc2
> M wc2
> M wc2/g
>
> But the commit fails with an out-of-date error before reaching the FS
> layer:
>
> $ svn ci -mm wc1
> Transmitting file data .
> Committed revision 3
> $ svn ci -mm wc2
> Sending wc2
> Sending wc2/g
> ../src/subversion/svn/commit-cmd.c:182,
> ../src/subversion/libsvn_client/commit.c:1076,
> ../src/subversion/libsvn_client/commit.c:154:
> (apr_err=SVN_ERR_RA_OUT_OF_DATE)
> svn: E170004: Commit failed (details follow):
> ../src/subversion/libsvn_client/commit.c:991,
> ../src/subversion/libsvn_client/commit_util.c:1884,
> ../src/subversion/libsvn_delta/path_driver.c:294,
> ../src/subversion/libsvn_delta/path_driver.c:100,
> ../src/subversion/libsvn_ra_serf/commit.c:1775,
> ../src/subversion/libsvn_ra_serf/commit.c:934,
> ../src/subversion/libsvn_ra_serf/util.c:933,
> ../src/subversion/libsvn_ra_serf/util.c:907,
> ../src/subversion/libsvn_ra_serf/util.c:872,
> ../src/subversion/libsvn_ra_serf/multistatus.c:560:
> (apr_err=SVN_ERR_RA_OUT_OF_DATE)
> svn: E170004: Directory '/A' is out of date
>
I am aware that this limitation still exists. The patch only
handles concurrent transactions but does not address
the out-of-date working copy problem.
As you already figured out, there is an explicit check in
the repos layer that prevents property changes on dirs
that are not at the txn base revision, i.e. node-HEAD at the
time of the begin of that txns. The reason is that the API
does not actually have a "change prop" but only a "set
prop" function. Prop changes to the same node between
wc BASE and txn base would simply be lost.
So, we still have a theoretical scalability issue but now
it is the time a "svn up --depth=empty" at the root node
(in theory a constant value) that should be less than the
average time between commits.
-- Stefan^2.
Re: svn commit: r1558224 - in /subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c
Posted by Philip Martin <ph...@wandisco.com>.
This change doesn't work over RA the way I would expect. I'd expect
these two commits to work:
svnadmin create repo
svn import -mm repo/format file://`pwd`/repo/A/f
svn import -mm repo/format file://`pwd`/repo/A/g
svn co http://localhost:8888/obj/repo/A wc1
echo 1 >> wc1/f
svn co http://localhost:8888/obj/repo/A wc2
svn ps p v wc2
echo 2 >> wc2/g
$ svn st wc1
M wc1/f
$ svn st wc2
M wc2
M wc2/g
But the commit fails with an out-of-date error before reaching the FS
layer:
$ svn ci -mm wc1
Transmitting file data .
Committed revision 3
$ svn ci -mm wc2
Sending wc2
Sending wc2/g
../src/subversion/svn/commit-cmd.c:182,
../src/subversion/libsvn_client/commit.c:1076,
../src/subversion/libsvn_client/commit.c:154: (apr_err=SVN_ERR_RA_OUT_OF_DATE)
svn: E170004: Commit failed (details follow):
../src/subversion/libsvn_client/commit.c:991,
../src/subversion/libsvn_client/commit_util.c:1884,
../src/subversion/libsvn_delta/path_driver.c:294,
../src/subversion/libsvn_delta/path_driver.c:100,
../src/subversion/libsvn_ra_serf/commit.c:1775,
../src/subversion/libsvn_ra_serf/commit.c:934,
../src/subversion/libsvn_ra_serf/util.c:933,
../src/subversion/libsvn_ra_serf/util.c:907,
../src/subversion/libsvn_ra_serf/util.c:872,
../src/subversion/libsvn_ra_serf/multistatus.c:560: (apr_err=SVN_ERR_RA_OUT_OF_DATE)
svn: E170004: Directory '/A' is out of date
The apache error log gives:
[Wed Jan 15 10:57:16 2014] [error] [client ::1] Attempting to modify out-of-date resource. [409, #170004]
[Wed Jan 15 10:57:16 2014] [error] [client ::1] Directory '/A' is out of date [409, #170004]
This comes from do_out_of_date_check in mod_dav_svn/repos.c.
There is a similar problem over ra_local:
Sending wc2
../src/subversion/svn/commit-cmd.c:182,
../src/subversion/libsvn_client/commit.c:1076,
../src/subversion/libsvn_client/commit.c:154: (apr_err=SVN_ERR_WC_NOT_UP_TO_DATE)
svn: E155011: Commit failed (details follow):
../src/subversion/libsvn_client/commit.c:991,
../src/subversion/libsvn_client/commit_util.c:1884,
../src/subversion/libsvn_delta/path_driver.c:174,
../src/subversion/libsvn_client/commit_util.c:1834,
../src/subversion/libsvn_client/commit_util.c:94: (apr_err=SVN_ERR_WC_NOT_UP_TO_DATE)
svn: E155011: Directory '/home/pm/sw/subversion/obj/wc2' is out of date
../src/subversion/libsvn_wc/adm_crawler.c:1222,
../src/subversion/libsvn_repos/commit.c:670,
../src/subversion/libsvn_repos/commit.c:165: (apr_err=SVN_ERR_FS_TXN_OUT_OF_DATE)
svn: E160028: Directory '/A' is out of date
and over ra_svn:
Sending wc2
Sending wc2/g
Transmitting file data .../src/subversion/svn/commit-cmd.c:182,
../src/subversion/libsvn_client/commit.c:1076,
../src/subversion/libsvn_client/commit.c:154: (apr_err=SVN_ERR_FS_TXN_OUT_OF_DATE)
svn: E160028: Commit failed (details follow):
../src/subversion/libsvn_client/commit.c:991,
../src/subversion/libsvn_client/commit_util.c:1945,
../src/subversion/libsvn_repos/commit.c:670,
../src/subversion/libsvn_repos/commit.c:165: (apr_err=SVN_ERR_FS_TXN_OUT_OF_DATE)
svn: E160028: Directory '/A' is out of date
Stefan Fuhrmann <st...@wandisco.com> writes:
> Hi Bert,
>
> We only accept incoming prop changes if there were
> no prop changes at the directory.
>
> -- Stefan^2.
>
>
> On Tue, Jan 14, 2014 at 11:39 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>> How do we now check that property changes don't conflict?
>>
>> Before this patch we assumed that the client code would do the
>> verification work. Are property changes now just overwritten?
>>
>> Or is your patch limited to one round of property changes?
>>
>> I think changes like this need more documentation than just mentioning
>> that it is a scalability issue that can be fixed 'easily'. There were quite
>> a few design decisions behind this limitation when I looked at it a few
>> years ago.
>>
>> This might break some of the promises that make the 'incomplete' handling
>> capable of resuming an update. Both the status of the properties and
>> entries of the directory are tied to that.
>>
>> Bert
>> ------------------------------
>> From: stefan2@apache.org
>> Sent: 14-1-2014 23:09
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1558224 - in
>> /subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c
>> tests/libsvn_fs/fs-test.c
>>
>> Author: stefan2
>> Date: Tue Jan 14 22:09:01 2014
>> New Revision: 1558224
>>
>> URL: http://svn.apache.org/r1558224
>> Log:
>> For FSFS and FSX, fix a major merge scalability issue described here:
>> http://svn.haxx.se/dev/archive-2014-01/0057.shtml
>>
>> The bottom line is that we allow incoming prop changes on directories
>> that have no addition nor deletion of entries relative to the TXN
>> base revision. This patch updates existing tests and provided a
>> specific new one.
>>
>> * subversion/libsvn_fs_fs/tree.c
>> (compare_dir_structure): A new utility function checking that all
>> dirents are still the same, although
>> possibly at different revisions.
>> (merge): An dir prop change in a txn should not conflict with
>> simply node upgrades within that directory.
>>
>> * subversion/libsvn_fs_x/tree.c
>> (compare_dir_structure,
>> merge): Similar implementation as for FSFS with a slight adaptation
>> due to different directory container types.
>>
>> * subversion/tests/libsvn_fs/fs-test.c
>> (unordered_txn_dirprops): Update expected results.
>> (dir_prop_merge): New test.
>> (test_compat_version): Register the new test.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_fs_fs/tree.c
>> subversion/trunk/subversion/libsvn_fs_x/tree.c
>> subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>>
>> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>>
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Tue Jan 14 22:09:01
>> 2014
>> @@ -1654,6 +1654,53 @@ conflict_err(svn_stringbuf_t *conflict_p
>> _("Conflict at '%s'"), path);
>> }
>>
>> +/* Compare the directory representations at nodes LHS and RHS and set
>> + * *CHANGED to TRUE, if at least one entry has been added or removed them.
>> + * Use POOL for temporary allocations.
>> + */
>> +static svn_error_t *
>> +compare_dir_structure(svn_boolean_t *changed,
>> + dag_node_t *lhs,
>> + dag_node_t *rhs,
>> + apr_pool_t *pool)
>> +{
>> + apr_array_header_t *lhs_entries;
>> + apr_array_header_t *rhs_entries;
>> + int i;
>> +
>> + SVN_ERR(svn_fs_fs__dag_dir_entries(&lhs_entries, lhs, pool));
>> + SVN_ERR(svn_fs_fs__dag_dir_entries(&rhs_entries, rhs, pool));
>> +
>> + /* different number of entries -> some addition / removal */
>> + if (lhs_entries->nelts != rhs_entries->nelts)
>> + {
>> + *changed = TRUE;
>> + return SVN_NO_ERROR;
>> + }
>> +
>> + /* Since directories are sorted by name, we can simply compare their
>> + entries one-by-one without binary lookup etc. */
>> + for (i = 0; i < lhs_entries->nelts; ++i)
>> + {
>> + svn_fs_dirent_t *lhs_entry
>> + = APR_ARRAY_IDX(lhs_entries, i, svn_fs_dirent_t *);
>> + svn_fs_dirent_t *rhs_entry
>> + = APR_ARRAY_IDX(rhs_entries, i, svn_fs_dirent_t *);
>> +
>> + if (strcmp(lhs_entry->name, rhs_entry->name)
>> + || !svn_fs_fs__id_part_eq(svn_fs_fs__id_node_id(lhs_entry->id),
>> + svn_fs_fs__id_node_id(rhs_entry->id))
>> + || !svn_fs_fs__id_part_eq(svn_fs_fs__id_copy_id(lhs_entry->id),
>> + svn_fs_fs__id_copy_id(rhs_entry->id)))
>> + {
>> + *changed = TRUE;
>> + return SVN_NO_ERROR;
>> + }
>> + }
>> +
>> + *changed = FALSE;
>> + return SVN_NO_ERROR;
>> +}
>>
>> /* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
>> * and TARGET must be distinct node revisions. TARGET_PATH should
>> @@ -1828,10 +1875,23 @@ merge(svn_stringbuf_t *conflict_p,
>> it doesn't do a brute-force comparison on textual contents, so
>> it won't do that here either. Checking to see if the propkey
>> atoms are `equal' is enough. */
>> - if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep,
>> anc_nr->prop_rep))
>> - return conflict_err(conflict_p, target_path);
>> if (! svn_fs_fs__noderev_same_rep_key(src_nr->prop_rep,
>> anc_nr->prop_rep))
>> return conflict_err(conflict_p, target_path);
>> +
>> + /* The directory entries got changed in the repository but the
>> directory
>> + properties did not. */
>> + if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep,
>> anc_nr->prop_rep))
>> + {
>> + /* There is an incoming prop change for this directory.
>> + We will accept it only if the directory changes were mere
>> updates
>> + to its entries, i.e. there were no additions or removals.
>> + Those could cause update problems to the working copy. */
>> + svn_boolean_t changed;
>> + SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
>> +
>> + if (changed)
>> + return conflict_err(conflict_p, target_path);
>> + }
>> }
>>
>> /* ### todo: it would be more efficient to simply check for a NULL
>>
>> Modified: subversion/trunk/subversion/libsvn_fs_x/tree.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>>
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_fs_x/tree.c (original)
>> +++ subversion/trunk/subversion/libsvn_fs_x/tree.c Tue Jan 14 22:09:01 2014
>> @@ -1630,6 +1630,56 @@ conflict_err(svn_stringbuf_t *conflict_p
>> _("Conflict at '%s'"), path);
>> }
>>
>> +/* Compare the directory representations at nodes LHS and RHS and set
>> + * *CHANGED to TRUE, if at least one entry has been added or removed them.
>> + * Use POOL for temporary allocations.
>> + */
>> +static svn_error_t *
>> +compare_dir_structure(svn_boolean_t *changed,
>> + dag_node_t *lhs,
>> + dag_node_t *rhs,
>> + apr_pool_t *pool)
>> +{
>> + apr_hash_t *lhs_entries;
>> + apr_hash_t *rhs_entries;
>> + apr_hash_index_t *hi;
>> +
>> + SVN_ERR(svn_fs_x__dag_dir_entries(&lhs_entries, lhs, pool));
>> + SVN_ERR(svn_fs_x__dag_dir_entries(&rhs_entries, rhs, pool));
>> +
>> + /* different number of entries -> some addition / removal */
>> + if (apr_hash_count(lhs_entries) != apr_hash_count(rhs_entries))
>> + {
>> + *changed = TRUE;
>> + return SVN_NO_ERROR;
>> + }
>> +
>> + /* Since the number of dirents is the same, we simply need to do a
>> + one-sided comparison. */
>> + for (hi = apr_hash_first(pool, lhs_entries); hi; hi = apr_hash_next(hi))
>> + {
>> + svn_fs_dirent_t *lhs_entry;
>> + svn_fs_dirent_t *rhs_entry;
>> + const char *name;
>> + apr_ssize_t klen;
>> +
>> + apr_hash_this(hi, (const void **)&name, &klen, (void **)&lhs_entry);
>> + rhs_entry = apr_hash_get(rhs_entries, name, klen);
>> +
>> + if (!rhs_entry
>> + || !svn_fs_x__id_part_eq(svn_fs_x__id_node_id(lhs_entry->id),
>> + svn_fs_x__id_node_id(rhs_entry->id))
>> + || !svn_fs_x__id_part_eq(svn_fs_x__id_copy_id(lhs_entry->id),
>> + svn_fs_x__id_copy_id(rhs_entry->id)))
>> + {
>> + *changed = TRUE;
>> + return SVN_NO_ERROR;
>> + }
>> + }
>> +
>> + *changed = FALSE;
>> + return SVN_NO_ERROR;
>> +}
>>
>> /* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
>> * and TARGET must be distinct node revisions. TARGET_PATH should
>> @@ -1803,10 +1853,23 @@ merge(svn_stringbuf_t *conflict_p,
>> it doesn't do a brute-force comparison on textual contents, so
>> it won't do that here either. Checking to see if the propkey
>> atoms are `equal' is enough. */
>> - if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep,
>> anc_nr->prop_rep))
>> - return conflict_err(conflict_p, target_path);
>> if (! svn_fs_x__noderev_same_rep_key(src_nr->prop_rep,
>> anc_nr->prop_rep))
>> return conflict_err(conflict_p, target_path);
>> +
>> + /* The directory entries got changed in the repository but the
>> directory
>> + properties did not. */
>> + if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep,
>> anc_nr->prop_rep))
>> + {
>> + /* There is an incoming prop change for this directory.
>> + We will accept it only if the directory changes were mere
>> updates
>> + to its entries, i.e. there were no additions or removals.
>> + Those could cause update problems to the working copy. */
>> + svn_boolean_t changed;
>> + SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
>> +
>> + if (changed)
>> + return conflict_err(conflict_p, target_path);
>> + }
>> }
>>
>> /* ### todo: it would be more efficient to simply check for a NULL
>>
>> Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>> URL:
>> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>>
>> ==============================================================================
>> --- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
>> +++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue Jan 14
>> 22:09:01 2014
>> @@ -4582,6 +4582,7 @@ unordered_txn_dirprops(const svn_test_op
>> svn_fs_root_t *txn_root, *txn_root2;
>> svn_string_t pval;
>> svn_revnum_t new_rev, not_rev;
>> + svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
>>
>> /* This is a regression test for issue #2751. */
>>
>> @@ -4638,10 +4639,21 @@ unordered_txn_dirprops(const svn_test_op
>> /* Commit the first one first. */
>> SVN_ERR(test_commit_txn(&new_rev, txn, NULL, pool));
>>
>> - /* Then commit the second -- but expect an conflict because the
>> - directory wasn't up-to-date, which is required for propchanges. */
>> - SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
>> - SVN_ERR(svn_fs_abort_txn(txn2, pool));
>> + /* Some backends are clever then others. */
>> + if (is_bdb)
>> + {
>> + /* Then commit the second -- but expect an conflict because the
>> + directory wasn't up-to-date, which is required for propchanges.
>> */
>> + SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
>> + SVN_ERR(svn_fs_abort_txn(txn2, pool));
>> + }
>> + else
>> + {
>> + /* Then commit the second -- there will be no conflict despite the
>> + directory being out-of-data because the properties as well as the
>> + directory structure (list of nodes) was up-to-date. */
>> + SVN_ERR(test_commit_txn(¬_rev, txn2, NULL, pool));
>> + }
>>
>> return SVN_NO_ERROR;
>> }
>> @@ -5222,6 +5234,80 @@ test_compat_version(const svn_test_opts_
>> return SVN_NO_ERROR;
>> }
>>
>> +static svn_error_t *
>> +dir_prop_merge(const svn_test_opts_t *opts,
>> + apr_pool_t *pool)
>> +{
>> + svn_fs_t *fs;
>> + svn_revnum_t head_rev;
>> + svn_fs_root_t *root;
>> + svn_fs_txn_t *txn, *mid_txn, *top_txn, *sub_txn, *c_txn;
>> + svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
>> +
>> + /* Create test repository. */
>> + SVN_ERR(svn_test__create_fs(&fs, "test-dir_prop-merge", opts, pool));
>> +
>> + SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, pool));
>> + SVN_ERR(svn_fs_txn_root(&root, txn, pool));
>> +
>> + /* Create and verify the greek tree. */
>> + SVN_ERR(svn_test__create_greek_tree(root, pool));
>> + SVN_ERR(test_commit_txn(&head_rev, txn, NULL, pool));
>> +
>> + /* Start concurrent transactions */
>> +
>> + /* 1st: modify a mid-level directory */
>> + SVN_ERR(svn_fs_begin_txn2(&mid_txn, fs, head_rev, 0, pool));
>> + SVN_ERR(svn_fs_txn_root(&root, mid_txn, pool));
>> + SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
>> + svn_string_create("val1", pool), pool));
>> + svn_fs_close_root(root);
>> +
>> + /* 2st: modify a top-level directory */
>> + SVN_ERR(svn_fs_begin_txn2(&top_txn, fs, head_rev, 0, pool));
>> + SVN_ERR(svn_fs_txn_root(&root, top_txn, pool));
>> + SVN_ERR(svn_fs_change_node_prop(root, "A", "test-prop",
>> + svn_string_create("val2", pool), pool));
>> + svn_fs_close_root(root);
>> +
>> + SVN_ERR(svn_fs_begin_txn2(&sub_txn, fs, head_rev, 0, pool));
>> + SVN_ERR(svn_fs_txn_root(&root, sub_txn, pool));
>> + SVN_ERR(svn_fs_change_node_prop(root, "A/D/G", "test-prop",
>> + svn_string_create("val3", pool), pool));
>> + svn_fs_close_root(root);
>> +
>> + /* 3rd: modify a conflicting change to the mid-level directory */
>> + SVN_ERR(svn_fs_begin_txn2(&c_txn, fs, head_rev, 0, pool));
>> + SVN_ERR(svn_fs_txn_root(&root, c_txn, pool));
>> + SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
>> + svn_string_create("valX", pool), pool));
>> + svn_fs_close_root(root);
>> +
>> + /* Prop changes to the same node should conflict */
>> + SVN_ERR(test_commit_txn(&head_rev, mid_txn, NULL, pool));
>> + SVN_ERR(test_commit_txn(&head_rev, c_txn, "/A/D", pool));
>> + SVN_ERR(svn_fs_abort_txn(c_txn, pool));
>> +
>> + /* Changes in a sub-tree should not conflict with prop changes to some
>> + parent directory but some backends are clever then others. */
>> + if (is_bdb)
>> + {
>> + SVN_ERR(test_commit_txn(&head_rev, top_txn, "/A", pool));
>> + SVN_ERR(svn_fs_abort_txn(top_txn, pool));
>> + }
>> + else
>> + {
>> + SVN_ERR(test_commit_txn(&head_rev, top_txn, NULL, pool));
>> + }
>> +
>> + /* The inverted case is not that trivial to handle. Hence, conflict.
>> + Depending on the checking order, the reported conflict path differs.
>> */
>> + SVN_ERR(test_commit_txn(&head_rev, sub_txn, is_bdb ? "/A/D" : "/A",
>> pool));
>> + SVN_ERR(svn_fs_abort_txn(sub_txn, pool));
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>>
>> /*
>> ------------------------------------------------------------------------ */
>>
>> @@ -5315,5 +5401,7 @@ struct svn_test_descriptor_t test_funcs[
>> "commit timestamp"),
>> SVN_TEST_OPTS_PASS(test_compat_version,
>> "test svn_fs__compatible_version"),
>> + SVN_TEST_OPTS_PASS(dir_prop_merge,
>> + "test merge directory properties"),
>> SVN_TEST_NULL
>> };
>>
>>
>>
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: svn commit: r1558224 - in /subversion/trunk/subversion:libsvn_fs_fs/tree.c
libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
Hi Bert,
We only accept incoming prop changes if there were
no prop changes at the directory.
-- Stefan^2.
On Tue, Jan 14, 2014 at 11:39 PM, Bert Huijben <be...@qqmail.nl> wrote:
> How do we now check that property changes don't conflict?
>
> Before this patch we assumed that the client code would do the
> verification work. Are property changes now just overwritten?
>
> Or is your patch limited to one round of property changes?
>
> I think changes like this need more documentation than just mentioning
> that it is a scalability issue that can be fixed 'easily'. There were quite
> a few design decisions behind this limitation when I looked at it a few
> years ago.
>
> This might break some of the promises that make the 'incomplete' handling
> capable of resuming an update. Both the status of the properties and
> entries of the directory are tied to that.
>
> Bert
> ------------------------------
> From: stefan2@apache.org
> Sent: 14-1-2014 23:09
> To: commits@subversion.apache.org
> Subject: svn commit: r1558224 - in
> /subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c
> tests/libsvn_fs/fs-test.c
>
> Author: stefan2
> Date: Tue Jan 14 22:09:01 2014
> New Revision: 1558224
>
> URL: http://svn.apache.org/r1558224
> Log:
> For FSFS and FSX, fix a major merge scalability issue described here:
> http://svn.haxx.se/dev/archive-2014-01/0057.shtml
>
> The bottom line is that we allow incoming prop changes on directories
> that have no addition nor deletion of entries relative to the TXN
> base revision. This patch updates existing tests and provided a
> specific new one.
>
> * subversion/libsvn_fs_fs/tree.c
> (compare_dir_structure): A new utility function checking that all
> dirents are still the same, although
> possibly at different revisions.
> (merge): An dir prop change in a txn should not conflict with
> simply node upgrades within that directory.
>
> * subversion/libsvn_fs_x/tree.c
> (compare_dir_structure,
> merge): Similar implementation as for FSFS with a slight adaptation
> due to different directory container types.
>
> * subversion/tests/libsvn_fs/fs-test.c
> (unordered_txn_dirprops): Update expected results.
> (dir_prop_merge): New test.
> (test_compat_version): Register the new test.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
> subversion/trunk/subversion/libsvn_fs_x/tree.c
> subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Tue Jan 14 22:09:01
> 2014
> @@ -1654,6 +1654,53 @@ conflict_err(svn_stringbuf_t *conflict_p
> _("Conflict at '%s'"), path);
> }
>
> +/* Compare the directory representations at nodes LHS and RHS and set
> + * *CHANGED to TRUE, if at least one entry has been added or removed them.
> + * Use POOL for temporary allocations.
> + */
> +static svn_error_t *
> +compare_dir_structure(svn_boolean_t *changed,
> + dag_node_t *lhs,
> + dag_node_t *rhs,
> + apr_pool_t *pool)
> +{
> + apr_array_header_t *lhs_entries;
> + apr_array_header_t *rhs_entries;
> + int i;
> +
> + SVN_ERR(svn_fs_fs__dag_dir_entries(&lhs_entries, lhs, pool));
> + SVN_ERR(svn_fs_fs__dag_dir_entries(&rhs_entries, rhs, pool));
> +
> + /* different number of entries -> some addition / removal */
> + if (lhs_entries->nelts != rhs_entries->nelts)
> + {
> + *changed = TRUE;
> + return SVN_NO_ERROR;
> + }
> +
> + /* Since directories are sorted by name, we can simply compare their
> + entries one-by-one without binary lookup etc. */
> + for (i = 0; i < lhs_entries->nelts; ++i)
> + {
> + svn_fs_dirent_t *lhs_entry
> + = APR_ARRAY_IDX(lhs_entries, i, svn_fs_dirent_t *);
> + svn_fs_dirent_t *rhs_entry
> + = APR_ARRAY_IDX(rhs_entries, i, svn_fs_dirent_t *);
> +
> + if (strcmp(lhs_entry->name, rhs_entry->name)
> + || !svn_fs_fs__id_part_eq(svn_fs_fs__id_node_id(lhs_entry->id),
> + svn_fs_fs__id_node_id(rhs_entry->id))
> + || !svn_fs_fs__id_part_eq(svn_fs_fs__id_copy_id(lhs_entry->id),
> + svn_fs_fs__id_copy_id(rhs_entry->id)))
> + {
> + *changed = TRUE;
> + return SVN_NO_ERROR;
> + }
> + }
> +
> + *changed = FALSE;
> + return SVN_NO_ERROR;
> +}
>
> /* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
> * and TARGET must be distinct node revisions. TARGET_PATH should
> @@ -1828,10 +1875,23 @@ merge(svn_stringbuf_t *conflict_p,
> it doesn't do a brute-force comparison on textual contents, so
> it won't do that here either. Checking to see if the propkey
> atoms are `equal' is enough. */
> - if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep,
> anc_nr->prop_rep))
> - return conflict_err(conflict_p, target_path);
> if (! svn_fs_fs__noderev_same_rep_key(src_nr->prop_rep,
> anc_nr->prop_rep))
> return conflict_err(conflict_p, target_path);
> +
> + /* The directory entries got changed in the repository but the
> directory
> + properties did not. */
> + if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep,
> anc_nr->prop_rep))
> + {
> + /* There is an incoming prop change for this directory.
> + We will accept it only if the directory changes were mere
> updates
> + to its entries, i.e. there were no additions or removals.
> + Those could cause update problems to the working copy. */
> + svn_boolean_t changed;
> + SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
> +
> + if (changed)
> + return conflict_err(conflict_p, target_path);
> + }
> }
>
> /* ### todo: it would be more efficient to simply check for a NULL
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/tree.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_x/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_x/tree.c Tue Jan 14 22:09:01 2014
> @@ -1630,6 +1630,56 @@ conflict_err(svn_stringbuf_t *conflict_p
> _("Conflict at '%s'"), path);
> }
>
> +/* Compare the directory representations at nodes LHS and RHS and set
> + * *CHANGED to TRUE, if at least one entry has been added or removed them.
> + * Use POOL for temporary allocations.
> + */
> +static svn_error_t *
> +compare_dir_structure(svn_boolean_t *changed,
> + dag_node_t *lhs,
> + dag_node_t *rhs,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *lhs_entries;
> + apr_hash_t *rhs_entries;
> + apr_hash_index_t *hi;
> +
> + SVN_ERR(svn_fs_x__dag_dir_entries(&lhs_entries, lhs, pool));
> + SVN_ERR(svn_fs_x__dag_dir_entries(&rhs_entries, rhs, pool));
> +
> + /* different number of entries -> some addition / removal */
> + if (apr_hash_count(lhs_entries) != apr_hash_count(rhs_entries))
> + {
> + *changed = TRUE;
> + return SVN_NO_ERROR;
> + }
> +
> + /* Since the number of dirents is the same, we simply need to do a
> + one-sided comparison. */
> + for (hi = apr_hash_first(pool, lhs_entries); hi; hi = apr_hash_next(hi))
> + {
> + svn_fs_dirent_t *lhs_entry;
> + svn_fs_dirent_t *rhs_entry;
> + const char *name;
> + apr_ssize_t klen;
> +
> + apr_hash_this(hi, (const void **)&name, &klen, (void **)&lhs_entry);
> + rhs_entry = apr_hash_get(rhs_entries, name, klen);
> +
> + if (!rhs_entry
> + || !svn_fs_x__id_part_eq(svn_fs_x__id_node_id(lhs_entry->id),
> + svn_fs_x__id_node_id(rhs_entry->id))
> + || !svn_fs_x__id_part_eq(svn_fs_x__id_copy_id(lhs_entry->id),
> + svn_fs_x__id_copy_id(rhs_entry->id)))
> + {
> + *changed = TRUE;
> + return SVN_NO_ERROR;
> + }
> + }
> +
> + *changed = FALSE;
> + return SVN_NO_ERROR;
> +}
>
> /* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
> * and TARGET must be distinct node revisions. TARGET_PATH should
> @@ -1803,10 +1853,23 @@ merge(svn_stringbuf_t *conflict_p,
> it doesn't do a brute-force comparison on textual contents, so
> it won't do that here either. Checking to see if the propkey
> atoms are `equal' is enough. */
> - if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep,
> anc_nr->prop_rep))
> - return conflict_err(conflict_p, target_path);
> if (! svn_fs_x__noderev_same_rep_key(src_nr->prop_rep,
> anc_nr->prop_rep))
> return conflict_err(conflict_p, target_path);
> +
> + /* The directory entries got changed in the repository but the
> directory
> + properties did not. */
> + if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep,
> anc_nr->prop_rep))
> + {
> + /* There is an incoming prop change for this directory.
> + We will accept it only if the directory changes were mere
> updates
> + to its entries, i.e. there were no additions or removals.
> + Those could cause update problems to the working copy. */
> + svn_boolean_t changed;
> + SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
> +
> + if (changed)
> + return conflict_err(conflict_p, target_path);
> + }
> }
>
> /* ### todo: it would be more efficient to simply check for a NULL
>
> Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue Jan 14
> 22:09:01 2014
> @@ -4582,6 +4582,7 @@ unordered_txn_dirprops(const svn_test_op
> svn_fs_root_t *txn_root, *txn_root2;
> svn_string_t pval;
> svn_revnum_t new_rev, not_rev;
> + svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
>
> /* This is a regression test for issue #2751. */
>
> @@ -4638,10 +4639,21 @@ unordered_txn_dirprops(const svn_test_op
> /* Commit the first one first. */
> SVN_ERR(test_commit_txn(&new_rev, txn, NULL, pool));
>
> - /* Then commit the second -- but expect an conflict because the
> - directory wasn't up-to-date, which is required for propchanges. */
> - SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
> - SVN_ERR(svn_fs_abort_txn(txn2, pool));
> + /* Some backends are clever then others. */
> + if (is_bdb)
> + {
> + /* Then commit the second -- but expect an conflict because the
> + directory wasn't up-to-date, which is required for propchanges.
> */
> + SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
> + SVN_ERR(svn_fs_abort_txn(txn2, pool));
> + }
> + else
> + {
> + /* Then commit the second -- there will be no conflict despite the
> + directory being out-of-data because the properties as well as the
> + directory structure (list of nodes) was up-to-date. */
> + SVN_ERR(test_commit_txn(¬_rev, txn2, NULL, pool));
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -5222,6 +5234,80 @@ test_compat_version(const svn_test_opts_
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +dir_prop_merge(const svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + svn_fs_t *fs;
> + svn_revnum_t head_rev;
> + svn_fs_root_t *root;
> + svn_fs_txn_t *txn, *mid_txn, *top_txn, *sub_txn, *c_txn;
> + svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
> +
> + /* Create test repository. */
> + SVN_ERR(svn_test__create_fs(&fs, "test-dir_prop-merge", opts, pool));
> +
> + SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, txn, pool));
> +
> + /* Create and verify the greek tree. */
> + SVN_ERR(svn_test__create_greek_tree(root, pool));
> + SVN_ERR(test_commit_txn(&head_rev, txn, NULL, pool));
> +
> + /* Start concurrent transactions */
> +
> + /* 1st: modify a mid-level directory */
> + SVN_ERR(svn_fs_begin_txn2(&mid_txn, fs, head_rev, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, mid_txn, pool));
> + SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
> + svn_string_create("val1", pool), pool));
> + svn_fs_close_root(root);
> +
> + /* 2st: modify a top-level directory */
> + SVN_ERR(svn_fs_begin_txn2(&top_txn, fs, head_rev, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, top_txn, pool));
> + SVN_ERR(svn_fs_change_node_prop(root, "A", "test-prop",
> + svn_string_create("val2", pool), pool));
> + svn_fs_close_root(root);
> +
> + SVN_ERR(svn_fs_begin_txn2(&sub_txn, fs, head_rev, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, sub_txn, pool));
> + SVN_ERR(svn_fs_change_node_prop(root, "A/D/G", "test-prop",
> + svn_string_create("val3", pool), pool));
> + svn_fs_close_root(root);
> +
> + /* 3rd: modify a conflicting change to the mid-level directory */
> + SVN_ERR(svn_fs_begin_txn2(&c_txn, fs, head_rev, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, c_txn, pool));
> + SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
> + svn_string_create("valX", pool), pool));
> + svn_fs_close_root(root);
> +
> + /* Prop changes to the same node should conflict */
> + SVN_ERR(test_commit_txn(&head_rev, mid_txn, NULL, pool));
> + SVN_ERR(test_commit_txn(&head_rev, c_txn, "/A/D", pool));
> + SVN_ERR(svn_fs_abort_txn(c_txn, pool));
> +
> + /* Changes in a sub-tree should not conflict with prop changes to some
> + parent directory but some backends are clever then others. */
> + if (is_bdb)
> + {
> + SVN_ERR(test_commit_txn(&head_rev, top_txn, "/A", pool));
> + SVN_ERR(svn_fs_abort_txn(top_txn, pool));
> + }
> + else
> + {
> + SVN_ERR(test_commit_txn(&head_rev, top_txn, NULL, pool));
> + }
> +
> + /* The inverted case is not that trivial to handle. Hence, conflict.
> + Depending on the checking order, the reported conflict path differs.
> */
> + SVN_ERR(test_commit_txn(&head_rev, sub_txn, is_bdb ? "/A/D" : "/A",
> pool));
> + SVN_ERR(svn_fs_abort_txn(sub_txn, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
>
> /*
> ------------------------------------------------------------------------ */
>
> @@ -5315,5 +5401,7 @@ struct svn_test_descriptor_t test_funcs[
> "commit timestamp"),
> SVN_TEST_OPTS_PASS(test_compat_version,
> "test svn_fs__compatible_version"),
> + SVN_TEST_OPTS_PASS(dir_prop_merge,
> + "test merge directory properties"),
> SVN_TEST_NULL
> };
>
>
>
Re: svn commit: r1558224 - in /subversion/trunk/subversion:libsvn_fs_fs/tree.c
libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
Hi Bert,
We only accept incoming prop changes if there were
no prop changes at the directory.
-- Stefan^2.
On Tue, Jan 14, 2014 at 11:39 PM, Bert Huijben <be...@qqmail.nl> wrote:
> How do we now check that property changes don't conflict?
>
> Before this patch we assumed that the client code would do the
> verification work. Are property changes now just overwritten?
>
> Or is your patch limited to one round of property changes?
>
> I think changes like this need more documentation than just mentioning
> that it is a scalability issue that can be fixed 'easily'. There were quite
> a few design decisions behind this limitation when I looked at it a few
> years ago.
>
> This might break some of the promises that make the 'incomplete' handling
> capable of resuming an update. Both the status of the properties and
> entries of the directory are tied to that.
>
> Bert
> ------------------------------
> From: stefan2@apache.org
> Sent: 14-1-2014 23:09
> To: commits@subversion.apache.org
> Subject: svn commit: r1558224 - in
> /subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c
> tests/libsvn_fs/fs-test.c
>
> Author: stefan2
> Date: Tue Jan 14 22:09:01 2014
> New Revision: 1558224
>
> URL: http://svn.apache.org/r1558224
> Log:
> For FSFS and FSX, fix a major merge scalability issue described here:
> http://svn.haxx.se/dev/archive-2014-01/0057.shtml
>
> The bottom line is that we allow incoming prop changes on directories
> that have no addition nor deletion of entries relative to the TXN
> base revision. This patch updates existing tests and provided a
> specific new one.
>
> * subversion/libsvn_fs_fs/tree.c
> (compare_dir_structure): A new utility function checking that all
> dirents are still the same, although
> possibly at different revisions.
> (merge): An dir prop change in a txn should not conflict with
> simply node upgrades within that directory.
>
> * subversion/libsvn_fs_x/tree.c
> (compare_dir_structure,
> merge): Similar implementation as for FSFS with a slight adaptation
> due to different directory container types.
>
> * subversion/tests/libsvn_fs/fs-test.c
> (unordered_txn_dirprops): Update expected results.
> (dir_prop_merge): New test.
> (test_compat_version): Register the new test.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/tree.c
> subversion/trunk/subversion/libsvn_fs_x/tree.c
> subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/tree.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/tree.c Tue Jan 14 22:09:01
> 2014
> @@ -1654,6 +1654,53 @@ conflict_err(svn_stringbuf_t *conflict_p
> _("Conflict at '%s'"), path);
> }
>
> +/* Compare the directory representations at nodes LHS and RHS and set
> + * *CHANGED to TRUE, if at least one entry has been added or removed them.
> + * Use POOL for temporary allocations.
> + */
> +static svn_error_t *
> +compare_dir_structure(svn_boolean_t *changed,
> + dag_node_t *lhs,
> + dag_node_t *rhs,
> + apr_pool_t *pool)
> +{
> + apr_array_header_t *lhs_entries;
> + apr_array_header_t *rhs_entries;
> + int i;
> +
> + SVN_ERR(svn_fs_fs__dag_dir_entries(&lhs_entries, lhs, pool));
> + SVN_ERR(svn_fs_fs__dag_dir_entries(&rhs_entries, rhs, pool));
> +
> + /* different number of entries -> some addition / removal */
> + if (lhs_entries->nelts != rhs_entries->nelts)
> + {
> + *changed = TRUE;
> + return SVN_NO_ERROR;
> + }
> +
> + /* Since directories are sorted by name, we can simply compare their
> + entries one-by-one without binary lookup etc. */
> + for (i = 0; i < lhs_entries->nelts; ++i)
> + {
> + svn_fs_dirent_t *lhs_entry
> + = APR_ARRAY_IDX(lhs_entries, i, svn_fs_dirent_t *);
> + svn_fs_dirent_t *rhs_entry
> + = APR_ARRAY_IDX(rhs_entries, i, svn_fs_dirent_t *);
> +
> + if (strcmp(lhs_entry->name, rhs_entry->name)
> + || !svn_fs_fs__id_part_eq(svn_fs_fs__id_node_id(lhs_entry->id),
> + svn_fs_fs__id_node_id(rhs_entry->id))
> + || !svn_fs_fs__id_part_eq(svn_fs_fs__id_copy_id(lhs_entry->id),
> + svn_fs_fs__id_copy_id(rhs_entry->id)))
> + {
> + *changed = TRUE;
> + return SVN_NO_ERROR;
> + }
> + }
> +
> + *changed = FALSE;
> + return SVN_NO_ERROR;
> +}
>
> /* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
> * and TARGET must be distinct node revisions. TARGET_PATH should
> @@ -1828,10 +1875,23 @@ merge(svn_stringbuf_t *conflict_p,
> it doesn't do a brute-force comparison on textual contents, so
> it won't do that here either. Checking to see if the propkey
> atoms are `equal' is enough. */
> - if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep,
> anc_nr->prop_rep))
> - return conflict_err(conflict_p, target_path);
> if (! svn_fs_fs__noderev_same_rep_key(src_nr->prop_rep,
> anc_nr->prop_rep))
> return conflict_err(conflict_p, target_path);
> +
> + /* The directory entries got changed in the repository but the
> directory
> + properties did not. */
> + if (! svn_fs_fs__noderev_same_rep_key(tgt_nr->prop_rep,
> anc_nr->prop_rep))
> + {
> + /* There is an incoming prop change for this directory.
> + We will accept it only if the directory changes were mere
> updates
> + to its entries, i.e. there were no additions or removals.
> + Those could cause update problems to the working copy. */
> + svn_boolean_t changed;
> + SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
> +
> + if (changed)
> + return conflict_err(conflict_p, target_path);
> + }
> }
>
> /* ### todo: it would be more efficient to simply check for a NULL
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/tree.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/tree.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_x/tree.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_x/tree.c Tue Jan 14 22:09:01 2014
> @@ -1630,6 +1630,56 @@ conflict_err(svn_stringbuf_t *conflict_p
> _("Conflict at '%s'"), path);
> }
>
> +/* Compare the directory representations at nodes LHS and RHS and set
> + * *CHANGED to TRUE, if at least one entry has been added or removed them.
> + * Use POOL for temporary allocations.
> + */
> +static svn_error_t *
> +compare_dir_structure(svn_boolean_t *changed,
> + dag_node_t *lhs,
> + dag_node_t *rhs,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *lhs_entries;
> + apr_hash_t *rhs_entries;
> + apr_hash_index_t *hi;
> +
> + SVN_ERR(svn_fs_x__dag_dir_entries(&lhs_entries, lhs, pool));
> + SVN_ERR(svn_fs_x__dag_dir_entries(&rhs_entries, rhs, pool));
> +
> + /* different number of entries -> some addition / removal */
> + if (apr_hash_count(lhs_entries) != apr_hash_count(rhs_entries))
> + {
> + *changed = TRUE;
> + return SVN_NO_ERROR;
> + }
> +
> + /* Since the number of dirents is the same, we simply need to do a
> + one-sided comparison. */
> + for (hi = apr_hash_first(pool, lhs_entries); hi; hi = apr_hash_next(hi))
> + {
> + svn_fs_dirent_t *lhs_entry;
> + svn_fs_dirent_t *rhs_entry;
> + const char *name;
> + apr_ssize_t klen;
> +
> + apr_hash_this(hi, (const void **)&name, &klen, (void **)&lhs_entry);
> + rhs_entry = apr_hash_get(rhs_entries, name, klen);
> +
> + if (!rhs_entry
> + || !svn_fs_x__id_part_eq(svn_fs_x__id_node_id(lhs_entry->id),
> + svn_fs_x__id_node_id(rhs_entry->id))
> + || !svn_fs_x__id_part_eq(svn_fs_x__id_copy_id(lhs_entry->id),
> + svn_fs_x__id_copy_id(rhs_entry->id)))
> + {
> + *changed = TRUE;
> + return SVN_NO_ERROR;
> + }
> + }
> +
> + *changed = FALSE;
> + return SVN_NO_ERROR;
> +}
>
> /* Merge changes between ANCESTOR and SOURCE into TARGET. ANCESTOR
> * and TARGET must be distinct node revisions. TARGET_PATH should
> @@ -1803,10 +1853,23 @@ merge(svn_stringbuf_t *conflict_p,
> it doesn't do a brute-force comparison on textual contents, so
> it won't do that here either. Checking to see if the propkey
> atoms are `equal' is enough. */
> - if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep,
> anc_nr->prop_rep))
> - return conflict_err(conflict_p, target_path);
> if (! svn_fs_x__noderev_same_rep_key(src_nr->prop_rep,
> anc_nr->prop_rep))
> return conflict_err(conflict_p, target_path);
> +
> + /* The directory entries got changed in the repository but the
> directory
> + properties did not. */
> + if (! svn_fs_x__noderev_same_rep_key(tgt_nr->prop_rep,
> anc_nr->prop_rep))
> + {
> + /* There is an incoming prop change for this directory.
> + We will accept it only if the directory changes were mere
> updates
> + to its entries, i.e. there were no additions or removals.
> + Those could cause update problems to the working copy. */
> + svn_boolean_t changed;
> + SVN_ERR(compare_dir_structure(&changed, source, ancestor, pool));
> +
> + if (changed)
> + return conflict_err(conflict_p, target_path);
> + }
> }
>
> /* ### todo: it would be more efficient to simply check for a NULL
>
> Modified: subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_fs/fs-test.c?rev=1558224&r1=1558223&r2=1558224&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_fs/fs-test.c (original)
> +++ subversion/trunk/subversion/tests/libsvn_fs/fs-test.c Tue Jan 14
> 22:09:01 2014
> @@ -4582,6 +4582,7 @@ unordered_txn_dirprops(const svn_test_op
> svn_fs_root_t *txn_root, *txn_root2;
> svn_string_t pval;
> svn_revnum_t new_rev, not_rev;
> + svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
>
> /* This is a regression test for issue #2751. */
>
> @@ -4638,10 +4639,21 @@ unordered_txn_dirprops(const svn_test_op
> /* Commit the first one first. */
> SVN_ERR(test_commit_txn(&new_rev, txn, NULL, pool));
>
> - /* Then commit the second -- but expect an conflict because the
> - directory wasn't up-to-date, which is required for propchanges. */
> - SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
> - SVN_ERR(svn_fs_abort_txn(txn2, pool));
> + /* Some backends are clever then others. */
> + if (is_bdb)
> + {
> + /* Then commit the second -- but expect an conflict because the
> + directory wasn't up-to-date, which is required for propchanges.
> */
> + SVN_ERR(test_commit_txn(¬_rev, txn2, "/A/B", pool));
> + SVN_ERR(svn_fs_abort_txn(txn2, pool));
> + }
> + else
> + {
> + /* Then commit the second -- there will be no conflict despite the
> + directory being out-of-data because the properties as well as the
> + directory structure (list of nodes) was up-to-date. */
> + SVN_ERR(test_commit_txn(¬_rev, txn2, NULL, pool));
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -5222,6 +5234,80 @@ test_compat_version(const svn_test_opts_
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *
> +dir_prop_merge(const svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + svn_fs_t *fs;
> + svn_revnum_t head_rev;
> + svn_fs_root_t *root;
> + svn_fs_txn_t *txn, *mid_txn, *top_txn, *sub_txn, *c_txn;
> + svn_boolean_t is_bdb = strcmp(opts->fs_type, "bdb") == 0;
> +
> + /* Create test repository. */
> + SVN_ERR(svn_test__create_fs(&fs, "test-dir_prop-merge", opts, pool));
> +
> + SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, txn, pool));
> +
> + /* Create and verify the greek tree. */
> + SVN_ERR(svn_test__create_greek_tree(root, pool));
> + SVN_ERR(test_commit_txn(&head_rev, txn, NULL, pool));
> +
> + /* Start concurrent transactions */
> +
> + /* 1st: modify a mid-level directory */
> + SVN_ERR(svn_fs_begin_txn2(&mid_txn, fs, head_rev, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, mid_txn, pool));
> + SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
> + svn_string_create("val1", pool), pool));
> + svn_fs_close_root(root);
> +
> + /* 2st: modify a top-level directory */
> + SVN_ERR(svn_fs_begin_txn2(&top_txn, fs, head_rev, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, top_txn, pool));
> + SVN_ERR(svn_fs_change_node_prop(root, "A", "test-prop",
> + svn_string_create("val2", pool), pool));
> + svn_fs_close_root(root);
> +
> + SVN_ERR(svn_fs_begin_txn2(&sub_txn, fs, head_rev, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, sub_txn, pool));
> + SVN_ERR(svn_fs_change_node_prop(root, "A/D/G", "test-prop",
> + svn_string_create("val3", pool), pool));
> + svn_fs_close_root(root);
> +
> + /* 3rd: modify a conflicting change to the mid-level directory */
> + SVN_ERR(svn_fs_begin_txn2(&c_txn, fs, head_rev, 0, pool));
> + SVN_ERR(svn_fs_txn_root(&root, c_txn, pool));
> + SVN_ERR(svn_fs_change_node_prop(root, "A/D", "test-prop",
> + svn_string_create("valX", pool), pool));
> + svn_fs_close_root(root);
> +
> + /* Prop changes to the same node should conflict */
> + SVN_ERR(test_commit_txn(&head_rev, mid_txn, NULL, pool));
> + SVN_ERR(test_commit_txn(&head_rev, c_txn, "/A/D", pool));
> + SVN_ERR(svn_fs_abort_txn(c_txn, pool));
> +
> + /* Changes in a sub-tree should not conflict with prop changes to some
> + parent directory but some backends are clever then others. */
> + if (is_bdb)
> + {
> + SVN_ERR(test_commit_txn(&head_rev, top_txn, "/A", pool));
> + SVN_ERR(svn_fs_abort_txn(top_txn, pool));
> + }
> + else
> + {
> + SVN_ERR(test_commit_txn(&head_rev, top_txn, NULL, pool));
> + }
> +
> + /* The inverted case is not that trivial to handle. Hence, conflict.
> + Depending on the checking order, the reported conflict path differs.
> */
> + SVN_ERR(test_commit_txn(&head_rev, sub_txn, is_bdb ? "/A/D" : "/A",
> pool));
> + SVN_ERR(svn_fs_abort_txn(sub_txn, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
>
> /*
> ------------------------------------------------------------------------ */
>
> @@ -5315,5 +5401,7 @@ struct svn_test_descriptor_t test_funcs[
> "commit timestamp"),
> SVN_TEST_OPTS_PASS(test_compat_version,
> "test svn_fs__compatible_version"),
> + SVN_TEST_OPTS_PASS(dir_prop_merge,
> + "test merge directory properties"),
> SVN_TEST_NULL
> };
>
>
>