I'd like to draw attention to one aspect of the FS API that received a change
in Subversion 1.9.
We have 2 functions, svn_fs_change_txn_prop() and svn_fs_change_txn_props(),
that allow changing properties of the existing transactions in a filesystem.
I think we might want to revisit their behavior in terms of the transaction
properties that we allow to change and in terms of what we do when a caller
attempts to change an internal property like 'svn:check-locks' or
'svn:client-date':
1) Currently we allow changing 'svn:check-ood' and 'svn:check-locks'. We also
silently discard a property change whenever it targets 'svn:client-date'.
I find the first part questionable, because we basically allow a caller to
mess with our private implementation details. Existence of the internal
properties is a consequence of the fact that we use them to persist the
transaction flags on the disk, but if, for example, we were using another
way of storing these flags, I doubt that we'd expose an ability to change
them through our API.
Apart from what is stated above, I think that silently discarding changes
to 'svn:client-date' is somewhat broken. As a caller, I would expect the
successive call to a function like svn_fs_change_txn_prop() to mean that the
requested change was applied without any problems. This is not true with
'svn:client-date', because doing so doesn't result in an error, but also
*doesn't change the property*. In other words, the change goes straight to
/dev/null, but the API says that the operation completed successfully.
I propose that we solve this part of the problem with the attached patch.
Please note that while preparing this patch, I also unveiled that the
SVN_FS_TXN_CLIENT_DATE flag might not work as expected with BDB.
I think we should fix this with a separate patch; see the comment in the
new test_internal_txn_props() test.
Log message:
[[[
Error out on attempts to change internal transaction properties like
'svn:check-locks' through the public FS API.
When creating a transaction, a caller can specify one or multiple flags like
SVN_FS_TXN_CHECK_LOCKS. These flags affect how the transaction behaves
when being prepared or committed. For example, a caller can tell us that
she/he doesn't want the back end to perform on-the-fly lock checking or might
want to specify the final 'svn:date' value for the transaction. We want to
persist these flags even if a transaction is reopened — that's why we map
them into a set of internal transaction properties, set these properties when
a transaction is created, and drop them just before the commit.
Prior to this changeset we allowed changing these internal properties with
our API and were somewhat picky about 'svn:client-date', i.e., any attempts
to change it were silently discarded. Instead of that, now we explicitly
prohibit changes for *all* known internal transaction properties.
We don't need this ability ourselves, and with this change we no longer allow
the callers to mess with our private implementation details. Furthermore,
this makes the corresponding API calls do what they were told to do or fail.
This is a better choice compared to silently stripping out 'svn:client-date'
changes and saying that everything went fine. As a caller, I would expect a
successive call to mean that the requested changes were applied without any
problems, and that's the behavior we get with this patch.
* subversion/libsvn_fs/fs-loader.c
(is_internal_txn_prop): New helper function.
(svn_fs_change_txn_prop): Error out on attempts to change internal
transaction properties.
(svn_fs_change_txn_props): Error out on attempts to change internal
transaction properties. Instead of silently skipping 'svn:client-date'
changes, we now use "all-or-nothing" approach and can drop the property
filtering code.
* subversion/tests/libsvn_fs/fs-test.c
(commit_timestamp): Don't test what happens if we try to alter the
'svn:client-date' property here. This (and other possible scenarios)
should now be covered by ...
(test_internal_txn_props): ...this new test.
(test_funcs): Reference new test.
]]]
2) These internal properties currently are a part of what svn_fs_txn_proplist()
and svn_fs_txn_prop() return. From my point of view, they are nothing more
than an implementation detail and I don't think that something like that
should ever reach the caller of a public API. However, this is how things
have been working for a long time, and I am not sure about changing it now,
although it does make sense to me.
Thoughts?
Regards,
Evgeny Kotkov
Index: subversion/libsvn_fs/fs-loader.c
===================================================================
--- subversion/libsvn_fs/fs-loader.c (revision 1663355)
+++ subversion/libsvn_fs/fs-loader.c (working copy)
@@ -929,13 +929,22 @@ svn_fs_txn_proplist(apr_hash_t **table_p, svn_fs_t
return svn_error_trace(txn->vtable->get_proplist(table_p, txn, pool));
}
+static svn_boolean_t
+is_internal_txn_prop(const char *name)
+{
+ return strcmp(name, SVN_FS__PROP_TXN_CHECK_LOCKS) == 0 ||
+ strcmp(name, SVN_FS__PROP_TXN_CHECK_OOD) == 0 ||
+ strcmp(name, SVN_FS__PROP_TXN_CLIENT_DATE) == 0;
+}
+
svn_error_t *
svn_fs_change_txn_prop(svn_fs_txn_t *txn, const char *name,
const svn_string_t *value, apr_pool_t *pool)
{
- /* Silently drop attempts to modify the internal property. */
- if (!strcmp(name, SVN_FS__PROP_TXN_CLIENT_DATE))
- return SVN_NO_ERROR;
+ if (is_internal_txn_prop(name))
+ return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
+ _("Attempt to modify internal transaction "
+ "property '%s'"), name);
return svn_error_trace(txn->vtable->change_prop(txn, name, value, pool));
}
@@ -946,25 +955,14 @@ svn_fs_change_txn_props(svn_fs_txn_t *txn, const a
{
int i;
- /* Silently drop attempts to modify the internal property. */
for (i = 0; i < props->nelts; ++i)
{
svn_prop_t *prop = &APR_ARRAY_IDX(props, i, svn_prop_t);
- if (!strcmp(prop->name, SVN_FS__PROP_TXN_CLIENT_DATE))
- {
- apr_array_header_t *reduced_props
- = apr_array_make(pool, props->nelts - 1, sizeof(svn_prop_t));
-
- for (i = 0; i < props->nelts; ++i)
- {
- prop = &APR_ARRAY_IDX(props, i, svn_prop_t);
- if (strcmp(prop->name, SVN_FS__PROP_TXN_CLIENT_DATE))
- APR_ARRAY_PUSH(reduced_props, svn_prop_t) = *prop;
- }
- props = reduced_props;
- break;
- }
+ if (is_internal_txn_prop(prop->name))
+ return svn_error_createf(SVN_ERR_INCORRECT_PARAMS, NULL,
+ _("Attempt to modify internal transaction "
+ "property '%s'"), prop->name);
}
return svn_error_trace(txn->vtable->change_props(txn, props, pool));
Index: subversion/tests/libsvn_fs/fs-test.c
===================================================================
--- subversion/tests/libsvn_fs/fs-test.c (revision 1663355)
+++ subversion/tests/libsvn_fs/fs-test.c (working copy)
@@ -5196,29 +5196,6 @@ commit_timestamp(const svn_test_opts_t *opts,
/* Commit that overwrites the specified svn:date. */
SVN_ERR(svn_fs_begin_txn(&txn, fs, rev, pool));
- {
- /* Setting the internal property doesn't enable svn:date behaviour. */
- apr_array_header_t *props = apr_array_make(pool, 3, sizeof(svn_prop_t));
- svn_prop_t prop, other_prop1, other_prop2;
- svn_string_t *val;
-
- prop.name = SVN_FS__PROP_TXN_CLIENT_DATE;
- prop.value = svn_string_create("1", pool);
- other_prop1.name = "foo";
- other_prop1.value = svn_string_create("fooval", pool);
- other_prop2.name = "bar";
- other_prop2.value = svn_string_create("barval", pool);
- APR_ARRAY_PUSH(props, svn_prop_t) = other_prop1;
- APR_ARRAY_PUSH(props, svn_prop_t) = prop;
- APR_ARRAY_PUSH(props, svn_prop_t) = other_prop2;
- SVN_ERR(svn_fs_change_txn_props(txn, props, pool));
- SVN_ERR(svn_fs_txn_prop(&val, txn, other_prop1.name, pool));
- SVN_TEST_ASSERT(val && !strcmp(val->data, other_prop1.value->data));
- SVN_ERR(svn_fs_txn_prop(&val, txn, other_prop2.name, pool));
- SVN_TEST_ASSERT(val && !strcmp(val->data, other_prop2.value->data));
-
- SVN_ERR(svn_fs_change_txn_prop(txn, prop.name, prop.value, pool));
- }
SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
SVN_ERR(svn_fs_make_dir(txn_root, "/bar", pool));
SVN_ERR(svn_fs_change_txn_prop(txn, SVN_PROP_REVISION_DATE, date, pool));
@@ -6698,6 +6675,119 @@ test_prop_and_text_rep_sharing_collision(const svn
return SVN_NO_ERROR;
}
+static svn_error_t *
+test_internal_txn_props(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ svn_fs_t *fs;
+ svn_fs_txn_t *txn;
+ svn_string_t *val;
+ svn_prop_t prop;
+ svn_prop_t internal_prop;
+ apr_array_header_t *props;
+ apr_hash_t *actual_props;
+ svn_error_t *err;
+
+ SVN_ERR(svn_test__create_fs(&fs, "test-repo-internal-txn-props",
+ opts, pool));
+ SVN_ERR(svn_fs_begin_txn2(&txn, fs, 0,
+ SVN_FS_TXN_CHECK_OOD |
+ SVN_FS_TXN_CHECK_LOCKS |
+ SVN_FS_TXN_CLIENT_DATE, pool));
+
+ val = svn_string_create("Ooops!", pool);
+
+ /* All attempts to modify internal txn properties should error out. */
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_OOD, val, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_OOD, NULL, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_LOCKS, val, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CHECK_LOCKS, NULL, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CLIENT_DATE, val, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+ err = svn_fs_change_txn_prop(txn, SVN_FS__PROP_TXN_CLIENT_DATE, NULL, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+
+ /* Try the same with svn_fs_change_txn_props(). */
+ prop.name = "foo";
+ prop.value = svn_string_create("bar", pool);
+ internal_prop.name = SVN_FS__PROP_TXN_CHECK_LOCKS;
+ internal_prop.value = svn_string_create("Ooops!", pool);
+
+ props = apr_array_make(pool, 2, sizeof(svn_prop_t));
+ APR_ARRAY_PUSH(props, svn_prop_t) = prop;
+ APR_ARRAY_PUSH(props, svn_prop_t) = internal_prop;
+
+ err = svn_fs_change_txn_props(txn, props, pool);
+ SVN_TEST_ASSERT_ERROR(err, SVN_ERR_INCORRECT_PARAMS);
+
+ /* Ensure that the txn properties are left unchanged:
+
+ K 13
+ svn:check-ood
+ V 4
+ true
+ K 15
+ svn:check-locks
+ V 4
+ true
+ K 15
+ svn:client-date
+ V 1
+ 0
+ K 8
+ svn:date
+ V 27
+ (timestamp like "2015-03-02T12:16:53.148931Z")
+ END
+ */
+ SVN_ERR(svn_fs_txn_proplist(&actual_props, txn, pool));
+
+ SVN_TEST_ASSERT(apr_hash_count(actual_props) == 4);
+
+ val = svn_hash_gets(actual_props, SVN_FS__PROP_TXN_CHECK_OOD);
+ SVN_TEST_ASSERT(val);
+ SVN_TEST_STRING_ASSERT(val->data, "true");
+
+ val = svn_hash_gets(actual_props, SVN_FS__PROP_TXN_CHECK_LOCKS);
+ SVN_TEST_ASSERT(val);
+ SVN_TEST_STRING_ASSERT(val->data, "true");
+
+ val = svn_hash_gets(actual_props, SVN_FS__PROP_TXN_CLIENT_DATE);
+ SVN_TEST_ASSERT(val);
+
+ if (strcmp(opts->fs_type, SVN_FS_TYPE_BDB) == 0)
+ {
+ /* ### Skip the svn:client-date check with BDB. I think that we have a
+ bug in the BDB backend, because when SVN_FS_TXN_CLIENT_DATE is
+ set, a transaction created with svn_fs_begin_txn2() has the
+ svn:client-date property value = "1", even though no explicit
+ svn:date changes happened (it's "0" in FSFS).
+
+ This happens because svn_fs_base__begin_txn() sets svn:date in a
+ separate svn_fs_base__change_txn_prop() call when the initial
+ internal properties, including SVN_FS__PROP_TXN_CLIENT_DATE, are
+ already set. As a consequence, we should have different bevavior
+ for transactions created with SVN_FS_TXN_CLIENT_DATE, and with no
+ explicit svn:date changes happening depending on the FS backend.
+ When such transaction is committed, FSFS is going to use the time
+ of the commit for the 'svn:date' value. BDB, however, is going
+ to erroneously use the timestamp of the svn_fs_base__begin_txn()
+ call.
+ */
+ }
+ else
+ SVN_TEST_STRING_ASSERT(val->data, "0");
+
+ val = svn_hash_gets(actual_props, SVN_PROP_REVISION_DATE);
+ SVN_TEST_ASSERT(val);
+
+ return SVN_NO_ERROR;
+}
+
/* ------------------------------------------------------------------------ */
/* The test table. */
@@ -6829,6 +6919,8 @@ static struct svn_test_descriptor_t test_funcs[] =
"test modify txn being written in FSFS"),
SVN_TEST_OPTS_PASS(test_prop_and_text_rep_sharing_collision,
"test property and text rep-sharing collision"),
+ SVN_TEST_OPTS_PASS(test_internal_txn_props,
+ "test attempts to change internal txn properties"),
SVN_TEST_NULL
};
