Skip to content

Commit

Permalink
Sort entries by segment when building a parent node to prevent unorde…
Browse files Browse the repository at this point in the history
…red directory structures.

When building a parent node from action inputs, the paths to the files are
sorted. These paths are then broken down into segments and a tree structure
is created from the segments.

Problem is, the segments at each level of the tree structure are not sorted
before they are added to the parent node. This can result in an unordered
directory tree.

For example, the sort order of this list of files
```
/foo/bar-client/bar-client_ijar.jar
/foo/bar/bar_ijar.jar
```

is maintained when it becomes a tree structure
```
foo ->
    bar-client ->
        bar-client_ijar.jar
    bar
        bar_ijar.jar
```
which is out of order.

Resolves: #5109

Closes #5110.

PiperOrigin-RevId: 195649710
  • Loading branch information
James Judd authored and Copybara-Service committed May 7, 2018
1 parent f286756 commit e6eaf25
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
Expand Down Expand Up @@ -123,6 +124,9 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(segment, child);
}

public static Comparator<ChildEntry> segmentOrder =
Comparator.comparing(ChildEntry::getSegment);
}

// Should only be called by the TreeNodeRepository.
Expand Down Expand Up @@ -344,6 +348,7 @@ private TreeNode buildParentNode(
}
}
}
Collections.sort(entries, TreeNode.ChildEntry.segmentOrder);
return interner.intern(new TreeNode(entries, null));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,20 @@ public void testDirectoryInput() throws Exception {
ActionInput fooH = ActionInputHelper.fromPath("/exec/root/a/foo/foo.h");
scratch.file("/exec/root/a/foo/foo.cc", "2");
ActionInput fooCc = ActionInputHelper.fromPath("/exec/root/a/foo/foo.cc");

Artifact bar = new Artifact(scratch.file("/exec/root/a/bar.txt"), rootDir);
TreeNodeRepository repo = createTestTreeNodeRepository();

TreeNode root = buildFromActionInputs(repo, foo, bar);
Artifact aClient = new Artifact(scratch.dir("/exec/root/a-client"), rootDir);
scratch.file("/exec/root/a-client/baz.txt", "3");
ActionInput baz = ActionInputHelper.fromPath("/exec/root/a-client/baz.txt");

TreeNode root = buildFromActionInputs(repo, foo, aClient, bar);
TreeNode aNode = root.getChildEntries().get(0).getChild();
TreeNode fooNode = aNode.getChildEntries().get(1).getChild(); // foo > bar in sort order!
TreeNode barNode = aNode.getChildEntries().get(0).getChild();
TreeNode aClientNode = root.getChildEntries().get(1).getChild(); // a-client > a in sort order
TreeNode bazNode = aClientNode.getChildEntries().get(0).getChild();

TreeNode fooHNode =
fooNode.getChildEntries().get(1).getChild(); // foo.h > foo.cc in sort order!
Expand All @@ -170,18 +177,30 @@ public void testDirectoryInput() throws Exception {
Digest fooDigest = repo.getMerkleDigest(fooNode);
Digest fooHDigest = repo.getMerkleDigest(fooHNode);
Digest fooCcDigest = repo.getMerkleDigest(fooCcNode);
Digest aClientDigest = repo.getMerkleDigest(aClientNode);
Digest bazDigest = repo.getMerkleDigest(bazNode);
Digest barDigest = repo.getMerkleDigest(barNode);
assertThat(digests)
.containsExactly(rootDigest, aDigest, barDigest, fooDigest, fooHDigest, fooCcDigest);
.containsExactly(
rootDigest,
aDigest,
barDigest,
fooDigest,
fooCcDigest,
fooHDigest,
aClientDigest,
bazDigest);

ArrayList<Directory> directories = new ArrayList<>();
ArrayList<ActionInput> actionInputs = new ArrayList<>();
repo.getDataFromDigests(digests, actionInputs, directories);
assertThat(actionInputs).containsExactly(bar, fooH, fooCc);
assertThat(directories).hasSize(3); // root, root/a and root/a/foo
assertThat(actionInputs).containsExactly(bar, fooH, fooCc, baz);
assertThat(directories).hasSize(4); // root, root/a, root/a/foo, and root/a-client
Directory rootDirectory = directories.get(0);
assertThat(rootDirectory.getDirectories(0).getName()).isEqualTo("a");
assertThat(rootDirectory.getDirectories(0).getDigest()).isEqualTo(aDigest);
assertThat(rootDirectory.getDirectories(1).getName()).isEqualTo("a-client");
assertThat(rootDirectory.getDirectories(1).getDigest()).isEqualTo(aClientDigest);
Directory aDirectory = directories.get(1);
assertThat(aDirectory.getFiles(0).getName()).isEqualTo("bar.txt");
assertThat(aDirectory.getFiles(0).getDigest()).isEqualTo(barDigest);
Expand All @@ -192,5 +211,8 @@ public void testDirectoryInput() throws Exception {
assertThat(fooDirectory.getFiles(0).getDigest()).isEqualTo(fooCcDigest);
assertThat(fooDirectory.getFiles(1).getName()).isEqualTo("foo.h");
assertThat(fooDirectory.getFiles(1).getDigest()).isEqualTo(fooHDigest);
Directory aClientDirectory = directories.get(3);
assertThat(aClientDirectory.getFiles(0).getName()).isEqualTo("baz.txt");
assertThat(aClientDirectory.getFiles(0).getDigest()).isEqualTo(bazDigest);
}
}

0 comments on commit e6eaf25

Please sign in to comment.