Re: svn commit: r1558224 - in /subversion/trunk/subversion:libsvn_fs_fs/tree.c libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c

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 <stefan.fuhrmann_at_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 <bert_at_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_at_apache.org
>> Sent: 14-1-2014 23:09
>> To: commits_at_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(&not_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(&not_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(&not_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*