[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: Tue, 25 Jan 2011 13:19:43 -0500

On Tue, Jan 25, 2011 at 11:27 AM, Paul Burba <ptburba_at_gmail.com> wrote:
> 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.

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
Received on 2011-01-25 19:20:19 CET

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