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
Received on 2015-06-15 13:21:02 CEST