[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Paul Burba <ptburba_at_gmail.com>
Date: Mon, 24 Jan 2011 17:01:49 -0500

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).

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-24 23:02:26 CET

This is an archived mail posted to the Subversion Dev mailing list.