Evgeny Kotkov wrote: > 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. [...] silently discarding > changes to 'svn:client-date' is somewhat broken. [...] > > I propose that we solve this part of the problem with the attached patch. > [...] > > Log message: > [[[ > Error out on attempts to change internal transaction properties like > 'svn:check-locks' through the public FS API.
+1 (concept) That sounds like the logical approach. I haven't reviewed the patch itself. > [...] 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. s/successive/successful/ > 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. +1. The only test of svn_fs_txn_proplist() is fs-test.c 14 "transaction_props", and it fails if I change its begin-txn code from SVN_ERR(svn_fs_begin_txn(&txn, fs, 0, pool)); to (for example) SVN_ERR(svn_fs_begin_txn2(&txn, fs, 0, SVN_FS_TXN_CHECK_OOD, pool)); because it does not expect to see any txn-props that it did not create itself, except for svn:date. The only other place where we currently acknowledge this leakage is in another test: $ grep -E -w "SVN_FS__(CHECK_LOCKS|CHECK_OOD|CLIENT_DATE)|check-locks|check-ood|client-date" \ subversion/libsvn_[^f]*/*.[ch] \ subversion/[^l]*/*.[ch] \ subversion/include/*.h \ subversion/tests/cmdline/*.py subversion/tests/cmdline/svnlook_tests.py: ' svn:check-locks\n', # internal prop, not really expected I think it would be good to change this now. I don't think there is any legitimate reason to keep exposing these properties just because we have been doing so for a long time. - Julian
