opendev/gerrit
Code Issues Proposed changes

Avoid "es" replaced by "es = Address"

Browse Source 
In some crazy cases the diff engine can select an odd delta
which causes us to present a pointless replace to the end-user.
Consider the patch:

 -Address[] replyToAddresses;
 +Address[] replyToAddresses = Address.unpack(list);

The MyersDiff engine anchored the second occurrence of "Address"
in the preimage with the third occurrence in the postimage.  We are
thus seeing the engine suggest an insert of the word "Address"
between the "o" and the "A" in the left side, and in a later edit
it supplied a replace of "es" with ".unpack(list)".

Previously we wound up showing the user that "es" was replaced by the
string "es = Address.unpack(list)".  By trying to join edits a second
time after the context shifting, and this time using only edits
that are strictly consecutive, we can see enough of the region at
once to shift it down and get the real delta, which is that insert.

This sort of scrubbing rule probably should go back into the
MyersDiff engine, or a wrapper around it in JGit.  But given how
slow JGit is right now at picking up patches I'm keeping it here
in Gerrit Code Review until the upstream MyersDiff can be tuned a
bit better for this case.

Change-Id: I86d84b7b8b8dad6a537653f5239b99232eac018c
Signed-off-by: Shawn O. Pearce <sop@google.com>
This commit is contained in:
Shawn O. Pearce
2010-03-02 21:47:47 -08:00
parent 58e99240e1
commit 3882948890
1 changed files with 20 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListCacheImpl.java
View File

@@ -286,6 +286,26 @@ public class PatchListCacheImpl implements PatchListCache {
int bb = c.getBeginB();
int be = c.getEndB();
// Sometimes the diff generator produces an INSERT or DELETE
// right up against a REPLACE, but we only find this after
// we've also played some shifting games on the prior edit.
// If that happened to us, coalesce them together so we can
// correct this mess for the user. If we don't we wind up
// with silly stuff like "es" -> "es = Addresses".
//
if (1 < j) {
Edit p = wordEdits.get(j - 1);
if (p.getEndA() == ab || p.getEndB() == bb) {
if (p.getEndA() == ab && p.getBeginA() < p.getEndA()) {
ab = p.getBeginA();
}
if (p.getEndB() == bb && p.getBeginB() < p.getEndB()) {
bb = p.getBeginB();
}
wordEdits.remove(--j);
}
}
// We sometimes collapsed an edit together in a strange way,
// such that the edges of each text is identical. Fix by
// by dropping out that incorrectly replaced region.