[RFC/PATCH] Modifying internal FS transaction properties

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