On 8/10/14 5:19 PM, Alexey Neyman wrote:
> - First, if the fs.change_node_prop was not intended for use in scripts, why is
> it exposed in bindings at all?
Bindings are not intended just for writing hook scripts. Rather the bindings
are intended to wrap the entirety of our C API (though sometimes the wrapping
doesn't always work because we add bits and forget to update it).
We've written all sorts of things with the bindings that are very low level.
It's not uncommon for us to take a higher level feature and implement it with
the bindings to work out if we like how it works before making changes to our C
APIs.
> - Second, if you care to read the quoted paragraph, you'd notice that it is
> phrased as a recommendation, not a restriction. For example, here's how the
> IETF determines the requirement levels:
>
>
>
> tools.ietf.org/html/rfc2119
>
> 3. SHOULD This word, or the adjective "RECOMMENDED", mean that there
>
> may exist valid reasons in particular circumstances to ignore a
>
> particular item, but the full implications must be understood and
>
> carefully weighed before choosing a different course.
Our book isn't an RFC.
Here's the most important sentence form the book:
"While hook scripts can do almost anything, there is one dimension in which
hook script authors should show restraint: do not modify a commit transaction
using hook scripts."
Yes the word should appears in that sentence. But we really are intending to
tell you not to modify a transaction. I suspect the word should is there
because there is actually one thing that is ok to modify in a hook script and
that's the unversioned properties that will become revision properties.
Perhaps the language could be improved, but I do think that italics on the word
"not" and the following text should give you a pretty good idea that this is
not a supported operation.
> - Third, if you care to read the original report I sent - you'd notice that it
> does not involve client caches at all (which is the potential issue that the
> SVN book warns about with respect to modifying the transaction). Instead, the
> transaction is modified in a way different from how the script attempted to
> modify it.
Actually this still makes the client that's doing the commit have a stale cache
of the property. It will believe that "myprop" either has no value or whatever
the previous value is.
You probably won't pick up any change to this property until the next time the
node changes. However, you can get away this if you're not terribly concerned
about the client having the correct state of the property because we never send
deltas for property changes. We just send the whole property value.
If that ever changes (which is entirely conceivable since there are property
use case that quite honestly have probably grown beyond the original intent of
properties, including svn:mergeinfo) you will be in a world of hurt since your
existing working copies when upgraded and run with clients that understand this
and servers that handle this will suddenly be busted. Every commit until you
remove this will break the working copy.
So I'm sure you've been getting away with this just fine. We however, won't
encourage you to do it because we don't promise not to break it.
I haven't dug in to the specifics of how we broke this particular use case.
But I suspect you'll find that if you create a transaction, call
svn_fs_change_node_prop() and commit the transaction that this call works just
fine. So I'd bet that the problem only happens when you try to do this from
the pre-commit hook. In fact we actually do exactly what I suggest in several
places in our test suite (though it happens to be in C but you're calling the
same function and the Python bindings are hardly wrapped). For example this
code from subversion/tests/libsvn_repos/dump-load-test.c in
est_dump_r0_mergeinfo():
[[[
/* Revision 2: Add bad mergeinfo */
SVN_ERR(svn_fs_begin_txn2(&txn, fs, youngest_rev, 0, pool));
SVN_ERR(svn_fs_txn_root(&txn_root, txn, pool));
SVN_ERR(svn_fs_change_node_prop(txn_root, "/bar", "svn:mergeinfo",
bad_mergeinfo, pool));
SVN_ERR(svn_repos_fs_commit_txn(NULL, repos, &youngest_rev, txn, pool));
SVN_TEST_ASSERT(SVN_IS_VALID_REVNUM(youngest_rev));
]]]
As a result my bet would be that someone made the assumption that this wasn't
allowed and made an optimization assuming that nobody does that.
All I can really tell you (without spending more of my Sunday on this), is that
we told you so in the documentation.
Received on 2014-08-11 03:21:36 CEST