Julian Foad wrote on Thu, Dec 08, 2011 at 12:28:23 +0000: > A comment about taking out a WC write lock. > > This implementation preserves the current behaviour of taking out a WC > write lock aroung the whole reintegrate merge procedure, and does so > by using SVN_WC__CALL_WITH_WRITE_LOCK within 'svn'. That's a bit of > a layering violation; if we want 'svn' to take out such a lock then we > should provide a libsvn_client function or macro for it to use. But > although ideally we would like to ensure that no other process starts > writing to this WC (or this WC subtree), there are other places within > 'svn' where we ought to care too and I don't believe we have ever > cared enough to do anything about it yet. If we're going to deal with > this issue at all we ought to deal with it throughout 'svn'. From > that perspective, it's a side issue. >
In short: calling the non-public API SVN_WC__CALL_WITH_WRITE_LOCK() is fine because it improves upon a bad situation where we wouldn't be taking a lock at all. I don't argue with that, but how do 3rd party clients solve this problem today? Do they also ignore the need to grab a write-lock, like you say the rest of subversion/svn/ does today? > - Julian > > > > ----- Original Message ----- > > From: Julian Foad <julianf...@btopenworld.com> > > To: "dev@subversion.apache.org" <dev@subversion.apache.org> > > Cc: > > Sent: Thursday, 8 December 2011, 12:17 > > Subject: [PATCH] Split up the reintegrate merge API: first find what to do, > > then do it > > > >T he current 'reintegrate' merge consists conceptually of two stages: > > > > * Determine what the equivalent two-URL merge is. > > > > * Do the two-URL merge. > > > > as well as checking that the target WC is clean and the source and target > > are > > ancestrally related. > > > > > > My gut feeling tells me that the libsvn_client API would be better if it > > presented these as two separate functions. A small but distinctly useful > > benefit is that svn or a GUI client can then tell the user what two-URL > > merge it > > is doing, which I firmly believe will help users to understand merging and > > therefore to issue the right merge commands more often. That information > > is > > currently only available inside svn_client_merge_reintegrate(). > > > > Modularity would be improved too: if we use the standard two-URL merge API > > for > > the "do it" stage, then we have fewer different code paths used in > > merging, so less likelihood of unintentional differences (bugs). > > > > > > The reintegrate merge up till now goes like this: > > > > client code (such as subversion/svn/merge-cmd.c)... > > | > > |- svn_client_merge_reintegrate() > > | | > > | |- check target WC is a clean single-revision WC > > | |- check source and target are related > > | |- determine URLs and revs of src, tgt, and common ancestor > > | |- do a 2-URL merge > > > > and with this patch it goes like this: > > > > > > client code (such as subversion/svn/merge-cmd.c)... > > | > > |- svn_client_ensure_wc_is_suitable_merge_target() > > | | > > | |- check target WC is a clean single-revision WC > > | > > |- svn_client_find_reintegrate_merge() > > | | > > | |- check source and target are related > > | |- determine URLs and revs of src, tgt, and common ancestor > > | > > |- Tell the user what equivalent two-URL merge we're doing > > | > > > > |- svn_client_do_reintegrate_merge() > > > > | |- do a 2-URL merge > > > > The last stage is currently exposed as a dedicated 'do_reintegrate' > > function because I'm not yet completely sure whether we want it to be > > semantically exactly the same as an 'ordinary' two-URL merge. If it > > should be semantically identical, then we just need to adjust the code; > > otherwise we may need to modify the two-URL API. That's secondary work, > > but > > certainly important to ensure that we have a nice clean set of API > > functions. > > > > Any comments on this approach? > > > > - Julian > >
