You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pt...@gmail.com> on 2011/01/18 22:44:56 UTC
[PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out
there? (was Re: svn commit: r966822)
> On Fri, Jan 7, 2011 at 7:08 PM, Paul Burba <ptburba_at_gmail.com> wrote:
>> Mike suggested to me that the fix for this problem can be made in
>> mod_dav_svn, since the update reporter already knows which properties
>> have changed, *regardless* of the send-all mode, but it discards this
>> information if send-all=FALSE and instead instead sends the client a
>> 'fetch-props' response. As described previously in this thread, this
>> causes the client to fetch all the properties of the path, not just
>> the changes, and *all* the props are pushed through the editor
>> callbacks as if they were all actual changes (which again is the core
>> problem of issue #3657).
>>
>> I'm working on that approach,
>
> I had pushed off the fix-it-in-mod_dav_svn approach till 1.8 since I
> was going in circles with it, but recently found another real-world
> example where this issue causes spurious conflicts on directory
> properties, see
> http://subversion.tigris.org/issues/show_bug.cgi?id=3657#desc13.
>
> This issue has existed before 1.5 so it's nothing new, but given that
> I found another non-theoretical example and given that the presence of
> mergeinfo is only going to make this issue more common[1] I thought
> I'd take another look for 1.7.
>
> I still haven't gotten anywhere with the mod_dav_svn fix, so not much
> to talk about there, but I did commit the fix for the directory
> property conflicts in
> http://svn.apache.org/viewvc?view=revision&revision=1056565. The fix
> here is pretty much the same thing I did in
> http://svn.apache.org/viewvc?view=revision&revision=966822: Filter out
> the phony prop changes in the libsvn_client/repos_diff.c
> svn_delta_editor_t change_[dir|file]_prop() callback implementations.
>
> Just to be clear about one thing, when I committed r966822 Bert asked:
>
>>> (change_file_prop): Only stash true property differences in the file
>>> baton. The DAV RA providers may be sending us all the properties on
>>> a file, not just the differences.
>>
>> Shouldn't this be fixed in the ra layer implementations then?
>>
>> This would be just 'guessing' if it is a change or a complete set.
>
> Why not the RA layer? Two reasons. The first I explained earlier in
> this thread, it reopens issue #2048 'Primary connection (of parallel
> network connections used) in ra-dav diff/merge timeout unnecessarily.'
> I won't go over that again, but even if there is a way to avoid that,
> the RA layer is simply doing what we ask it when using the DAV
> providers. Specifically, the mod_dav_svn update report handler, when
> in 'skelta' mode[2] and describing changes to a path on which a
> property has changed, asks the client to later fetch all properties
> and figure out what has changed itself, i.e. the client asks the RA
> layer to give us *all* the properties, it obliges, so where is the
> problem as far as the RA layer is concerned?
>
> So this ultimately needs to be fixed on the server. Given that, I am
> leaving these libsvn_client filters in place so a 1.7 client will DTRT
> with older servers (or maybe 1.7 servers too, since as I said, I'm not
> having great success there).
>
> As to the 'guessing', that was true. We'd go through the motions of
> filtering over ra_local and ra_svn too. A pretty minor performance
> hit, but in r1056565 I tweaked the filtering so it only applies when
> using ra_serf and ra_neon.
>
> Paul
>
> [1] simply by increasing the potential pool of property changes
> incoming during a merge.
>
> [2] Which merges always are, regardless of whether we use ra_serf or ra_neon.
>
> Paul
Ok, after many false starts and detours I finally have a server-side
fix for this issue. Pursuing a suggestion by cmpilato, I made the
mod_dav_svn update report always send property changes, regardless of
whether the report is in skelta (send-all=false) mode or non-skelta
(send-all=true) mode.
In addition to the normal serf/neon trunk tests, I tested 1.5.10 (dev
build) and 1.6.16 (dev build) against this patched 1.7 server and
there were no unexpected [1] failures.
If any mod_dav_svn experts have time to take a look at this patch I'd
appreciate it.
Thanks,
Paul
[[[
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 md5-checksum, version-name, creationdate, and
creator-displayname. All four are already sent elsewhere and latter three
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.
]]]
[1] No unexpected failures, but there were these:
Serf 1.5.10 and 1.6.16:
FAIL: prop_tests.py 14: 'test binary property support'
This passes with Passes with r1058722
(http://svn.apache.org/viewvc?view=revision&revision=1058722)
backported (this is nominated for backport to both 1.5.x and 1.6.x).
Neon 1.5.10 and 1.6.16:
FAIL: log_tests.py 26: 'svn log -g target_with_bogus_mergeinfo'
This fails without the patch too, the failure is due to the way the
test is written, see
http://svn.apache.org/viewvc?view=revision&revision=926065, a change
that was (understandably) never backported.
Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out
there? (was Re: svn commit: r966822)
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Jan 25, 2011 at 11:27 AM, Paul Burba <pt...@gmail.com> wrote:
> On Mon, Jan 24, 2011 at 5:01 PM, Paul Burba <pt...@gmail.com> wrote:
>> On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato <cm...@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
Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out
there? (was Re: svn commit: r966822)
Posted by Paul Burba <pt...@gmail.com>.
On Mon, Jan 24, 2011 at 5:01 PM, Paul Burba <pt...@gmail.com> wrote:
> On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato <cm...@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
>
Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out
there? (was Re: svn commit: r966822)
Posted by Paul Burba <pt...@gmail.com>.
On Mon, Jan 24, 2011 at 5:01 PM, Paul Burba <pt...@gmail.com> wrote:
> On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato <cm...@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
>
Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts out
there? (was Re: svn commit: r966822)
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Jan 19, 2011 at 10:23 AM, C. Michael Pilato <cm...@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
Re: [PATCH] Server fix for issue #3657 - Any mod_dav_svn experts
out there? (was Re: svn commit: r966822)
Posted by "C. Michael Pilato" <cm...@collab.net>.
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 <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand