On Fri, Feb 21, 2014 at 6:01 PM, Julian Foad <julianfoad_at_btopenworld.com>wrote:
> Stefan Fuhrmann wrote:
> > Julian Foad wrote:
> >> Stefan Fuhrmann wrote:
> >>> r1568600 uncovered an inconsistency in our API usage / interpretation
> >>> making blame -g tests fail for FSX.
> >>>
> >>> The starting point is svn_fs_contents_changed and svn_fs_props_changed.
> >>> FSFS and probably BDB implement those as "has there been an
> intermittent
> >>> change?" E.g. if the result of some merge yields equal content on both
> sides,
> >>> they will still be considered "changed". That is a violation of the
> API definition
> >>> basically asking "is the delta empty?". Even the FSX code is not fully
> consistent
> >>> with that, yet.
> >>
> >> BDB and FSFS and FSX all implement the comparison in exactly the same
> way:
> >> "does it have the same rep-key?".
> >
> > Well, not exactly. FSFS adds a "unifier" that tells different
> > instances of a shared representation apart (but uses the
> > same unifier when the rep does not change between
> > successive noderevs).
>
> Isn't libsvn_fs_base/fs.h:node_revision_t.data_key_uniquifier exactly the
> same thing in the BDB code? See the comparison function,
> svn_fs_base__things_different().
>
You are right. I wasn't aware that BDB supported rep
sharing as well and that the uniquifier stuff got added
there for the same reasons as in FSFS.
> >> This does not mean "is the content identical?", but it does not
> necessarily
> >> mean "has there been an intervening change to the content?" either.
> >
> > Well for FSFS, it does the latter.
>
> It's not clear to me that it does exactly the latter, that it says
> "changed" if and only if there has been an intervening change. In my next
> paragraph I speculated about a situation where that might not be the case.
> It seems to me that there may be cases where the rep-key can change even
> when the content does not change. Perhaps that is not in fact possible, but
> I can't see any clear reason why not.
>
My point here was only to say that FSFS will always
say "contents changed" when data was written to the
file in some transaction - even if that was an empty
delta or ended up to use an existing shared rep.
The actual content key is the physical storage location
in FSFS and that does not change without touching
the contents in some txn. If there were two instances
of the same contents (rep sharing off), it would be legal
to reference them alternatively e.g. between successive
noderevs along the same line of history. But there is no
code in SVN that could cause such a behavior.
As for changing the uniquifier, the situation is similar.
It is perfectly legal to simply "bump" it but there is no
code that would do it outside the rep sharing, which
kicks in only after new contents have been provided.
>> It only means "was the same representation instance chosen to represent
> this
> >> content?". For example, I think it might be possible for this to return
> >> "different" even if there has been no intervening change to the
> content, if
> >> the rep cache was disabled and re-enabled during a series of commits,
> although
> >> that depends on even more implementation details.
> >>
> >> One thing we can say for certain is that, despite the doc string, these
> APIs
> >> have always been implemented to return "the same" in some cases when the
> >> content is known to be the same, and return "different" in cases where
> the
> >> content might be different but might not be.
> >>
> >> Some of the callers use the call to optimize whether to fetch and
> compare the
> >> content. That's fine of course.
> >>
> >> Some other callers expose this behaviour to higher level APIs, where
> the caller
> >> might be expecting a definite answer to "Does the content differ?", and
> that's
> >> where the problems begin.
> >>
> >> Both semantics are valid, of course, so we might end up wanting to
> provide both.
> >
> > I agree. We might simply a allow_false_positives flag in
> > svn_fs_*_changed2. [...]
>
> It's not just a question of whether the function may report some
> non-changes along with the definite changes. The current blame code relies
> on a *specific* pattern of positive outputs which have a specific meaning
> which happens to be different from what the doc string says.
I think that is not the case: The blame code will
happily process empty changes as those don't
change any lines in the blame structure. The
problem was that empty "mainline" deltas still
had to be processed through blame's delta handler.
So, it basically relied on same contents being
reported as "different" when those were on different
paths and had changed with respect to their predecessor
on the respective path, i.e. a shared representation.
If any more (potential) deltas were reported, the
blame code will still work. IOW, the pattern is not
that specific.
> In that sense, the outputs are not "false" at all. It would be fair to say
> that it relies on a completely different API whose output is a superset of
> all content changes but does not contain any "random" false positives.
>
Here, I partly agree again. The expected false
positives are not entirely random. But they are
not well-defined either (except for the minimum).
Not sure if you meant the same.
My POV is that the blame code is / was broken
as it should not *rely* on any false positives.
I think we should take the liberty to make the
to-be-deprecated svn_fs_*_changed API behave
slightly different than 1.8: compare contents
by checksum and only return potential false
positives for nodes in a txn.
>>> However, our blame implementation relies on those false positives
> because
> >>> it tracks contents from revisions to be merged later separately from
> "mainline"
> >>> changes. When the merge finally happens but results in the same
> contents
> >>> as the previous branch revision, the delta handler must be called to
> pull in
> >>> the blame info from the branch. The latter no longer happens with the
> strict
> >>> interpretation of svn_fs_contents_changed.
> >>
> >> Without following the details, I think what you mean about merges is
> that the
> >> blame algorithm is trying to report an event whenever a merge happened,
> even
> >> if that merge did not result in a content change. The gist of it seems
> to be
> >> that the blame implementation assumes:
> >>
> >> * that a new content rep-key is assigned whenever a merge is
> committed, even
> >> if the content is unchanged, but not at other times when the content is
> >> unchanged; and
> >
> > Yep, that is the implementation detail leak that I'm concerned about.
> >
> >> * that svn_ra_get_file_revs2() will notify if and only if a new
> rep-key is
> >> assigned.
> >
> > No. False positives are fine. Always reporting an (empty) delta should
> > just work. In the blame case, that is.
>
> (As I understand it, the current implementation of 'blame' does make this
> assumption; I think your "No" isn't refuting that, but is meaning to say
> that get_file_revs2 doesn't notify if-and-only-if a new rep-key is
> assigned. And you're saying in theory it ought to be fine to report none or
> any or all of the empty deltas and a different 'blame' implementation
> should be able to work with that uncertainty.)
>
I objected to the if-and-only-if part; only the first
prong of that two-pronged condition is required.
> >> or something very much like that. Those assumptions look like the real
> problem here.
> >>
> >>
> >>> I see the following options:
> >>>
> >>> * Redefine svn_fs_contents_changed and svn_fs_props_changed to match
> >>> the current implementation. I'd rather not but that just a -0 from
> my side.
> >>
> >> We should deprecate them, and adjust their doc strings with a
> historical note
> >> about the discrepancy, and replace them with one or two new (pairs of)
> APIs:
> >>
> >> -- a quick optimization API -- the definitely/maybe question -- like
> the
> >> existing implementations but documented properly and named
> appropriately; and
> >>
> >> -- (perhaps) a definite content comparison API: "has the content
> changed?"
> >
> > I tend to prefer an extra flag parameter in a bumped version of the two
> > API functions as it makes existing API users aware of how the API has
> > been changed.
>
> Well, this is just a general API design style issue, but I think a name
> change is equally visible. I think a boolean parameter is appropriate where
> the caller is likely to choose from both variants at run time, but in this
> case I think callers will choose one or the other at design time, never at
> run time. I think we have far too many inappropriate boolean behaviour
> modifiers already in Subversion APIs. I think they make the call sites hard
> to read.
>
Fair point. I was thinking about revving svn_ra_get_file_revs2
as well with the same new option (callers can chose whether
need exact reports or want to process the delta anyway).
That would be a pass-through value for svn_fs_*_changed.
>> Why would a prop change be considered an empty delta?
> >
> > Blame does not process prop changes. However, prop changes are
> > somewhat frequent and would (often) result in an empty file contents
> > delta. So, there is a point in pre-checking for equal reps at all.
>
> OK, empty *text* delta.
>
Correct. Bert already pointed out that prop changes
may also be relevant to blame but are not handled
at all ATM.
> >>> * Make svn_ra_get_file_revs2 accept delta handlers (roughly) as today
> >>> but use the log information for that, i.e. whenever the content
> modified
> >>> flag is set for the respective file path. That would be sound
> approach
> >>> since we are in rev_hunt.c anyway. The info may already be available
> >>> (haven't checked yet).
> >>
> >> Not sure what you mean there, exactly.
> >
> > The "accept" bit: see above. The "modified" flag is part of the
> > changed paths lists we get from log info. It is basically what
> > FSFS tries to indicate with its false positives in *_changed:
> > Data has been written to this node in that rev - whether it
> > ended up as a true change or not.
>
> OK, I hear you, but it sounds just as arbitrary as the current behaviour.
> I can't see how it improves anything really.
>
I agree. By the time I wrote the original post,
I wasn't sure yet which route to take.
> >>> * Make the blame implementation update the mainline blame info even
> >>> if no new delta info came in but the previous callback wasn't for the
> >>> mainline.
> >>
> >> Sorry, I can't follow that one either without digging into the blame
> code.
> >
> > Well, according to Bert, the blame code is only an approximation
> > in the blame -g case. The critical sequence is:
> >
> > handle-blame(r2, "/some/branch", some-delta)
> > handle-blame(r3, "mainline", no change) // was a merge from
> "/some/branch"
> >
> > Today, FSFS reports a false positive different between r3 and r2
> > which also the blame delta handler to pull in the non-mainline
> > blame info into the mainline blame data (even as part of an
> > otherwise empty delta).
>
> Hmm. But, if I'm following correctly (a big "if"), in this sequence:
>
> handle-blame(r2, "/some/branch", some-delta)
> handle-blame(r3, "mainline", other-delta)
> handle-blame(r4, "mainline", no change) // was a merge from
> "/some/branch"
>
> the previous change *is* from the mainline, so this suggestion wouldn't
> recognize the merge.
>
You got it *almost* right. The thing is that the merge in r4
did not change any line vs. r3, so the blame does not need
to be updated (even if the same lines were changed on
the branch).
However, the corrected code now applies a (potentially
empty) delta in all mainline revs in -g mode. This is the
least intrusive option as it does not require any additional
bookkeeping.
>
> >> Clearly the blame implementation needs to be changed, but if we can
> start
> >> by determining what question it needs to know the answer to and define
> >> suitable APIs for answering it, that would be a good way to proceed.
> >
> > Bumping that svn_fs_*_changed API would certainly be a good
> > starting point. Since I'm currently on "working vacation", I may find
> > some time to make the blame code work with the "strict" change
> > semantics as well. That would fix the FSX release tests.
> >
> > As for svn_ra_get_file_revs2, I feel that it needs a better docstring.
> > We might want to bump it in the same way as the svn_fs_*_changed
> > API in case someone relied on undefined behaviour just like we
> > did with blame.
>
> +1 to that.
>
-- Stefan^2.
Received on 2014-02-23 23:34:09 CET