Re: [RFC/PATCH] Modifying internal FS transaction properties

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
Received on 2015-03-03 11:21:38 CET