On Thu, Aug 12, 2010 at 6:01 PM, Paul Burba <ptbu...@gmail.com> wrote: > On Tue, Aug 3, 2010 at 12:40 PM, Paul Burba <ptbu...@gmail.com> wrote: >> Ok, I am waving the white flag as far as implementing a fix for issue >> #3657 in the RA layer. I simply don't see how it can be done outside >> of this: Put the DAV update report handler into send-all=TRUE >> <S:update-report send-all=TRUE> mode when creating a >> svn_ra_reporter3_t * during a merge/diff. This would prevent >> <S:fetch-props/> response that causes the client to fetch *all* the >> properties of a path which subsequently pushes these to the >> svn_delta_editor_t callbacks as if they were all prop *changes* (as >> Mike discussed here >> http://subversion.tigris.org/issues/show_bug.cgi?id=3657#desc9). >> >> fyi: Currently this is how the two DAV RA layers use send-all mode: >> >> send-all send-all >> mode mode >> operation ra_serf ra_neon >> --------- -------- --------- >> update FALSE TRUE >> status FALSE TRUE >> switch FALSE TRUE >> diff FALSE FALSE >> >> In that attached patch I reverted r966822 and tried the aforementioned >> approach by tweaking ra_neon so the send-all mode on diff/merge is >> TRUE. The whole test suite passes (including the test for issue >> #3657). I haven't got an equivalent change to ra_serf to work yet, >> but I'm not too worried about that yet because... >> >> ...After testing out this idea, I realized the problem with this >> approach: It reopens issue #2048 'Primary connection (of parallel >> network connections used) in ra-dav diff/merge timeout >> unnecessarily.': >> >> http://subversion.tigris.org/issues/show_bug.cgi?id=2048 >> http://svn.apache.org/viewvc?view=revision&revision=853457 >> >> So I'm at a dead-end right now. I'm happy to revert r966822 and >> reopen issue #3657 if the consensus is that my initial fix is a sloppy >> band-aid to cover incorrect ra_neon/ra_serf behavior and that the >> latter two are where the fix belongs. >> >> Paul > > Mike suggested to me that the fix for this problem can be made in > mod_dav_svn, since the update reporter already knows which properties > have changed, *regardless* of the send-all mode, but it discards this > information if send-all=FALSE and instead instead sends the client a > 'fetch-props' response. As described previously in this thread, this > causes the client to fetch all the properties of the path, not just > the changes, and *all* the props are pushed through the editor > callbacks as if they were all actual changes (which again is the core > problem of issue #3657). > > I'm working on that approach,
I had pushed off the fix-it-in-mod_dav_svn approach till 1.8 since I was going in circles with it, but recently found another real-world example where this issue causes spurious conflicts on directory properties, see http://subversion.tigris.org/issues/show_bug.cgi?id=3657#desc13. This issue has existed before 1.5 so it's nothing new, but given that I found another non-theoretical example and given that the presence of mergeinfo is only going to make this issue more common[1] I thought I'd take another look for 1.7. I still haven't gotten anywhere with the mod_dav_svn fix, so not much to talk about there, but I did commit the fix for the directory property conflicts in http://svn.apache.org/viewvc?view=revision&revision=1056565. The fix here is pretty much the same thing I did in http://svn.apache.org/viewvc?view=revision&revision=966822: Filter out the phony prop changes in the libsvn_client/repos_diff.c svn_delta_editor_t change_[dir|file]_prop() callback implementations. Just to be clear about one thing, when I committed r966822 Bert asked: >> (change_file_prop): Only stash true property differences in the file >> baton. The DAV RA providers may be sending us all the properties on >> a file, not just the differences. > > Shouldn't this be fixed in the ra layer implementations then? > > This would be just 'guessing' if it is a change or a complete set. Why not the RA layer? Two reasons. The first I explained earlier in this thread, it reopens issue #2048 'Primary connection (of parallel network connections used) in ra-dav diff/merge timeout unnecessarily.' I won't go over that again, but even if there is a way to avoid that, the RA layer is simply doing what we ask it when using the DAV providers. Specifically, the mod_dav_svn update report handler, when in 'skelta' mode[2] and describing changes to a path on which a property has changed, asks the client to later fetch all properties and figure out what has changed itself, i.e. the client asks the RA layer to give us *all* the properties, it obliges, so where is the problem as far as the RA layer is concerned? So this ultimately needs to be fixed on the server. Given that, I am leaving these libsvn_client filters in place so a 1.7 client will DTRT with older servers (or maybe 1.7 servers too, since as I said, I'm not having great success there). As to the 'guessing', that was true. We'd go through the motions of filtering over ra_local and ra_svn too. A pretty minor performance hit, but in r1056565 I tweaked the filtering so it only applies when using ra_serf and ra_neon. Paul [1] simply by increasing the potential pool of property changes incoming during a merge. [2] Which merges always are, regardless of whether we use ra_serf or ra_neon. Paul
