On Tue, Jan 25, 2011 at 11:27 AM, Paul Burba <ptbu...@gmail.com> wrote: > On Mon, Jan 24, 2011 at 5:01 PM, Paul Burba <ptbu...@gmail.com> wrote: >> On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato <cmpil...@collab.net> >> wrote: >>> Hey, Paul. Overall, I think the change makes sense. I've given some inline >>> review below. (This is based mostly on a reading of the patch itself, not a >>> reading of the patched source files.) >> >> Thanks for the review Mike, comments below. >> >>> On 01/18/2011 04:44 PM, Paul Burba wrote: >>>> Index: subversion/mod_dav_svn/reports/update.c >>>> =================================================================== >>>> --- subversion/mod_dav_svn/reports/update.c (revision 1060528) >>>> +++ subversion/mod_dav_svn/reports/update.c (working copy) >>> >>> [...] >>> >>>> @@ -413,8 +407,16 @@ >>>> return SVN_NO_ERROR; >>>> >>>> /* ### ack! binary names won't float here! */ >>>> - /* If this is a copied file/dir, we can have removed props. */ >>>> - if (baton->removed_props && (! baton->added || baton->copyfrom)) >>>> + /* If this is a copied file/dir, we can have removed props. >>>> + >>>> + Old features never die: 1.7+ clients don't require this block because >>>> + they never ask for copyfrom information from the server when adding >>>> + files created by a copy, but 1.5-1.6 clients will ask for it so we >>>> + have to keep sending it. >>>> + >>>> + See http://svn.haxx.se/dev/archive-2010-09/0265.shtml and >>>> + http://subversion.tigris.org/issues/show_bug.cgi?id=3711. */ >>> >>> 1.7's RA interface still allows clients to request copyfrom information, and >>> we never know if we might later use this codepath again. So I'm not sure >>> it's accurate to claim that "1.7+ clients don't require this block". Maybe >>> TortoiseSVN is using it? Maybe our repos diff code will use it (I seem to >>> recall someone talking about doing this)? I think this whole comment change >>> can be reverted. >> >> Understood, I removed that comment. >> >>>> - SVN_ERR(dav_svn__brigade_puts(baton->uc->bb, baton->uc->output, >>>> "<S:prop>")); >>>> - >>>> - /* Both modern and non-modern clients need the checksum... */ >>>> - if (baton->text_checksum) >>>> - { >>>> - SVN_ERR(dav_svn__brigade_printf(baton->uc->bb, baton->uc->output, >>>> - >>>> "<V:md5-checksum>%s</V:md5-checksum>", >>>> - baton->text_checksum)); >>>> - } >>>> - >>> >>> This removal doesn't seem right. There are two kinds of checksum sent over >>> the wire in this REPORT response: >>> >>> 1. a "base-checksum", which is the checksum of the file against which a >>> content delta is being transmitted (so the client can verify that it's about >>> to apply that delta against the right base), and >>> 2. a "text-checksum", which is the checksum of final text content (either >>> as retrieved via fulltext or via delta application to a base. >>> >>> Maybe I'm overlooking it, but it seems you're no longer transmitting the >>> text-checksum any longer. >> >> You are correct, I put that back. I must have had some text_checksum >> vs. base_checksum confusion, because despite what I said in the log, >> we *don't* send the text_checksum anywhere else. No tests failed >> because the svn_delta_editor_t close_file callback ignores >> text_checksum if it is NULL (it isn't even used during a merge, even >> if present). >> >> Rerunning the tests for the new patch (attached). > > Everything passed. Committed with a few more comment/log message > tweaks in http://svn.apache.org/viewvc?view=revision&revision=1063337 > > I suspect there is more I can do here, particularly in > mod_dav_svn/reports/update.c: I don't see why upd_change_xxx_prop() > ever needs to cache removed properties for additions in skelta mode > (thus letting close_helper() send remove-prop). Looking at that now.
Scratch that... As Mike mentioned earlier, "1.7's RA interface still allows clients to request copyfrom information". And older (<1.7) clients do exactly that during updates because they predate the issue #3711[1] fixes and so get copyfrom-path and copyfrom-rev from the server. When using ra_serf, if we didn't send the prop deletions in close_helper, then an update could add a path created by a copy with properties that don't exist on the server (a bad thing). Paul [1] http://subversion.tigris.org/issues/show_bug.cgi?id=3711 http://svn.apache.org/viewvc?view=revision&revision=998183 http://svn.apache.org/viewvc?view=revision&revision=998193
