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. Paul > New Log: > [[[ > Server-side fix for issue #3657 'dav update report handler in skelta mode can > cause spurious conflicts'. > > This change has two parts that are loosely related: > > 1) It stops mod_dav_svn from ever sending the 'fetch-props' response to a > client, even when in skelta (i.e. send-all=false) mode. The server > already knows what properties have changed so now it simply sends them, > rather than telling the client to ask for them later. This prevents > the svn_delta_editor_t change_[file|dir]_prop callback from receiving > faux property changes when the client asks for *all* the properties. > See http://subversion.tigris.org/issues/show_bug.cgi?id=3657 for all the > gory details. > > 2) The comments in our code frequently make reference to 'modern' servers > and clients in the context of mod_dav_svn. Unfortunately there is no such > thing as a pre-modern server, they are all modern! The use of the term > 'modern' in our code comments predates Subversion 1.0.0, so the comments > are misleading and the special case code for pre-modern servers (or clients) > is unnecessary. The special-case code is interdependent with the changes in > #1 though, so these changes are being made in one commit, despite the fact > one could argue they are logically separate. > > * subversion/libsvn_ra_neon/fetch.c > > (report_baton_t, start_element): Remove comments about 'modern' servers and > replace as needed with discussion of whether we are in 'send-all' mode or > not. > > * subversion/mod_dav_svn/reports/update.c > > (struct item_baton_t): Remove changed_props, committed_rev, committed_date, > and last_author members, none of these are necessary. > > (close_helper): Simplify check for when we need to send remove-prop. > Remove code which sends 'fetch-props' to the client, we've already sent > them. Stop sending version-name, creationdate, and creator-displayname, > these were only needed by 'non-modern clients' which don't actually exist! > > (upd_change_xxx_prop): Whether we are in send-all mode or not, we have the > property changes already so send them now, rather than telling the client > to 'fetch-props' later. The only time we don't send the props is for > additions while not in send-all mode (i.e. skelta mode). In that case we > do the bare minimum necessary to not break old clients re issue #3711. > ]]] > > Paul >