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.