fix: use the correct child order in line graph exports

When importing line graphs, we consider the order of the edge lines as
the definite child ordering even if we export the child order using
`ChildOrderEdgeFormat`. However, the previous algorithm blindly exports
all `DiffType.NON` edges disregarding the fact the other edges might
need to be exported first. As the order of edge lines acts as a diff
(before edges act as deletions and after edges as insertions), we need
to compute a line diff of the child orders at both times to know which
edges can be exported as existing at both times (unchanged).

Note that some edges will be exported as inserted and deleted instead of
unmodified although both the child and the parent are the same because
line diffs cannot encode moves. Consumers of line graphs should not
depend on the fact which edges are exported as unchanged because the
diffing algorithm needs to apply heuristics to select these edges.

Author: ibbem

src/main/java/org/variantsync/diffdetective/variation/diff/serialize/Format.java

@@ -1,11 +1,16 @@
 package org.variantsync.diffdetective.variation.diff.serialize;
 
+import org.eclipse.jgit.diff.DiffAlgorithm;
+import org.eclipse.jgit.diff.EditList;
+import org.eclipse.jgit.diff.Sequence;
+import org.eclipse.jgit.diff.SequenceComparator;
 import org.variantsync.diffdetective.variation.Label;
 import org.variantsync.diffdetective.variation.diff.DiffNode;
 import org.variantsync.diffdetective.variation.diff.VariationDiff;
 import org.variantsync.diffdetective.variation.diff.serialize.edgeformat.EdgeLabelFormat;
 import org.variantsync.diffdetective.variation.diff.serialize.nodeformat.DiffNodeLabelFormat;
 
+import java.util.List;
 import java.util.function.Consumer;
 
 import static org.variantsync.diffdetective.variation.diff.Time.AFTER;
@@ -69,20 +74,55 @@ public <La extends L> void forEachNode(VariationDiff<La> variationDiff, Consumer
      */
     public <La extends L> void forEachEdge(VariationDiff<La> variationDiff, Consumer<StyledEdge<La>> callback) {
         variationDiff.forAll((node) -> {
-            var beforeParent = node.getParent(BEFORE);
-            var afterParent = node.getParent(AFTER);
-
-            // Are both parent edges the same?
-            if (beforeParent != null && afterParent != null && beforeParent == afterParent) {
-                sortedEdgeWithLabel(node, node.getParent(BEFORE), StyledEdge.ALWAYS, callback);
-            } else {
-                if (beforeParent != null) {
-                    sortedEdgeWithLabel(node, node.getParent(BEFORE), StyledEdge.BEFORE, callback);
+            List<DiffNode<La>> beforeChildren = node.getChildOrder(BEFORE);
+            List<DiffNode<La>> afterChildren = node.getChildOrder(AFTER);
+
+            class ListSequence<E> extends Sequence {
+                public final List<E> list;
+
+                public ListSequence(List<E> list) {
+                    this.list = list;
+                }
+
+                @Override
+                public int size() {
+                    return list.size();
                 }
-                if (afterParent != null) {
-                    sortedEdgeWithLabel(node, node.getParent(AFTER), StyledEdge.AFTER, callback);
+            }
+
+            class ListSequenceComparator<E> extends SequenceComparator<ListSequence<E>> {
+                @Override
+                public boolean equals(ListSequence<E> sequence1, int index1, ListSequence<E> sequence2, int index2) {
+                    return sequence1.list.get(index1) == sequence2.list.get(index2);
+                }
+
+                @Override
+                public int hash(ListSequence<E> sequence, int index) {
+                    return System.identityHashCode(sequence.list.get(index));
                 }
             }
+
+            final EditList editList = DiffAlgorithm.getAlgorithm(DiffAlgorithm.SupportedAlgorithm.MYERS).diff(
+                new ListSequenceComparator<DiffNode<La>>(),
+                new ListSequence<>(beforeChildren),
+                new ListSequence<>(afterChildren)
+            );
+
+            int beforeIndex = 0;
+            for (var edit : editList) {
+                for (; beforeIndex < edit.getBeginA(); ++beforeIndex) {
+                    sortedEdgeWithLabel(beforeChildren.get(beforeIndex), node, StyledEdge.ALWAYS, callback);
+                }
+                for (; beforeIndex < edit.getEndA(); ++beforeIndex) {
+                    sortedEdgeWithLabel(beforeChildren.get(beforeIndex), node, StyledEdge.BEFORE, callback);
+                }
+                for (int afterIndex = edit.getBeginB(); afterIndex < edit.getEndB(); ++afterIndex) {
+                    sortedEdgeWithLabel(afterChildren.get(afterIndex), node, StyledEdge.AFTER, callback);
+                }
+            }
+            for (; beforeIndex < beforeChildren.size(); ++beforeIndex) {
+                sortedEdgeWithLabel(beforeChildren.get(beforeIndex), node, StyledEdge.ALWAYS, callback);
+            }
         });
     }
 

src/main/java/org/variantsync/diffdetective/variation/diff/serialize/edgeformat/ChildOrderEdgeFormat.java

@@ -15,7 +15,7 @@
  *
  * This index is encoded into decimal and delimited by a semicolon from the previous value.
  *
- * This format is mainly useful to equivalence of two {@link VariationDiff}s, for example in tests.
+ * This format is mainly useful to verify the equivalence of two {@link VariationDiff}s, for example in tests.
  *
  * @author Benjamin Moosherr
  */

src/test/java/LineGraphTest.java

@@ -1,24 +1,32 @@
 import org.apache.commons.io.IOUtils;
+import org.apache.commons.lang3.function.FailableConsumer;
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.variantsync.diffdetective.diff.result.DiffParseException;
+import org.variantsync.diffdetective.diff.text.DiffLineNumber;
+import org.variantsync.diffdetective.util.IO;
 import org.variantsync.diffdetective.variation.DiffLinesLabel;
+import org.variantsync.diffdetective.variation.diff.DiffNode;
+import org.variantsync.diffdetective.variation.diff.DiffType;
+import org.variantsync.diffdetective.variation.diff.Time;
 import org.variantsync.diffdetective.variation.diff.VariationDiff;
 import org.variantsync.diffdetective.variation.diff.serialize.*;
 import org.variantsync.diffdetective.variation.diff.serialize.edgeformat.DefaultEdgeLabelFormat;
 import org.variantsync.diffdetective.variation.diff.serialize.nodeformat.LabelOnlyDiffNodeFormat;
 import org.variantsync.diffdetective.variation.diff.serialize.treeformat.CommitDiffVariationDiffLabelFormat;
-import org.variantsync.diffdetective.util.IO;
-
-import static org.junit.jupiter.api.Assertions.fail;
+import org.variantsync.diffdetective.variation.diff.source.CommitDiffVariationDiffSource;
 
+import java.io.BufferedOutputStream;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.Paths;
 import java.util.List;
 import java.util.stream.Stream;
 
+import static org.junit.jupiter.api.Assertions.fail;
+
 /**
  * For testing the import of a line graph.
  */
@@ -34,7 +42,9 @@ public class LineGraphTest {
     );
 
     public static Stream<Path> testCases() throws IOException {
-        return Files.list(Paths.get("src/test/resources/line_graph"));
+        return Files
+            .list(Constants.RESOURCE_DIR.resolve("line_graph"))
+            .filter(file -> file.getFileName().toString().endsWith(".lg"));
     }
 
     /**
@@ -49,22 +59,26 @@ public void idempotentReadWrite(Path testFile) throws IOException {
         }
         assertConsistencyForAll(variationDiffs);
 
-        Path actualPath = testFile.getParent().resolve(testFile.getFileName().toString() + ".actual");
-        try (var output = IO.newBufferedOutputStream(actualPath)) {
-            LineGraphExport.toLineGraphFormat(variationDiffs, EXPORT_OPTIONS, output);
-        }
+        assertEqualsFile(testFile, output -> LineGraphExport.toLineGraphFormat(variationDiffs, EXPORT_OPTIONS, output));
+    }
 
-        try (
-                var expectedFile = Files.newBufferedReader(testFile);
-                var actualFile = Files.newBufferedReader(actualPath);
-        ) {
-            if (!IOUtils.contentEqualsIgnoreEOL(expectedFile, actualFile)) {
-                fail("The file " + testFile + " couldn't be exported or imported without modifications");
-            } else {
-                // Only keep output file on errors
-                Files.delete(actualPath);
-            }
-        }
+    @Test
+    public void testChildOrder() throws IOException, DiffParseException {
+        Path expectedPath = Constants.RESOURCE_DIR.resolve("line_graph").resolve("childOrder.lg");
+
+        // Note that this variation diff doesn't have a line diff representation because it
+        // represents a move (it could be parseable using a tree differ).
+        var root = DiffNode.createRoot(new DiffLinesLabel());
+        var A = DiffNode.createArtifact(DiffType.NON, new DiffLineNumber(1, 1, 2), new DiffLineNumber(1, 1, 2), new DiffLinesLabel(List.of(new DiffLinesLabel.Line("A", new DiffLineNumber(1, DiffLineNumber.InvalidLineNumber, 2)))));
+        var B = DiffNode.createArtifact(DiffType.NON, new DiffLineNumber(2, 2, 1), new DiffLineNumber(2, 2, 1), new DiffLinesLabel(List.of(new DiffLinesLabel.Line("B", new DiffLineNumber(2, DiffLineNumber.InvalidLineNumber, 1)))));
+        root.addChild(A, Time.BEFORE);
+        root.addChild(B, Time.BEFORE);
+        root.addChild(B, Time.AFTER);
+        root.addChild(A, Time.AFTER);
+
+        final var variationDiff = new VariationDiff<DiffLinesLabel>(root, new CommitDiffVariationDiffSource(Path.of("fileName"), "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"));
+
+        assertEqualsFile(expectedPath, output -> LineGraphExport.toLineGraphFormat(List.of(variationDiff), EXPORT_OPTIONS, output));
     }
 
     /**
@@ -78,4 +92,23 @@ private static void assertConsistencyForAll(final List<VariationDiff<DiffLinesLa
 //        }
         treeList.forEach(VariationDiff::assertConsistency);
     }
+
+    private static void assertEqualsFile(Path expectedPath, FailableConsumer<BufferedOutputStream, IOException> actualOutput) throws IOException {
+        Path actualPath = expectedPath.getParent().resolve(expectedPath.getFileName().toString() + ".actual");
+        try (var output = IO.newBufferedOutputStream(actualPath)) {
+            actualOutput.accept(output);
+        }
+
+        try (
+                var expectedFile = Files.newBufferedReader(expectedPath);
+                var actualFile = Files.newBufferedReader(actualPath);
+        ) {
+            if (!IOUtils.contentEqualsIgnoreEOL(expectedFile, actualFile)) {
+                fail("Expected the content of " + expectedPath + " but got the content of " + actualPath);
+            } else {
+                // Only keep output file on errors
+                Files.delete(actualPath);
+            }
+        }
+    }
 }

src/test/resources/diffs/differ/4a4c1db1b192e221d8e25460d6d1128d1bdd0c0d.lg

@@ -5,5 +5,5 @@ v 339 NON;ARTIFACT;(old: 3, diff: 4, new: 3);(old: 4, diff: 5, new: 4);;Another
 v 259 ADD;ARTIFACT;(old: -1, diff: 3, new: 2);(old: -1, diff: 4, new: 3);;Hallo
 e 147 16 ba;0,0
 e 203 16 b;1,-1
-e 339 16 ba;2,2
 e 259 16 a;-1,1
+e 339 16 ba;2,2

src/test/resources/diffs/differ/a032346092e47048becb36a7cb183b4739547370.lg

@@ -29,19 +29,19 @@ e 336 16 ba;3,3
 e 403 336 ba;0,0
 e 467 336 ba;1,1
 e 528 336 ba;2,2
-e 595 528 ba;0,0
-e 657 528 ba;1,1
-e 723 657 ba;0,0
 e 851 336 ba;3,3
 e 915 336 ba;4,4
 e 979 336 ba;5,5
 e 1043 336 ba;6,6
 e 1104 336 ba;7,7
-e 1299 1104 ba;0,0
-e 1363 1104 ba;1,1
 e 1619 336 ba;8,8
 e 1680 336 ba;9,9
-e 1811 1680 ba;0,0
 e 1939 336 ba;10,10
 e 2000 336 ba;11,11
+e 595 528 ba;0,0
+e 657 528 ba;1,1
+e 723 657 ba;0,0
+e 1299 1104 ba;0,0
+e 1363 1104 ba;1,1
+e 1811 1680 ba;0,0
 e 2067 2000 ba;0,0

src/test/resources/diffs/jpp/basic_jpp_expected.lg

@@ -20,16 +20,16 @@ e 131 16 a;-1,0
 e 195 16 a;-1,1
 e 259 16 a;-1,2
 e 320 16 a;-1,3
-e 387 320 a;-1,0
-e 451 320 a;-1,1
-e 515 320 a;-1,2
 e 643 16 a;-1,4
 e 707 16 a;-1,5
 e 768 16 a;-1,6
-e 835 768 a;-1,0
-e 899 768 a;-1,1
-e 963 768 a;-1,2
 e 1091 16 a;-1,7
 e 1155 16 a;-1,8
 e 1216 16 a;-1,9
+e 387 320 a;-1,0
+e 451 320 a;-1,1
+e 515 320 a;-1,2
+e 835 768 a;-1,0
+e 899 768 a;-1,1
+e 963 768 a;-1,2
 e 1283 1216 a;-1,0

src/test/resources/diffs/parser/02_expected.lg

@@ -3,6 +3,6 @@ v 136 REM;IF;(old: 1, diff: 1, new: -1);(old: 3, diff: 3, new: -1);A;#if A
 v 211 NON;ARTIFACT;(old: 2, diff: 2, new: 1);(old: 3, diff: 3, new: 2);;  Code
 v 323 ADD;ARTIFACT;(old: -1, diff: 4, new: 2);(old: -1, diff: 5, new: 3);;  // Hehe
 e 136 16 b;0,-1
-e 211 136 b;0,-1
 e 211 16 a;-1,0
 e 323 16 a;-1,1
+e 211 136 b;0,-1

src/test/resources/diffs/parser/03_expected.lg

@@ -5,8 +5,8 @@ v 595 NON;ARTIFACT;(old: 6, diff: 8, new: 5);(old: 7, diff: 9, new: 6);;Code
 v 128 ADD;IF;(old: -1, diff: 1, new: 1);(old: -1, diff: 10, new: 6);A | B | C;#if A \;  || B \;  || C \;  // Foo
 v 771 ADD;ARTIFACT;(old: -1, diff: 11, new: 7);(old: -1, diff: 12, new: 8);;  // Bar
 e 136 16 b;0,-1
+e 128 16 a;-1,0
+e 771 16 a;-1,1
 e 523 136 b;0,-1
 e 595 136 b;1,-1
 e 595 128 a;-1,0
-e 128 16 a;-1,0
-e 771 16 a;-1,1

src/test/resources/diffs/parser/08_expected.lg

@@ -3,6 +3,6 @@ v 136 REM;IF;(old: 1, diff: 1, new: -1);(old: 3, diff: 4, new: -1);defined(A);#i
 v 275 NON;ARTIFACT;(old: 2, diff: 3, new: 2);(old: 3, diff: 4, new: 3);;Code
 v 192 ADD;IF;(old: -1, diff: 2, new: 1);(old: -1, diff: 4, new: 3);defined(B);#ifdef B
 e 136 16 b;0,-1
+e 192 16 a;-1,0
 e 275 136 b;0,-1
 e 275 192 a;-1,0
-e 192 16 a;-1,0

src/test/resources/diffs/parser/09_expected.lg

@@ -4,8 +4,8 @@ v 211 NON;ARTIFACT;(old: 2, diff: 2, new: 2);(old: 3, diff: 3, new: 3);;Code 1
 v 339 NON;ARTIFACT;(old: 4, diff: 4, new: 3);(old: 5, diff: 5, new: 4);;Code 2
 v 128 ADD;IF;(old: -1, diff: 1, new: 1);(old: -1, diff: 5, new: 4);defined(A);#ifdef A
 e 136 16 b;0,-1
+e 339 16 b;1,-1
+e 128 16 a;-1,0
 e 211 136 b;0,-1
 e 211 128 a;-1,0
-e 339 16 b;1,-1
 e 339 128 a;-1,1
-e 128 16 a;-1,0