Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out there? (was Re: svn commit: r966822)

On Mon, Jan 24, 2011 at 5:01 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato <cmpilato_at_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
>
Received on 2011-01-25 17:40:23 CET