Skip to content

Commit c860a88

Browse files
committed
fix: always preserve the child order in BadVDiff
It seems like `VariationDiffParser` never generates a variation diff which triggers the bug in `BadVDiff` that doesn't preserve the child order. Note that it should be possible to generate such variation diffs with a tree matcher such as GumTree.
1 parent cbf504b commit c860a88

4 files changed

Lines changed: 69 additions & 11 deletions

File tree

src/main/java/org/variantsync/diffdetective/variation/diff/DiffNode.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,6 @@ public List<DiffNode<L>> removeChildren(Time time) {
360360

361361
/**
362362
* Removes all children from the given node and adds them as children to this node at the respective times.
363-
* The order of children is not stable because first all before children are transferred and then all after children.
364363
* The given node will have no children afterwards.
365364
* @param other The node whose children should be stolen.
366365
*/

src/main/java/org/variantsync/diffdetective/variation/diff/bad/BadVDiff.java

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -198,8 +198,18 @@ public static <L extends Label> BadVDiff<L> fromGood(VariationDiff<L> d) {
198198
record EdgeToConstruct<L extends Label>(
199199
VariationTreeNode<L> child,
200200
DiffNode<L> parent,
201-
Time t
202-
) {}
201+
Time t,
202+
int index
203+
) {
204+
public EdgeToConstruct(
205+
VariationTreeNode<L> child,
206+
DiffNode<L> parent,
207+
Time t,
208+
DiffNode<L> originalChild
209+
) {
210+
this(child, parent, t, parent.indexOfChild(originalChild, t));
211+
}
212+
}
203213

204214
final FromGoodNodeTranslation<L> nodeTranslation = new FromGoodNodeTranslation<>();
205215

@@ -259,7 +269,7 @@ record EdgeToConstruct<L extends Label>(
259269

260270
nodeTranslation.put(diffNode, time, self);
261271

262-
edgesToConstruct.add(new EdgeToConstruct<>(self, diffNode.getParent(time), time));
272+
edgesToConstruct.add(new EdgeToConstruct<>(self, diffNode.getParent(time), time, diffNode));
263273

264274
// further metadata to copy
265275
lines.put(self, dRange);
@@ -283,16 +293,17 @@ record EdgeToConstruct<L extends Label>(
283293
*/
284294
if (pbefore != null) {
285295
edgesToConstruct.add(new EdgeToConstruct<>(
286-
self, pbefore, BEFORE
296+
self, pbefore, BEFORE, diffNode
287297
));
288298
} else if (pafter != null) {
289299
edgesToConstruct.add(new EdgeToConstruct<>(
290-
self, pafter, AFTER
300+
self, pafter, AFTER, diffNode
291301
));
292302
}
293303
}
294304
});
295305

306+
edgesToConstruct.sort(Comparator.comparingInt(EdgeToConstruct::index));
296307
for (final EdgeToConstruct<L> e : edgesToConstruct) {
297308
nodeTranslation.get(e.parent, e.t).addChild(e.child);
298309
}
@@ -324,8 +335,18 @@ public VariationDiff<L> toGood() {
324335
record EdgeToConstruct<L extends Label>(
325336
DiffNode<L> child,
326337
VariationTreeNode<L> parent,
327-
Time time
328-
) {}
338+
Time time,
339+
int index
340+
) {
341+
public EdgeToConstruct(
342+
DiffNode<L> child,
343+
VariationTreeNode<L> parent,
344+
Time time,
345+
VariationTreeNode<L> originalChild
346+
) {
347+
this(child, parent, time, parent.indexOfChild(originalChild));
348+
}
349+
}
329350

330351
final List<EdgeToConstruct<L>> edgesToConstruct = new ArrayList<>();
331352
final Map<VariationTreeNode<L>, DiffNode<L>> nodeTranslation = new HashMap<>();
@@ -351,7 +372,7 @@ record EdgeToConstruct<L extends Label>(
351372

352373
nodeTranslation.put(vtnode, vGood);
353374
coloring.get(vtnode).forAllTimesOfExistence(
354-
t -> edgesToConstruct.add(new EdgeToConstruct<>(vGood, parent, t))
375+
t -> edgesToConstruct.add(new EdgeToConstruct<>(vGood, parent, t, vtnode))
355376
);
356377
} else {
357378
// v was cloned.
@@ -369,14 +390,15 @@ record EdgeToConstruct<L extends Label>(
369390
// invoke the callback for a single time:
370391
// BEFORE for REM and AFTER for ADD.
371392
vColor.forAllTimesOfExistence(
372-
t -> edgesToConstruct.add(new EdgeToConstruct<>(vGood, parent, t))
393+
t -> edgesToConstruct.add(new EdgeToConstruct<>(vGood, parent, t, vtnode))
373394
);
374395
badBuddyColor.forAllTimesOfExistence(
375-
t -> edgesToConstruct.add(new EdgeToConstruct<>(vGood, badBuddy.getParent(), t))
396+
t -> edgesToConstruct.add(new EdgeToConstruct<>(vGood, badBuddy.getParent(), t, badBuddy))
376397
);
377398
}
378399
});
379400

401+
edgesToConstruct.sort(Comparator.comparingInt(EdgeToConstruct::index));
380402
for (final EdgeToConstruct<L> e : edgesToConstruct) {
381403
nodeTranslation.get(e.parent()).addChild(e.child(), e.time());
382404
}

src/test/java/BadVDiffTest.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import org.junit.jupiter.api.Assertions;
2+
import org.junit.jupiter.api.Test;
23
import org.junit.jupiter.params.ParameterizedTest;
34
import org.junit.jupiter.params.provider.ValueSource;
45
import org.tinylog.Logger;
@@ -8,6 +9,12 @@
89
import org.variantsync.diffdetective.variation.diff.VariationDiff;
910
import org.variantsync.diffdetective.variation.diff.bad.BadVDiff;
1011
import org.variantsync.diffdetective.variation.diff.parse.VariationDiffParseOptions;
12+
import org.variantsync.diffdetective.variation.diff.serialize.GraphFormat;
13+
import org.variantsync.diffdetective.variation.diff.serialize.LineGraphImport;
14+
import org.variantsync.diffdetective.variation.diff.serialize.LineGraphImportOptions;
15+
import org.variantsync.diffdetective.variation.diff.serialize.edgeformat.ChildOrderEdgeFormat;
16+
import org.variantsync.diffdetective.variation.diff.serialize.nodeformat.FullNodeFormat;
17+
import org.variantsync.diffdetective.variation.diff.serialize.treeformat.IndexedTreeFormat;
1118

1219
import java.io.IOException;
1320
import java.nio.file.Path;
@@ -39,4 +46,21 @@ public void toGood_after_fromGood_idempotency(String filename) throws IOExceptio
3946

4047
Assertions.assertTrue(initialVDiff.isSameAs(goodDiff));
4148
}
49+
50+
@Test
51+
public void childOrderIsPreserved() throws IOException {
52+
final Path fileGraphFile = resDir.resolve("childOrder.lg");
53+
final var importOptions = new LineGraphImportOptions<DiffLinesLabel>(
54+
GraphFormat.VARIATION_DIFF,
55+
new IndexedTreeFormat(),
56+
new FullNodeFormat(),
57+
new ChildOrderEdgeFormat<>()
58+
);
59+
60+
final VariationDiff<DiffLinesLabel> initialVDiff = LineGraphImport.fromFile(fileGraphFile, importOptions).get(0);
61+
final var badDiff = BadVDiff.fromGood(initialVDiff);
62+
final var goodDiff = badDiff.toGood();
63+
64+
Assertions.assertTrue(initialVDiff.isSameAs(goodDiff));
65+
}
4266
}
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
t # 1
2+
v 16 NON;IF;(old: -1, diff: -1, new: -1);(old: -1, diff: -1, new: -1);True
3+
v 136 REM;IF;(old: 1, diff: 1, new: -1);(old: 5, diff: 7, new: -1);A;#if A
4+
v 264 REM;IF;(old: 2, diff: 3, new: -1);(old: 4, diff: 6, new: -1);X;#if X
5+
v 403 NON;ARTIFACT;(old: 3, diff: 5, new: 4);(old: 4, diff: 6, new: 5);;foo
6+
v 192 ADD;IF;(old: -1, diff: 2, new: 1);(old: -1, diff: 7, new: 5);B;#if B
7+
v 256 ADD;IF;(old: -1, diff: 3, new: 2);(old: -1, diff: 4, new: 3);X;#if X
8+
e 136 16 b;0,-1
9+
e 264 136 b;0,-1
10+
e 403 264 b;0,-1
11+
e 192 16 a;-1,0
12+
e 256 192 a;-1,0
13+
e 403 192 a;-1,1

0 commit comments

Comments
 (0)