You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2015/06/09 18:50:10 UTC
Blame behaviour change in 1.9
Build 1.8 and 1.9 (I'm building 1684452). Run a 1.9 server pointing at
the 1.8 build directory; the server can be httpd or svnserve, but
svnserve is probably easiest:
../obj-1.9/subversion/svnserve/svnserve -Tdr ../obj-1.8/subversion/tests/cmdline
then run blame_tests.py 10:
cd ../obj-1.8/subversion/tests/cmdline
.../blame_tests.py 10 --url svn://localhost
and the test fails. We can see the cause by running blame on the
working copy of the failed test with the 1.9 server. The 1.8 client
gives the wrong output:
$ ../../svn/svn blame -g svn-test-work/working_copies/blame_tests-10/trunk/iota
2 jrandom This is the file 'iota'.
2 jrandom 'A' has changed a bit, with 'upsilon', and 'xi'.
while the 1.9 client gives the correct output:
$ svn blame -g svn-test-work/working_copies/blame_tests-10/trunk/iota
2 jrandom This is the file 'iota'.
G 11 jrandom 'A' has changed a bit, with 'upsilon', and 'xi'.
If I kill the 1.9 server and start a 1.8 server then both clients gives
the correct output. It appears the 1.9 server is sending data that the
1.8 client does not interpret correctly.
--
Philip
Re: Blame behaviour change in 1.9
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
> I think there's no doubt that the 1.9 server has to send no-op deltas
> the same way the 1.8 server did. Changing an API should not affect
> server behaviour as seen by the clients.
A simple testcase:
svnadmin create repo --compatible-version 1.8
svnmucc -mm -U file://`pwd`/repo mkdir A put repo/README.txt A/f
svnmucc -mm -U file://`pwd`/repo cp 1 A B
svnmucc -mm -U file://`pwd`/repo put repo/format A/f
svn co file://`pwd`/repo/B wc
svn merge ^/A wc
svn ci -mm wc
Using a 1.8 server both 1.8 and 1.9 clients show
$ svn blame -g http://localhost:8888/obj/repo/B/f
G 3 pm 5
Using a 1.9 server a 1.9 client shows the same but a 1.8 client shows:
$ svn blame -g http://localhost:8888/obj/repo/B/f
1 pm 5
The 1.8 server REPORT is:
<?xml version="1.0" encoding="utf-8"?>
<S:file-revs-report xmlns:S="svn:" xmlns:D="DAV:">
<S:file-rev path="/A/f" rev="1">
<S:rev-prop name="svn:date">2015-06-09T20:11:55.652556Z</S:rev-prop>
<S:rev-prop name="svn:log">m</S:rev-prop>
<S:rev-prop name="svn:author">pm</S:rev-prop>
<S:txdelta>U1ZOAAAAgXYDgXaAgXZUaGlzIGlzIGEgU3VidmVyc2lvbiByZXBvc2l0b3J5OyB1c2UgdGhlICdz
dm5hZG1pbicgYW5kICdzdm5sb29rJyAKdG9vbHMgdG8gZXhhbWluZSBpdC4gIERvIG5vdCBhZGQs
IGRlbGV0ZSwgb3IgbW9kaWZ5IGZpbGVzIGhlcmUgCnVubGVzcyB5b3Uga25vdyBob3cgdG8gYXZv
aWQgY29ycnVwdGluZyB0aGUgcmVwb3NpdG9yeS4KClZpc2l0IGh0dHA6Ly9zdWJ2ZXJzaW9uLmFw
YWNoZS5vcmcvIGZvciBtb3JlIGluZm9ybWF0aW9uLgo=
</S:txdelta></S:file-rev>
<S:file-rev path="/B/f" rev="2">
<S:rev-prop name="svn:date">2015-06-09T20:11:55.728662Z</S:rev-prop>
<S:rev-prop name="svn:log">m</S:rev-prop>
<S:rev-prop name="svn:author">pm</S:rev-prop>
</S:file-rev>
<S:file-rev path="/A/f" rev="3">
<S:rev-prop name="svn:date">2015-06-09T20:11:55.821288Z</S:rev-prop>
<S:rev-prop name="svn:log">m</S:rev-prop>
<S:rev-prop name="svn:author">pm</S:rev-prop>
<S:merged-revision/><S:txdelta>U1ZOAACBdgIBAoI1Cg==
</S:txdelta></S:file-rev>
<S:file-rev path="/B/f" rev="4">
<S:rev-prop name="svn:log">m</S:rev-prop>
<S:rev-prop name="svn:author">pm</S:rev-prop>
<S:rev-prop name="svn:date">2015-06-09T20:11:55.932638Z</S:rev-prop>
<S:txdelta>U1ZOAAACAgIAAgA=
</S:txdelta></S:file-rev>
</S:file-revs-report>
The 1.9 server report is:
<?xml version="1.0" encoding="utf-8"?>
<S:file-revs-report xmlns:S="svn:" xmlns:D="DAV:">
<S:file-rev path="/A/f" rev="1">
<S:rev-prop name="svn:log">m</S:rev-prop>
<S:rev-prop name="svn:author">pm</S:rev-prop>
<S:rev-prop name="svn:date">2015-06-09T20:11:55.652556Z</S:rev-prop>
<S:txdelta>U1ZOAAAAgXYDgXaAgXZUaGlzIGlzIGEgU3VidmVyc2lvbiByZXBvc2l0b3J5OyB1c2UgdGhlICdz
dm5hZG1pbicgYW5kICdzdm5sb29rJyAKdG9vbHMgdG8gZXhhbWluZSBpdC4gIERvIG5vdCBhZGQs
IGRlbGV0ZSwgb3IgbW9kaWZ5IGZpbGVzIGhlcmUgCnVubGVzcyB5b3Uga25vdyBob3cgdG8gYXZv
aWQgY29ycnVwdGluZyB0aGUgcmVwb3NpdG9yeS4KClZpc2l0IGh0dHA6Ly9zdWJ2ZXJzaW9uLmFw
YWNoZS5vcmcvIGZvciBtb3JlIGluZm9ybWF0aW9uLgo=
</S:txdelta></S:file-rev>
<S:file-rev path="/B/f" rev="2">
<S:rev-prop name="svn:log">m</S:rev-prop>
<S:rev-prop name="svn:author">pm</S:rev-prop>
<S:rev-prop name="svn:date">2015-06-09T20:11:55.728662Z</S:rev-prop>
</S:file-rev>
<S:file-rev path="/A/f" rev="3">
<S:rev-prop name="svn:log">m</S:rev-prop>
<S:rev-prop name="svn:author">pm</S:rev-prop>
<S:rev-prop name="svn:date">2015-06-09T20:11:55.821288Z</S:rev-prop>
<S:merged-revision/><S:txdelta>U1ZOAACBdgIBAoI1Cg==
</S:txdelta></S:file-rev>
<S:file-rev path="/B/f" rev="4">
<S:rev-prop name="svn:date">2015-06-09T20:11:55.932638Z</S:rev-prop>
<S:rev-prop name="svn:log">m</S:rev-prop>
<S:rev-prop name="svn:author">pm</S:rev-prop>
</S:file-rev>
</S:file-revs-report>
The significant difference is the missing txdeltas (ignore the hash
ordering of revprops).
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Blame behaviour change in 1.9
Posted by Branko Čibej <br...@wandisco.com>.
On 09.06.2015 21:31, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> The 1.9 server is sending the same revisions as the 1.8 server but some
>> of the revisions from the 1.9 server do not have a textdelta. Both the
>> 1.8 and 1.9 servers call svn_repos_get_file_revs2() which calls
>> send_path_revision(). Inside send_path_revision() the 1.8 server uses
>> svn_fs_contents_changed() to determine if there is a textdelta while 1.9
>> uses svn_fs_contents_different(). svn_fs_contents_different() does not
>> exist in 1.8 but is related to svn_fs_contents_changed() via the strict
>> flag. It seems the strict flag is supposed to make this work but it
>> doesn't appear to work in this case.
> There was discussion at the time of the change:
>
> http://svn.haxx.se/dev/archive-2014-02/0165.shtml
> http://svn.haxx.se/dev/archive-2014-02/0324.shtml
> http://svn.haxx.se/dev/archive-2014-03/0000.shtml
>
> 1.9 has a more accurate svn_fs_contents_changed() that doesn't report as
> many false positives. This means that some (or all?) of the -g revisions
> reported by svn_repos_get_file_revs2() do not include a textdelta that
> was included by 1.8. It appears at some level the 1.9 client interprets
> the revision without a textdelta as being the same as a revision with a
> textdelta that changes nothing, while the 1.8 client relies on the
> missing textdelta. If I force the 1.9 server to send textdeltas between
> identical texts the 1.8 client then behaves as the test expects.
I think there's no doubt that the 1.9 server has to send no-op deltas
the same way the 1.8 server did. Changing an API should not affect
server behaviour as seen by the clients.
We could optionally add a capability so that 1.9 clients can tell the
server not to send the unnecessary deltas. This would make sense only if
calculating the deltas is expensive.
-- Brane
Re: Blame behaviour change in 1.9
Posted by Bert Huijben <be...@qqmail.nl>.
+1.
Just noting that only -g appears to be affected, while I don’t know if the server side change also affects non -g. If non -g was optimized while it didn't affect clients we should try to keep that part.
Instead of announcing a client side capabilty that is transfered for every ra operation just passing a new flag would work as well… and it would allow keeping the old ra api behavior stable.
Bert
Sent from Surface
From: Branko Čibej
Sent: Friday, June 12, 2015 8:38 PM
To: dev@subversion.apache.org
On 12.06.2015 16:55, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Philip Martin <ph...@wandisco.com> writes:
>>
>>> 1.9 has a more accurate svn_fs_contents_changed() that doesn't report as
>>> many false positives. This means that some (or all?) of the -g revisions
>>> reported by svn_repos_get_file_revs2() do not include a textdelta that
>>> was included by 1.8. It appears at some level the 1.9 client interprets
>>> the revision without a textdelta as being the same as a revision with a
>>> textdelta that changes nothing, while the 1.8 client relies on the
>>> missing textdelta. If I force the 1.9 server to send textdeltas between
>>> identical texts the 1.8 client then behaves as the test expects.
>> Looking at r1573111
>>
>> /* Check if the contents *may* have changed. (Allow false positives,
>> for now, as the blame implementation currently depends on them.) */
>> /* Special case: In the first revision, we always provide a delta. */
>> if (sb->last_root)
>> - SVN_ERR(svn_fs_contents_changed2(&contents_changed, sb->last_root,
>> - sb->last_path, root, path_rev->path,
>> - FALSE, sb->iterpool));
>> + SVN_ERR(svn_fs_contents_different(&contents_changed, sb->last_root,
>> + sb->last_path, root, path_rev->path,
>> + sb->iterpool));
>> else
>> contents_changed = TRUE;
>>
>> That looks like an error since svn_fs_contents_different() passes TRUE
>> for strict, not FALSE. I think the intention was that r1573111 should
>> call svn_fs_contents_changed().
>>
>> However the implementation of root_vtable_t.contents_changed() has
>> changed in 1.9 and it no longer reports as many changes as it used to
>> report so even replacing the _different() call with a _changed() call
>> does not solve the problem. The only way I can see to fix the problem
>> is to simply ignore/remove the call altogether and to hard-code
>> contents_changed to TRUE.
> This revision is the client-side magic:
>
> r1570936 | stefan2 | 2014-02-22 22:27:06 +0000 (Sat, 22 Feb 2014) | 10 lines
>
> Fix the blame -g implemenation to no longer depend on false positives
> in the file contents change detection.
>
> A 1.8 server works with all clients whether they have the fix or not. A
> 1.9 server only works when the client has the fix.
>
> That client-side fix is only in 1.9, so at the moment, only 1.9 clients
> work with 1.9 servers.
IMO, the server-side "optimization" should either be reverted, or made
dependent on a — probably new — client capability that 1.9 clients (and
possibly patched 1.8 clients) can advertise.
-- Brane
Re: Blame behaviour change in 1.9
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Jun 15, 2015 at 1:20 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> My understanding of the situation is as follows. I'm not adding much
> to what Philip and Stefan have already diagnosed, I just want to say
> how I see it as well, and allow others to confirm it or correct me if
> necessary.
>
[snip insightful commentary that I agree with]
> Stefan Fuhrmann wrote:
> > Branko Čibej wrote:
> >> Philip Martin wrote:
> >>> Philip Martin writes:
> >>>> However the implementation of root_vtable_t.contents_changed() has
> >>>> changed in 1.9 and it no longer reports as many changes as it used to
> >>>> report so even replacing the _different() call with a _changed() call
> >>>> does not solve the problem. The only way I can see to fix the problem
> >>>> is to simply ignore/remove the call altogether and to hard-code
> >>>> contents_changed to TRUE.
>
> In other words, make svn_repos_get_file_revs2() give a text delta in
> all cases. If that makes the old blame code work again as well as it
> did before (I don't say 'correctly'), that is probably a good
> solution, as it doesn't sound like it would add much overhead or cause
> any other known problems. Of course that is bodging the behaviour to
> suit one particular caller that we know about. Other callers may exist
> that depend on some different behavioural details, but it's better to
> fix the breakage we know about.
>
Always sending a delta is reasonably safe - at least for blame.
In r1696478, I implemented a slightly more specific workaround:
It unconditionally sends the text delta whenever the path changes.
We are still deterministic and slightly more efficient than 1.8.
Future releases should have an option to either always send
deltas (empty or not) or to only send non-empty ones. Depending
on client capabilities, we can then pick either one.
-- Stefan^2.
Re: Blame behaviour change in 1.9
Posted by Julian Foad <ju...@btopenworld.com>.
My understanding of the situation is as follows. I'm not adding much
to what Philip and Stefan have already diagnosed, I just want to say
how I see it as well, and allow others to confirm it or correct me if
necessary.
First we need to be clear about 'change' and 'difference'. There has
been some confusion in the past due to the fact that the FS API can
record and report a 'change' even when there is no difference in
content, conveying the information that the two path-revs of interest
are represented by different node-revs. However, it is important to
notice that this distinction is only meaningful when comparing two
path-revs on the same line of history. That is the only case where
they can possibly be the same node-rev. When comparing path-revs on
different 'branches' (copies), which is the case in the 'blame'
scenario of interest, then the node-revs are necessarily different and
the only interesting question is whether the content differs.
The client-side 'blame -g' code was written in such a way that it
happened to rely on an undocumented detail of the report from
svn_repos_get_file_revs2(). Specifically, it assumed that the report
includes a text delta, even if an empty one, whenever it switches from
reporting a merged version to reporting a version from the main line
of history. If such a text delta were not present, the old blame code
would not 'notice' that the report had switched from a merged to a
main-line version, and would produce garbled output such as reporting
a revision number against a line that did not exist in that revision.
Note that there is no meaning attached to the presence of an empty
text delta, as opposed to no text delta. Both cases mean there is no
difference in the text, and therefore the blame should not report any
text as changed in that new revision. The client-side blame code just
happened to assume that the text delta syntactic element was always
present in the merged-to-mainline transition, and relied on its
presence to trigger some logic which could and should have been
triggered another way. (Changed in r1570936 to no longer rely on it.)
The report from svn_repos_get_file_revs2() happened to include such
deltas, because of an undocumented detail of svn_fs_contents_changed()
which happened to report 'changed' in such cases even when there was
no difference.
As Stefan says, the reason for the underlying change was determinism,
not optimization. The general false-positives behaviour of
svn_fs_contents_changed() was not useful to any sane caller. That is
why all callers were changed to use the new 'strict' function
svn_fs_contents_different() in r1572363 and r1573111.
Now we're going to have a behaviour change (yes, a bug) for old
clients accessing new servers if we don't do something to restore
exact compatibility at the client-server protocol level.
As Philip says:
> However the implementation of root_vtable_t.contents_changed() has
> changed in 1.9 and it no longer reports as many changes as it used to
(Clarification: it no longer reports as many false positive results
for non-changes as it used to.)
So just switching the call in svn_repos_get_file_revs2() back to using
the _changed() function doesn't solve the problem.
Would it be feasible to restore exactly (or enough of) the previous
behaviour of _changed()? I don't know. In theory, there may be other
direct callers of _changed() that depend on its exact false-positives
behaviour. However, we don't know of any besides get_file_revs.
Stefan Fuhrmann wrote:
> Branko Čibej wrote:
>> Philip Martin wrote:
>>> Philip Martin writes:
>>>> However the implementation of root_vtable_t.contents_changed() has
>>>> changed in 1.9 and it no longer reports as many changes as it used to
>>>> report so even replacing the _different() call with a _changed() call
>>>> does not solve the problem. The only way I can see to fix the problem
>>>> is to simply ignore/remove the call altogether and to hard-code
>>>> contents_changed to TRUE.
In other words, make svn_repos_get_file_revs2() give a text delta in
all cases. If that makes the old blame code work again as well as it
did before (I don't say 'correctly'), that is probably a good
solution, as it doesn't sound like it would add much overhead or cause
any other known problems. Of course that is bodging the behaviour to
suit one particular caller that we know about. Other callers may exist
that depend on some different behavioural details, but it's better to
fix the breakage we know about.
- Julian
Re: Blame behaviour change in 1.9
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jun 12, 2015 at 8:38 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 12.06.2015 16:55, Philip Martin wrote:
> > Philip Martin <ph...@wandisco.com> writes:
> >
> >> Philip Martin <ph...@wandisco.com> writes:
> >>
> >>> 1.9 has a more accurate svn_fs_contents_changed() that doesn't report
> as
> >>> many false positives. This means that some (or all?) of the -g
> revisions
> >>> reported by svn_repos_get_file_revs2() do not include a textdelta that
> >>> was included by 1.8. It appears at some level the 1.9 client
> interprets
> >>> the revision without a textdelta as being the same as a revision with a
> >>> textdelta that changes nothing, while the 1.8 client relies on the
> >>> missing textdelta. If I force the 1.9 server to send textdeltas
> between
> >>> identical texts the 1.8 client then behaves as the test expects.
> >> Looking at r1573111
> >>
> >> /* Check if the contents *may* have changed. (Allow false positives,
> >> for now, as the blame implementation currently depends on them.) */
> >> /* Special case: In the first revision, we always provide a delta. */
> >> if (sb->last_root)
> >> - SVN_ERR(svn_fs_contents_changed2(&contents_changed, sb->last_root,
> >> - sb->last_path, root,
> path_rev->path,
> >> - FALSE, sb->iterpool));
> >> + SVN_ERR(svn_fs_contents_different(&contents_changed, sb->last_root,
> >> + sb->last_path, root,
> path_rev->path,
> >> + sb->iterpool));
> >> else
> >> contents_changed = TRUE;
> >>
> >> That looks like an error since svn_fs_contents_different() passes TRUE
> >> for strict, not FALSE. I think the intention was that r1573111 should
> >> call svn_fs_contents_changed().
> >>
> >> However the implementation of root_vtable_t.contents_changed() has
> >> changed in 1.9 and it no longer reports as many changes as it used to
> >> report so even replacing the _different() call with a _changed() call
> >> does not solve the problem. The only way I can see to fix the problem
> >> is to simply ignore/remove the call altogether and to hard-code
> >> contents_changed to TRUE.
> > This revision is the client-side magic:
> >
> > r1570936 | stefan2 | 2014-02-22 22:27:06 +0000 (Sat, 22 Feb 2014) | 10
> lines
> >
> > Fix the blame -g implemenation to no longer depend on false positives
> > in the file contents change detection.
> >
> > A 1.8 server works with all clients whether they have the fix or not. A
> > 1.9 server only works when the client has the fix.
> >
> > That client-side fix is only in 1.9, so at the moment, only 1.9 clients
> > work with 1.9 servers.
>
> IMO, the server-side "optimization" should either be reverted, or made
> dependent on a — probably new — client capability that 1.9 clients (and
> possibly patched 1.8 clients) can advertise.
>
First of all, that commit is neither intended as an optimization
nor an "optimization". It is all about predictable output based
on actual API guarantees. Reverting to the unreliable code
(as in: different backends and different server versions may
give different answers) seems like the least desirable option.
I'll need some time to figure out whether the current response
for "-g" is actuall wrong, i.e. shows revisions that never held
the respective code snippets. Then we need to look into how
we can change the code to produce a better approximation of
what users may "-g" expect to return.
I also do have an idea of what an ideal -g result would look
like but that requires a different API and / or client-side logic.
But that would be a separate topic.
-- Stefan^2.
Re: Blame behaviour change in 1.9
Posted by Branko Čibej <br...@wandisco.com>.
On 12.06.2015 16:55, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Philip Martin <ph...@wandisco.com> writes:
>>
>>> 1.9 has a more accurate svn_fs_contents_changed() that doesn't report as
>>> many false positives. This means that some (or all?) of the -g revisions
>>> reported by svn_repos_get_file_revs2() do not include a textdelta that
>>> was included by 1.8. It appears at some level the 1.9 client interprets
>>> the revision without a textdelta as being the same as a revision with a
>>> textdelta that changes nothing, while the 1.8 client relies on the
>>> missing textdelta. If I force the 1.9 server to send textdeltas between
>>> identical texts the 1.8 client then behaves as the test expects.
>> Looking at r1573111
>>
>> /* Check if the contents *may* have changed. (Allow false positives,
>> for now, as the blame implementation currently depends on them.) */
>> /* Special case: In the first revision, we always provide a delta. */
>> if (sb->last_root)
>> - SVN_ERR(svn_fs_contents_changed2(&contents_changed, sb->last_root,
>> - sb->last_path, root, path_rev->path,
>> - FALSE, sb->iterpool));
>> + SVN_ERR(svn_fs_contents_different(&contents_changed, sb->last_root,
>> + sb->last_path, root, path_rev->path,
>> + sb->iterpool));
>> else
>> contents_changed = TRUE;
>>
>> That looks like an error since svn_fs_contents_different() passes TRUE
>> for strict, not FALSE. I think the intention was that r1573111 should
>> call svn_fs_contents_changed().
>>
>> However the implementation of root_vtable_t.contents_changed() has
>> changed in 1.9 and it no longer reports as many changes as it used to
>> report so even replacing the _different() call with a _changed() call
>> does not solve the problem. The only way I can see to fix the problem
>> is to simply ignore/remove the call altogether and to hard-code
>> contents_changed to TRUE.
> This revision is the client-side magic:
>
> r1570936 | stefan2 | 2014-02-22 22:27:06 +0000 (Sat, 22 Feb 2014) | 10 lines
>
> Fix the blame -g implemenation to no longer depend on false positives
> in the file contents change detection.
>
> A 1.8 server works with all clients whether they have the fix or not. A
> 1.9 server only works when the client has the fix.
>
> That client-side fix is only in 1.9, so at the moment, only 1.9 clients
> work with 1.9 servers.
IMO, the server-side "optimization" should either be reverted, or made
dependent on a — probably new — client capability that 1.9 clients (and
possibly patched 1.8 clients) can advertise.
-- Brane
Re: Blame behaviour change in 1.9
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Philip Martin <ph...@wandisco.com> writes:
>
>> 1.9 has a more accurate svn_fs_contents_changed() that doesn't report as
>> many false positives. This means that some (or all?) of the -g revisions
>> reported by svn_repos_get_file_revs2() do not include a textdelta that
>> was included by 1.8. It appears at some level the 1.9 client interprets
>> the revision without a textdelta as being the same as a revision with a
>> textdelta that changes nothing, while the 1.8 client relies on the
>> missing textdelta. If I force the 1.9 server to send textdeltas between
>> identical texts the 1.8 client then behaves as the test expects.
>
> Looking at r1573111
>
> /* Check if the contents *may* have changed. (Allow false positives,
> for now, as the blame implementation currently depends on them.) */
> /* Special case: In the first revision, we always provide a delta. */
> if (sb->last_root)
> - SVN_ERR(svn_fs_contents_changed2(&contents_changed, sb->last_root,
> - sb->last_path, root, path_rev->path,
> - FALSE, sb->iterpool));
> + SVN_ERR(svn_fs_contents_different(&contents_changed, sb->last_root,
> + sb->last_path, root, path_rev->path,
> + sb->iterpool));
> else
> contents_changed = TRUE;
>
> That looks like an error since svn_fs_contents_different() passes TRUE
> for strict, not FALSE. I think the intention was that r1573111 should
> call svn_fs_contents_changed().
>
> However the implementation of root_vtable_t.contents_changed() has
> changed in 1.9 and it no longer reports as many changes as it used to
> report so even replacing the _different() call with a _changed() call
> does not solve the problem. The only way I can see to fix the problem
> is to simply ignore/remove the call altogether and to hard-code
> contents_changed to TRUE.
This revision is the client-side magic:
r1570936 | stefan2 | 2014-02-22 22:27:06 +0000 (Sat, 22 Feb 2014) | 10 lines
Fix the blame -g implemenation to no longer depend on false positives
in the file contents change detection.
A 1.8 server works with all clients whether they have the fix or not. A
1.9 server only works when the client has the fix.
That client-side fix is only in 1.9, so at the moment, only 1.9 clients
work with 1.9 servers.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Blame behaviour change in 1.9
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> 1.9 has a more accurate svn_fs_contents_changed() that doesn't report as
> many false positives. This means that some (or all?) of the -g revisions
> reported by svn_repos_get_file_revs2() do not include a textdelta that
> was included by 1.8. It appears at some level the 1.9 client interprets
> the revision without a textdelta as being the same as a revision with a
> textdelta that changes nothing, while the 1.8 client relies on the
> missing textdelta. If I force the 1.9 server to send textdeltas between
> identical texts the 1.8 client then behaves as the test expects.
Looking at r1573111
/* Check if the contents *may* have changed. (Allow false positives,
for now, as the blame implementation currently depends on them.) */
/* Special case: In the first revision, we always provide a delta. */
if (sb->last_root)
- SVN_ERR(svn_fs_contents_changed2(&contents_changed, sb->last_root,
- sb->last_path, root, path_rev->path,
- FALSE, sb->iterpool));
+ SVN_ERR(svn_fs_contents_different(&contents_changed, sb->last_root,
+ sb->last_path, root, path_rev->path,
+ sb->iterpool));
else
contents_changed = TRUE;
That looks like an error since svn_fs_contents_different() passes TRUE
for strict, not FALSE. I think the intention was that r1573111 should
call svn_fs_contents_changed().
However the implementation of root_vtable_t.contents_changed() has
changed in 1.9 and it no longer reports as many changes as it used to
report so even replacing the _different() call with a _changed() call
does not solve the problem. The only way I can see to fix the problem
is to simply ignore/remove the call altogether and to hard-code
contents_changed to TRUE.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Blame behaviour change in 1.9
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> The 1.9 server is sending the same revisions as the 1.8 server but some
> of the revisions from the 1.9 server do not have a textdelta. Both the
> 1.8 and 1.9 servers call svn_repos_get_file_revs2() which calls
> send_path_revision(). Inside send_path_revision() the 1.8 server uses
> svn_fs_contents_changed() to determine if there is a textdelta while 1.9
> uses svn_fs_contents_different(). svn_fs_contents_different() does not
> exist in 1.8 but is related to svn_fs_contents_changed() via the strict
> flag. It seems the strict flag is supposed to make this work but it
> doesn't appear to work in this case.
There was discussion at the time of the change:
http://svn.haxx.se/dev/archive-2014-02/0165.shtml
http://svn.haxx.se/dev/archive-2014-02/0324.shtml
http://svn.haxx.se/dev/archive-2014-03/0000.shtml
1.9 has a more accurate svn_fs_contents_changed() that doesn't report as
many false positives. This means that some (or all?) of the -g revisions
reported by svn_repos_get_file_revs2() do not include a textdelta that
was included by 1.8. It appears at some level the 1.9 client interprets
the revision without a textdelta as being the same as a revision with a
textdelta that changes nothing, while the 1.8 client relies on the
missing textdelta. If I force the 1.9 server to send textdeltas between
identical texts the 1.8 client then behaves as the test expects.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Blame behaviour change in 1.9
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@codematters.co.uk> writes:
> $ ../../svn/svn blame -g svn-test-work/working_copies/blame_tests-10/trunk/iota
> 2 jrandom This is the file 'iota'.
> 2 jrandom 'A' has changed a bit, with 'upsilon', and 'xi'.
>
> while the 1.9 client gives the correct output:
>
> $ svn blame -g svn-test-work/working_copies/blame_tests-10/trunk/iota
> 2 jrandom This is the file 'iota'.
> G 11 jrandom 'A' has changed a bit, with 'upsilon', and 'xi'.
>
> If I kill the 1.9 server and start a 1.8 server then both clients gives
> the correct output. It appears the 1.9 server is sending data that the
> 1.8 client does not interpret correctly.
The 1.9 server is sending the same revisions as the 1.8 server but some
of the revisions from the 1.9 server do not have a textdelta. Both the
1.8 and 1.9 servers call svn_repos_get_file_revs2() which calls
send_path_revision(). Inside send_path_revision() the 1.8 server uses
svn_fs_contents_changed() to determine if there is a textdelta while 1.9
uses svn_fs_contents_different(). svn_fs_contents_different() does not
exist in 1.8 but is related to svn_fs_contents_changed() via the strict
flag. It seems the strict flag is supposed to make this work but it
doesn't appear to work in this case.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*