Greg Stein <gstein@lyra.org> writes:
> That's an awfully long comment. I think you can describe the problem with
> much shorter text. There are two situations here:
The comment was longer because it was saying something different. The
shorter text you write below comes to a different initial conclusion,
and thus avoids the need for the entire second half of my text :-).
> * svn_wc_text_modified_p() says the wc is unmodified
>
> - it could be that the WC is a copy of a corrupted text base. thus, we
> still need to compute a checksum of the text base.
>
> * svn_wc_text_modified_p() says the wc is modified
>
> - the text base might still be corrupted, so we have to check, to
> ensure we don't copy corrupted stuff out to the WC.
>
> Thus, we always need to compute the checksum.
Yah, we don't strictly "need" to, even for revert. See my recent mail
in the thread
"svn diff doesn't verify MD5 sums (Related to issues #1774 and #1177)"
But yes, it's nice to (obviously agree, since I committed r8897 :-) ).
> The second issue is whether it is possible to collapse the checksum into
> text_modified's byte-by-byte comparison. That *should* be doable.
>
> IOW, the net result could be:
>
> * the files appear the same at the surface, so a byte-by-byte comparison
> is made. at the same time, a checksum can be computed on the source.
>
> note that early exit during the byte-by-byte is no longer allowed.
>
> * the files differ at the surface, so text_modified exits early. a
> normal checksum is run on the text base.
>
> So... I think you *can* optimize the behavior here. Rather than two passes
> over the files (quite possible now), you can drop it to a single pass in
> all cases.
Hmmm. If we're willing to lose the "early out on first difference" in
svn_wc_text_modified_p()'s byte-by-byte check, then yes, the above
works.
Other operations, like status, use svn_wc_text_modified_p(), but
really shouldn't be examining every byte if they can help it (it would
slow them down). That's why I didn't want to remove the efficient
behavior. Yet adding a flag to make it optional meant an API change,
and you know where that leads...
...But, we're saved by the existence of the 'force_comparison' flag to
svn_wc_text_modified_p(), which can be compatibly overloaded with an
additional interpretation:
"Not only skip early outs, but also run verify the checksum of the
text base (implying scan all bytes unconditionally)."
This gives us the optimized behavior when we want it, but the ability
to indicate the careful behavior when appropriate -- such as in
'revert'.
So okay, I'll buy what you're selling :-).
Maybe I can make the change on the flight home tomorrow...
Thanks for the review, as always!
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Mar 6 02:00:03 2004