You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2008/12/23 17:10:25 UTC
Re: svn commit: r34896 - trunk/subversion/tests/libsvn_fs_base
This test seems to only exercise the FS API, so it ought to pass
against FSFS too. Why not put it in fs-test? Just because FSFS
didn't have this bug now (or did it?) doesn't mean it's not a worthy
test.
--dave
On Tue, Dec 23, 2008 at 7:35 AM, C. Michael Pilato <cm...@collab.net> wrote:
> Author: cmpilato
> Date: Tue Dec 23 07:35:08 2008
> New Revision: 34896
>
> Log:
> Add regression text for issue #3349 (BDB backend could allow `changes'
> table out-of-sync with actual changes).
>
> * subversion/tests/libsvn_fs_base/fs-base-test.c
> (orphaned_textmod_change): New regression test.
> (test_funcs): Add reference to new test.
>
> Modified:
> trunk/subversion/tests/libsvn_fs_base/fs-base-test.c
>
> Modified: trunk/subversion/tests/libsvn_fs_base/fs-base-test.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/libsvn_fs_base/fs-base-test.c?pathrev=34896&r1=34895&r2=34896
> ==============================================================================
> --- trunk/subversion/tests/libsvn_fs_base/fs-base-test.c Tue Dec 23 05:34:01 2008 (r34895)
> +++ trunk/subversion/tests/libsvn_fs_base/fs-base-test.c Tue Dec 23 07:35:08 2008 (r34896)
> @@ -1463,6 +1463,65 @@ redundant_copy(const char **msg,
> }
>
>
> +static svn_error_t *
> +orphaned_textmod_change(const char **msg,
> + svn_boolean_t msg_only,
> + svn_test_opts_t *opts,
> + apr_pool_t *pool)
> +{
> + apr_pool_t *subpool = svn_pool_create(pool);
> + svn_fs_t *fs;
> + svn_fs_txn_t *txn;
> + svn_fs_root_t *txn_root, *root;
> + svn_revnum_t youngest_rev = 0;
> + svn_txdelta_window_handler_t wh_func;
> + void *wh_baton;
> + apr_hash_t *changed_paths;
> +
> + *msg = "test for orphaned textmod changed paths";
> + if (msg_only)
> + return SVN_NO_ERROR;
> +
> + /* Create a filesystem and repository. */
> + SVN_ERR(svn_test__create_bdb_fs(&fs, "test-repo-orphaned-changes", opts,
> + pool));
> +
> + /* Revision 1: Create and commit the greek tree. */
> + SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, subpool));
> + SVN_ERR(svn_fs_txn_root(&txn_root, txn, subpool));
> + SVN_ERR(svn_test__create_greek_tree(txn_root, subpool));
> + SVN_ERR(svn_fs_commit_txn(NULL, &youngest_rev, txn, subpool));
> + svn_pool_clear(subpool);
> +
> + /* Revision 2: Start to change "iota", but don't complete the work. */
> + SVN_ERR(svn_fs_begin_txn(&txn, fs, youngest_rev, subpool));
> + SVN_ERR(svn_fs_txn_root(&txn_root, txn, subpool));
> + SVN_ERR(svn_fs_apply_textdelta
> + (&wh_func, &wh_baton, txn_root, "iota", NULL, NULL, subpool));
> + /* Don't send any delta windows! */
> + SVN_ERR(svn_fs_commit_txn(NULL, &youngest_rev, txn, subpool));
> + svn_pool_clear(subpool);
> +
> + /* Fetch changed paths for the youngest revision. We should find none. */
> + SVN_ERR(svn_fs_revision_root(&root, fs, youngest_rev, subpool));
> + SVN_ERR(svn_fs_paths_changed(&changed_paths, root, subpool));
> + if (apr_hash_count(changed_paths) != 0)
> + {
> + svn_fs_path_change_t *change = apr_hash_get(changed_paths, "/iota",
> + APR_HASH_KEY_STRING);
> + if (change && change->text_mod)
> + return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
> + "Got unexpected textmods changed path "
> + "for 'iota'");
> + else
> + return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
> + "Got non-empty changed paths hash where empty "
> + "one expected");
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> /* ------------------------------------------------------------------------ */
>
> /* The test table. */
> @@ -1479,5 +1538,6 @@ struct svn_test_descriptor_t test_funcs[
> SVN_TEST_PASS(canonicalize_abspath),
> SVN_TEST_PASS(skip_deltas),
> SVN_TEST_PASS(redundant_copy),
> + SVN_TEST_XFAIL(orphaned_textmod_change),
> SVN_TEST_NULL
> };
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=990751
>
--
glasser@davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=990802
Re: svn commit: r34896 - trunk/subversion/tests/libsvn_fs_base
Posted by "C. Michael Pilato" <cm...@collab.net>.
David Glasser wrote:
> Perhaps add a comment to the test saying essentially "We are testing
> that undefined calls to the FS API don't introduce more damage to the
> repository than they have to"?
r35039. Thanks, Dave.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1007714
Re: svn commit: r34896 - trunk/subversion/tests/libsvn_fs_base
Posted by David Glasser <gl...@davidglasser.net>.
On Tue, Dec 23, 2008 at 2:51 PM, C. Michael Pilato <cm...@collab.net> wrote:
> David Glasser wrote:
>> Oh, huh. So, um, which FS is implementing the FS API correctly then?
>> This sort of difference should not be externally observable...
>
> Sometime after FSFS was added, this additional restriction on the API was
> added to the docstring:
>
> * The caller must send all delta windows including the terminating
> * NULL window to @a *contents_p before making further changes to the
> * transaction.
>
> And, really, this restriction only makes any sense at all for FSFS, because
> BDB doesn't care if you start sending some windows, then go off and do other
> stuff, and then come back later to finish up the textdelta.
>
> So FSFS enforces this requirement, though I suspect not explicitly. (If you
> try to violate it, you get the generic error, "Cannot write to the prototype
> revision file of transaction 'f00' because a previous representation is
> currently being written by this process".) BDB doesn't even try, and really
> has no way to do so.
>
> So, in answer to your question, both FS APIs behave correctly if said APIs
> are used as demanded by the docstring and no problems occur. If a problem
> occurs such that the window-sending doesn't complete successfully, *and* the
> FS API driver decides not to care and commit the transaction anyway, I
> noticed that BDB would register text-mod changes on the file that was being
> edited even though those changes never "took". Now, having just typed that,
> I've looked into the FSFS code, and it records the change at the same time
> that BDB did -- when editing was started, not when it was finalized. But I
> don't know enough about FSFS to say if that leads to the same busted place
> that BDB does, though, and I can't easily construct a test case that obeys
> the API's documented rules *and* still triggers the problem.
I see.
Perhaps add a comment to the test saying essentially "We are testing
that undefined calls to the FS API don't introduce more damage to the
repository than they have to"?
--dave
--
glasser@davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=991050
Re: svn commit: r34896 - trunk/subversion/tests/libsvn_fs_base
Posted by "C. Michael Pilato" <cm...@collab.net>.
David Glasser wrote:
> Oh, huh. So, um, which FS is implementing the FS API correctly then?
> This sort of difference should not be externally observable...
Sometime after FSFS was added, this additional restriction on the API was
added to the docstring:
* The caller must send all delta windows including the terminating
* NULL window to @a *contents_p before making further changes to the
* transaction.
And, really, this restriction only makes any sense at all for FSFS, because
BDB doesn't care if you start sending some windows, then go off and do other
stuff, and then come back later to finish up the textdelta.
So FSFS enforces this requirement, though I suspect not explicitly. (If you
try to violate it, you get the generic error, "Cannot write to the prototype
revision file of transaction 'f00' because a previous representation is
currently being written by this process".) BDB doesn't even try, and really
has no way to do so.
So, in answer to your question, both FS APIs behave correctly if said APIs
are used as demanded by the docstring and no problems occur. If a problem
occurs such that the window-sending doesn't complete successfully, *and* the
FS API driver decides not to care and commit the transaction anyway, I
noticed that BDB would register text-mod changes on the file that was being
edited even though those changes never "took". Now, having just typed that,
I've looked into the FSFS code, and it records the change at the same time
that BDB did -- when editing was started, not when it was finalized. But I
don't know enough about FSFS to say if that leads to the same busted place
that BDB does, though, and I can't easily construct a test case that obeys
the API's documented rules *and* still triggers the problem.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=991039
Re: svn commit: r34896 - trunk/subversion/tests/libsvn_fs_base
Posted by David Glasser <gl...@davidglasser.net>.
Oh, huh. So, um, which FS is implementing the FS API correctly then?
This sort of difference should not be externally observable...
--dave
On Tue, Dec 23, 2008 at 10:40 AM, C. Michael Pilato <cm...@collab.net> wrote:
> FSFS fails with a different error altogether, preventing the commit, and
> forcing the entire test logic down a whole 'nuther path. Rather than have a
> giant conditional in the test logic, I just wrote the test for fs-base.
>
> David Glasser wrote:
>> This test seems to only exercise the FS API, so it ought to pass
>> against FSFS too. Why not put it in fs-test? Just because FSFS
>> didn't have this bug now (or did it?) doesn't mean it's not a worthy
>> test.
>>
>> --dave
>>
>> On Tue, Dec 23, 2008 at 7:35 AM, C. Michael Pilato <cm...@collab.net> wrote:
>>> Author: cmpilato
>>> Date: Tue Dec 23 07:35:08 2008
>>> New Revision: 34896
>>>
>>> Log:
>>> Add regression text for issue #3349 (BDB backend could allow `changes'
>>> table out-of-sync with actual changes).
>>>
>>> * subversion/tests/libsvn_fs_base/fs-base-test.c
>>> (orphaned_textmod_change): New regression test.
>>> (test_funcs): Add reference to new test.
>>>
>>> Modified:
>>> trunk/subversion/tests/libsvn_fs_base/fs-base-test.c
>>>
>>> Modified: trunk/subversion/tests/libsvn_fs_base/fs-base-test.c
>>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/libsvn_fs_base/fs-base-test.c?pathrev=34896&r1=34895&r2=34896
>>> ==============================================================================
>>> --- trunk/subversion/tests/libsvn_fs_base/fs-base-test.c Tue Dec 23 05:34:01 2008 (r34895)
>>> +++ trunk/subversion/tests/libsvn_fs_base/fs-base-test.c Tue Dec 23 07:35:08 2008 (r34896)
>>> @@ -1463,6 +1463,65 @@ redundant_copy(const char **msg,
>>> }
>>>
>>>
>>> +static svn_error_t *
>>> +orphaned_textmod_change(const char **msg,
>>> + svn_boolean_t msg_only,
>>> + svn_test_opts_t *opts,
>>> + apr_pool_t *pool)
>>> +{
>>> + apr_pool_t *subpool = svn_pool_create(pool);
>>> + svn_fs_t *fs;
>>> + svn_fs_txn_t *txn;
>>> + svn_fs_root_t *txn_root, *root;
>>> + svn_revnum_t youngest_rev = 0;
>>> + svn_txdelta_window_handler_t wh_func;
>>> + void *wh_baton;
>>> + apr_hash_t *changed_paths;
>>> +
>>> + *msg = "test for orphaned textmod changed paths";
>>> + if (msg_only)
>>> + return SVN_NO_ERROR;
>>> +
>>> + /* Create a filesystem and repository. */
>>> + SVN_ERR(svn_test__create_bdb_fs(&fs, "test-repo-orphaned-changes", opts,
>>> + pool));
>>> +
>>> + /* Revision 1: Create and commit the greek tree. */
>>> + SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, subpool));
>>> + SVN_ERR(svn_fs_txn_root(&txn_root, txn, subpool));
>>> + SVN_ERR(svn_test__create_greek_tree(txn_root, subpool));
>>> + SVN_ERR(svn_fs_commit_txn(NULL, &youngest_rev, txn, subpool));
>>> + svn_pool_clear(subpool);
>>> +
>>> + /* Revision 2: Start to change "iota", but don't complete the work. */
>>> + SVN_ERR(svn_fs_begin_txn(&txn, fs, youngest_rev, subpool));
>>> + SVN_ERR(svn_fs_txn_root(&txn_root, txn, subpool));
>>> + SVN_ERR(svn_fs_apply_textdelta
>>> + (&wh_func, &wh_baton, txn_root, "iota", NULL, NULL, subpool));
>>> + /* Don't send any delta windows! */
>>> + SVN_ERR(svn_fs_commit_txn(NULL, &youngest_rev, txn, subpool));
>>> + svn_pool_clear(subpool);
>>> +
>>> + /* Fetch changed paths for the youngest revision. We should find none. */
>>> + SVN_ERR(svn_fs_revision_root(&root, fs, youngest_rev, subpool));
>>> + SVN_ERR(svn_fs_paths_changed(&changed_paths, root, subpool));
>>> + if (apr_hash_count(changed_paths) != 0)
>>> + {
>>> + svn_fs_path_change_t *change = apr_hash_get(changed_paths, "/iota",
>>> + APR_HASH_KEY_STRING);
>>> + if (change && change->text_mod)
>>> + return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
>>> + "Got unexpected textmods changed path "
>>> + "for 'iota'");
>>> + else
>>> + return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
>>> + "Got non-empty changed paths hash where empty "
>>> + "one expected");
>>> + }
>>> +
>>> + return SVN_NO_ERROR;
>>> +}
>>> +
>>> /* ------------------------------------------------------------------------ */
>>>
>>> /* The test table. */
>>> @@ -1479,5 +1538,6 @@ struct svn_test_descriptor_t test_funcs[
>>> SVN_TEST_PASS(canonicalize_abspath),
>>> SVN_TEST_PASS(skip_deltas),
>>> SVN_TEST_PASS(redundant_copy),
>>> + SVN_TEST_XFAIL(orphaned_textmod_change),
>>> SVN_TEST_NULL
>>> };
>>>
>>> ------------------------------------------------------
>>> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=990751
>>>
>>
>>
>>
>
>
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
>
--
glasser@davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=990979
Re: svn commit: r34896 - trunk/subversion/tests/libsvn_fs_base
Posted by "C. Michael Pilato" <cm...@collab.net>.
FSFS fails with a different error altogether, preventing the commit, and
forcing the entire test logic down a whole 'nuther path. Rather than have a
giant conditional in the test logic, I just wrote the test for fs-base.
David Glasser wrote:
> This test seems to only exercise the FS API, so it ought to pass
> against FSFS too. Why not put it in fs-test? Just because FSFS
> didn't have this bug now (or did it?) doesn't mean it's not a worthy
> test.
>
> --dave
>
> On Tue, Dec 23, 2008 at 7:35 AM, C. Michael Pilato <cm...@collab.net> wrote:
>> Author: cmpilato
>> Date: Tue Dec 23 07:35:08 2008
>> New Revision: 34896
>>
>> Log:
>> Add regression text for issue #3349 (BDB backend could allow `changes'
>> table out-of-sync with actual changes).
>>
>> * subversion/tests/libsvn_fs_base/fs-base-test.c
>> (orphaned_textmod_change): New regression test.
>> (test_funcs): Add reference to new test.
>>
>> Modified:
>> trunk/subversion/tests/libsvn_fs_base/fs-base-test.c
>>
>> Modified: trunk/subversion/tests/libsvn_fs_base/fs-base-test.c
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/tests/libsvn_fs_base/fs-base-test.c?pathrev=34896&r1=34895&r2=34896
>> ==============================================================================
>> --- trunk/subversion/tests/libsvn_fs_base/fs-base-test.c Tue Dec 23 05:34:01 2008 (r34895)
>> +++ trunk/subversion/tests/libsvn_fs_base/fs-base-test.c Tue Dec 23 07:35:08 2008 (r34896)
>> @@ -1463,6 +1463,65 @@ redundant_copy(const char **msg,
>> }
>>
>>
>> +static svn_error_t *
>> +orphaned_textmod_change(const char **msg,
>> + svn_boolean_t msg_only,
>> + svn_test_opts_t *opts,
>> + apr_pool_t *pool)
>> +{
>> + apr_pool_t *subpool = svn_pool_create(pool);
>> + svn_fs_t *fs;
>> + svn_fs_txn_t *txn;
>> + svn_fs_root_t *txn_root, *root;
>> + svn_revnum_t youngest_rev = 0;
>> + svn_txdelta_window_handler_t wh_func;
>> + void *wh_baton;
>> + apr_hash_t *changed_paths;
>> +
>> + *msg = "test for orphaned textmod changed paths";
>> + if (msg_only)
>> + return SVN_NO_ERROR;
>> +
>> + /* Create a filesystem and repository. */
>> + SVN_ERR(svn_test__create_bdb_fs(&fs, "test-repo-orphaned-changes", opts,
>> + pool));
>> +
>> + /* Revision 1: Create and commit the greek tree. */
>> + SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, subpool));
>> + SVN_ERR(svn_fs_txn_root(&txn_root, txn, subpool));
>> + SVN_ERR(svn_test__create_greek_tree(txn_root, subpool));
>> + SVN_ERR(svn_fs_commit_txn(NULL, &youngest_rev, txn, subpool));
>> + svn_pool_clear(subpool);
>> +
>> + /* Revision 2: Start to change "iota", but don't complete the work. */
>> + SVN_ERR(svn_fs_begin_txn(&txn, fs, youngest_rev, subpool));
>> + SVN_ERR(svn_fs_txn_root(&txn_root, txn, subpool));
>> + SVN_ERR(svn_fs_apply_textdelta
>> + (&wh_func, &wh_baton, txn_root, "iota", NULL, NULL, subpool));
>> + /* Don't send any delta windows! */
>> + SVN_ERR(svn_fs_commit_txn(NULL, &youngest_rev, txn, subpool));
>> + svn_pool_clear(subpool);
>> +
>> + /* Fetch changed paths for the youngest revision. We should find none. */
>> + SVN_ERR(svn_fs_revision_root(&root, fs, youngest_rev, subpool));
>> + SVN_ERR(svn_fs_paths_changed(&changed_paths, root, subpool));
>> + if (apr_hash_count(changed_paths) != 0)
>> + {
>> + svn_fs_path_change_t *change = apr_hash_get(changed_paths, "/iota",
>> + APR_HASH_KEY_STRING);
>> + if (change && change->text_mod)
>> + return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
>> + "Got unexpected textmods changed path "
>> + "for 'iota'");
>> + else
>> + return svn_error_create(SVN_ERR_TEST_FAILED, NULL,
>> + "Got non-empty changed paths hash where empty "
>> + "one expected");
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> /* ------------------------------------------------------------------------ */
>>
>> /* The test table. */
>> @@ -1479,5 +1538,6 @@ struct svn_test_descriptor_t test_funcs[
>> SVN_TEST_PASS(canonicalize_abspath),
>> SVN_TEST_PASS(skip_deltas),
>> SVN_TEST_PASS(redundant_copy),
>> + SVN_TEST_XFAIL(orphaned_textmod_change),
>> SVN_TEST_NULL
>> };
>>
>> ------------------------------------------------------
>> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=495&dsMessageId=990751
>>
>
>
>
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=990854