[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: C. Michael Pilato <cmpilato_at_collab.net>
Date: Wed, 19 Jan 2011 10:23:51 -0500

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

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.

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

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

Received on 2011-01-19 16:24:36 CET

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

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.