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 <[email protected]> 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 <[email protected]> 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: [email protected] >> Sent: 14-1-2014 23:09 >> To: [email protected] >> 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*
