Stefan Sperling wrote on Wed, Sep 22, 2010 at 15:45:41 +0200: > On Wed, Sep 22, 2010 at 03:33:20PM +0200, Daniel Shahaf wrote: > > Stefan Sperling wrote on Wed, Sep 22, 2010 at 14:31:58 +0200: > > > On Wed, Sep 22, 2010 at 01:05:24PM +0200, Daniel Shahaf wrote: > > > > Daniel Shahaf wrote on Tue, Sep 21, 2010 at 18:50:56 +0200: > > > > > How about combining (1) and (3) --- that is, using the old locking > > > > > mode > > > > > (with its known race condition) but print a warning that the mirror > > > > > server should be upgraded? That seems better than a fatal error for > > > > > a condition that needs to be fixed on the server end. > > > > > > > > > > > > > No replies, so I've implemented this in r999868. Let me know what you > > > > think :). > > > > > > > > The error message says "external locking"; does the svnbook define this > > > > term? > > > > > > Maybe use "disabled built-in locking", like the svnsync help sync says: > > > > > > --disable-locking : Disable built-in locking. Use of this option > > > can > > > corrupt the mirror unless you ensure that no > > > other > > > instance of svnsync is running concurrently. > > > > > > > The intention of the phrasing "external locking" is to remind users that > > they shouldn't pass --disable-locking unless they have some other, > > out-of-band mutex in use. (Which may be in their own head in the case of > > a single-user file://localhost mirror.) I'm afraid that just saying > > "disable built-in locking" will result in people passing --disable-locking > > without a second thought, which is much more likely to result in corruption. > > > > > > (so the error message could link to the workaround docs) > > > > > > There are no workaround docs, apart from some scattered mailing list > > > posts :( > > > > > > I'd prefer requiring people to pass either --disable-locking or > > > --use-pre-1.7-style-locking to write to mirrors using pre-1.7 servers. > > > > I see how an explicit --use-pre-1.7-style-locking would be justified if > > the race condition was easy to trigger and often caused corruptions, but > > I don't recall hearing much reports of the corruption issue on us...@. > > Does the corruption issue (considering its severity and commonness) > > really warrant an explicit flag? > > See Jon's comment made earlier about data integrity. > > But, granted, there isn't any loss of versioned data when the race triggers. > The mirror needs to be unwedged by tweaking the svn:sync-* revprops at > revision 0. It will then resume operation. >
Unwedging the mirror is not bad; if some commit were replayed twice to the mirror, /then/ I would be worried. :-) Anyway, we could add that --use-pre-1.7-style-locking flag, or we could make the warning message more strongly worded (say that it has been known to corrupt/wedge mirror metadata), or probably a few other options. I don't feel strongly here; please feel free to jump in and implement some saner handling for this case (a pre-1.7 target server). > > > > The docstring on trunk has always promised only a racy implementation. > > > > I'll just leave that as-is. > > > > > > But the Book hasn't. That is the problem. > > > Our users don't read the docstrings, and shouldn't have to. > > > > > > > Our users don't call svn_client_revprop_set2(); our API consumers do. > > And they should have warned *their* users about the raciness of that API > > since long before the branch. > > We're also consumers of our APIs, so we should have warned our users :) > We've done so partly on the users@ list, if people happened to read > the relevant threads. But we should update the book, too. I'd say most > people rely on the book. > Agreed. (but, and especially for this particular issue, I think you'll do a better job than me of updating the book) > > One of those consumers is 'svn propedit --revprop'; are you suggesting we > > change the way 'svn propedit --revprop' works with pre-1.7 servers? > > Hmmm... no. And that's actually a good point against having a command line > switch that is essentially there to allow svnsync to make revprop edits. > > > On a more general issue, svn_client_revprop_set2()'s docstring promises > > that the change will be atomic if the server has the > > SVN_RA_CAPABILITY_ATOMIC_REVPROPS capability, but I'm not sure whether > > that condition is testable by svn_client_revprop_set2()'s caller? > > Not sure. I don't know off-hand if the client has a way to query the > server's capabilities without doing a commit or prop edit. > That function opens the RA session by itself, so the caller can't check that. But the function could grow a boolean "bail if atomicity is unavailable" parameter.
