4.95 of 5: Merger changes

* Merge conflicts are now bright red warnings, as the previous merger was simultaneously stricter and looser than expected.
* Legacy resource rules are now detected and cause bright red warnings.
* Merge conflicts will test for equality either of file contents or parsed xml

--
MOS_MIGRATED_REVID=121510152

4 files changed, 394 insertions, 202 deletions

diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java b/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
index 965579ef30..3b39205ad8 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
@@ -14,39 +14,189 @@
 package com.google.devtools.build.android;
 
 import com.google.common.base.Joiner;
+import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.devtools.build.android.ParsedAndroidData.ParsedAndroidDataBuildingPathWalker;
 
 import com.android.ide.common.res2.MergingException;
 
+import java.io.BufferedInputStream;
 import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.attribute.BasicFileAttributeView;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.logging.Logger;
 
 /**
  * Handles the Merging of ParsedAndroidData.
  */
 public class AndroidDataMerger {
 
+  private static final Logger logger = Logger.getLogger(AndroidDataMerger.class.getCanonicalName());
+
+  /** Interface for comparing paths. */
+  interface SourceChecker {
+    boolean checkEquality(Path one, Path two) throws IOException;
+  }
+
+  /** Compares two paths by the contents of the files. */
+  static class ContentComparingChecker implements SourceChecker {
+
+    static SourceChecker create() {
+      return new ContentComparingChecker();
+    }
+
+    @Override
+    public boolean checkEquality(Path one, Path two) throws IOException {
+      // TODO(corysmith): Is there a filesystem hash we can use?
+      if (getFileSize(one) != getFileSize(two)) {
+        return false;
+      }
+      try (final InputStream oneStream = new BufferedInputStream(Files.newInputStream(one));
+          final InputStream twoStream = new BufferedInputStream(Files.newInputStream(two))) {
+        int bytesRead = 0;
+        while (true) {
+          int oneByte = oneStream.read();
+          int twoByte = twoStream.read();
+          bytesRead++;
+          if (oneByte == -1 || twoByte == -1) {
+            if (oneByte == twoByte) {
+              return true;
+            } else {
+              // getFileSize did not return correct size.
+              logger.severe(
+                  String.format(
+                      "Filesystem size of %s (%s) or %s (%s) is inconsistant with bytes read %s.",
+                      one,
+                      getFileSize(one),
+                      two,
+                      getFileSize(two),
+                      bytesRead));
+              return false;
+            }
+          }
+          if (oneByte != twoByte) {
+            return false;
+          }
+        }
+      }
+    }
+
+    private long getFileSize(Path path) throws IOException {
+      return Files.getFileAttributeView(path, BasicFileAttributeView.class).readAttributes().size();
+    }
+  }
+
+  static class NoopSourceChecker implements SourceChecker {
+    static SourceChecker create() {
+      return new NoopSourceChecker();
+    }
+
+    @Override
+    public boolean checkEquality(Path one, Path two) {
+      return false;
+    }
+  }
+
+  private final SourceChecker deDuplicator;
+
+  /** Creates a merger with no path deduplication. */
+  public static AndroidDataMerger create() {
+    return new AndroidDataMerger(NoopSourceChecker.create());
+  }
+
+  /** Creates a merger with a custom deduplicator. */
+  public static AndroidDataMerger create(SourceChecker deDuplicator) {
+    return new AndroidDataMerger(deDuplicator);
+  }
+
+  /** Creates a merger with a file contents hashing deduplicator. */
+  public static AndroidDataMerger createWithPathDeduplictor() {
+    return create(ContentComparingChecker.create());
+  }
+
+  private AndroidDataMerger(SourceChecker deDuplicator) {
+    this.deDuplicator = deDuplicator;
+  }
+
   /**
-   * Merges DataResources into an UnwrittenMergedAndroidData.
+   * Merges a list of {@link DependencyAndroidData} with a {@link UnvalidatedAndroidData}.
    *
+   * @see AndroidDataMerger#merge(ParsedAndroidData, ParsedAndroidData, UnvalidatedAndroidData,
+   * boolean) for details.
+   */
+  UnwrittenMergedAndroidData merge(
+      List<DependencyAndroidData> transitive,
+      List<DependencyAndroidData> direct,
+      UnvalidatedAndroidData primary,
+      boolean allowPrimaryOverrideAll)
+      throws IOException, MergingException {
+    Stopwatch timer = Stopwatch.createStarted();
+    try {
+      final ParsedAndroidData.Builder directBuilder = ParsedAndroidData.Builder.newBuilder();
+      final ParsedAndroidData.Builder transitiveBuilder = ParsedAndroidData.Builder.newBuilder();
+      final AndroidDataSerializer serializer = AndroidDataSerializer.create();
+      for (final DependencyAndroidData dependency : direct) {
+        parseDependencyData(directBuilder, serializer, dependency);
+      }
+      for (final DependencyAndroidData dependency : transitive) {
+        parseDependencyData(transitiveBuilder, serializer, dependency);
+      }
+      logger.fine(
+          String.format("Merged dependencies read in %sms", timer.elapsed(TimeUnit.MILLISECONDS)));
+      timer.reset().start();
+      return merge(
+          transitiveBuilder.build(), directBuilder.build(), primary, allowPrimaryOverrideAll);
+    } finally {
+      logger.fine(String.format("Resources merged in %sms", timer.elapsed(TimeUnit.MILLISECONDS)));
+    }
+  }
+
+  private void parseDependencyData(
+      final ParsedAndroidData.Builder parsedDataBuilder,
+      final AndroidDataSerializer serializer,
+      final DependencyAndroidData dependency)
+      throws IOException, MergingException {
+    try {
+      dependency.deserialize(serializer, parsedDataBuilder.consumers());
+    } catch (DeserializationException e) {
+      if (!e.isLegacy()) {
+        throw new MergingException(e);
+      }
+      //TODO(corysmith): List the offending a target here.
+      logger.warning(
+          String.format(
+              "\u001B[31mDEPRECATION:\u001B[0m Legacy resources used for %s",
+              dependency.getManifest()));
+      // Legacy android resources -- treat them as direct dependencies.
+      dependency.walk(ParsedAndroidDataBuildingPathWalker.create(parsedDataBuilder));
+    }
+  }
+
+  /**
+   * Merges DataResources into an UnwrittenMergedAndroidData.
+   * <p>
    * This method has two basic states, library and binary. These are distinguished by
-   * allowPrimaryOverrideAll, which allows the primary data to overwrite any value in the closure,
-   * a trait associated with binaries, as a binary is a leaf node. The other semantics are
-   * slightly more complicated: a given resource can be overwritten only if it resides in the
-   * direct dependencies of primary data. This forces an explicit simple priority for each resource,
+   * allowPrimaryOverrideAll, which allows the primary data to overwrite any value in the closure, a
+   * trait associated with binaries, as a binary is a leaf node. The other semantics are slightly
+   * more complicated: a given resource can be overwritten only if it resides in the direct
+   * dependencies of primary data. This forces an explicit simple priority for each resource,
    * instead of the more subtle semantics of multiple layers of libraries with potential overwrites.
+   * <p>
+   * The UnwrittenMergedAndroidData contains only one of each DataKey in both the direct and
+   * transitive closure.
    *
-   * The UnwrittenMergedAndroidData contains only one of each DataKey in both the
-   * direct and transitive closure.
-   *
-   * The merge semantics are as follows:
+   * The merge semantics are as follows: <pre>
    *   Key:
    *     A(): package A
    *     A(foo): package A with resource symbol foo
@@ -76,115 +226,124 @@ public class AndroidDataMerger {
    *     A() -> B(foo),C(foo) -> D() == Conflict
    *     A(foo),B(foo) -> C() -> D(foo) == Valid
    *     A() -> B(foo),C(foo) -> D(foo) == Valid
+   * </pre>
    *
    * @param transitive The transitive dependencies to merge.
    * @param direct The direct dependencies to merge.
    * @param primaryData The primary data to merge against.
    * @param allowPrimaryOverrideAll Boolean that indicates if the primary data will be considered
-   *                                the ultimate source of truth, provided it doesn't conflict
-   *                                with itself.
-   * @return An UnwrittenMergedAndroidData, containing DataResource objects that can be written
-   * to disk for aapt processing or serialized for future merge passes.
+   *    the ultimate source of truth, provided it doesn't conflict with itself.
+   * @return An UnwrittenMergedAndroidData, containing DataResource objects that can be written to
+   *    disk for aapt processing or serialized for future merge passes.
    * @throws MergingException if there are merge conflicts or issues with parsing resources from
-   * Primary.
-   * @throws IOException if there are issues with reading resources.
+   *    Primary.
    */
   UnwrittenMergedAndroidData merge(
       ParsedAndroidData transitive,
       ParsedAndroidData direct,
       UnvalidatedAndroidData primaryData,
       boolean allowPrimaryOverrideAll)
-      throws MergingException, IOException {
-
-    // Extract the primary resources.
-    ParsedAndroidData primary = ParsedAndroidData.from(primaryData);
+      throws MergingException {
 
-    Map<DataKey, DataResource> overwritableDeps = new HashMap<>();
-    Map<DataKey, DataAsset> assets = new HashMap<>();
+    try {
+      // Extract the primary resources.
+      ParsedAndroidData primary = ParsedAndroidData.from(primaryData);
 
-    Set<MergeConflict> conflicts = new HashSet<>();
-    conflicts.addAll(primary.conflicts());
+      Map<DataKey, DataResource> overwritableDeps = new HashMap<>();
+      Map<DataKey, DataAsset> assets = new HashMap<>();
 
-    for (MergeConflict conflict : Iterables.concat(direct.conflicts(), transitive.conflicts())) {
-      if (allowPrimaryOverrideAll
-          && (primary.containsOverwritable(conflict.dataKey())
-              || primary.containsAsset(conflict.dataKey()))) {
-        continue;
+      Set<MergeConflict> conflicts = new HashSet<>();
+      conflicts.addAll(primary.conflicts());
+      for (MergeConflict conflict : Iterables.concat(direct.conflicts(), transitive.conflicts())) {
+        if (allowPrimaryOverrideAll
+            && (primary.containsOverwritable(conflict.dataKey())
+                || primary.containsAsset(conflict.dataKey()))) {
+          continue;
+        }
+        conflicts.add(conflict);
       }
-      conflicts.add(conflict);
-    }
 
-    // resources
-    for (Map.Entry<DataKey, DataResource> entry : direct.iterateOverwritableEntries()) {
-      // Direct dependencies are simply overwritten, no conflict.
-      if (!primary.containsOverwritable(entry.getKey())) {
-        overwritableDeps.put(entry.getKey(), entry.getValue());
-      }
-    }
-    for (Map.Entry<DataKey, DataResource> entry : transitive.iterateOverwritableEntries()) {
-      // If the primary is considered to be intentional (usually at the binary level),
-      // skip.
-      if (primary.containsOverwritable(entry.getKey()) && allowPrimaryOverrideAll) {
-        continue;
+      // resources
+      for (Map.Entry<DataKey, DataResource> entry : direct.iterateOverwritableEntries()) {
+        // Direct dependencies are simply overwritten, no conflict.
+        if (!primary.containsOverwritable(entry.getKey())) {
+          overwritableDeps.put(entry.getKey(), entry.getValue());
+        }
       }
-      // If a transitive value is in the direct map report a conflict, as it is commonly
-      // unintentional.
-      if (direct.containsOverwritable(entry.getKey())) {
-        conflicts.add(direct.foundResourceConflict(entry.getKey(), entry.getValue()));
-      } else if (primary.containsOverwritable(entry.getKey())) {
-        // If overwriting a transitive value with a primary map, assume it's an unintentional
-        // override, unless allowPrimaryOverrideAll is set. At which point, this code path
-        // should not be reached.
-        conflicts.add(primary.foundResourceConflict(entry.getKey(), entry.getValue()));
-      } else {
-        // If it's in none of the of sources, add it.
-        overwritableDeps.put(entry.getKey(), entry.getValue());
+      for (Map.Entry<DataKey, DataResource> entry : transitive.iterateOverwritableEntries()) {
+        // If the primary is considered to be intentional (usually at the binary level),
+        // skip.
+        if (primary.containsOverwritable(entry.getKey()) && allowPrimaryOverrideAll) {
+          continue;
+        }
+        // If a transitive value is in the direct map report a conflict, as it is commonly
+        // unintentional.
+        if (direct.containsOverwritable(entry.getKey())) {
+          conflicts.add(direct.foundResourceConflict(entry.getKey(), entry.getValue()));
+        } else if (primary.containsOverwritable(entry.getKey())) {
+          // If overwriting a transitive value with a primary map, assume it's an unintentional
+          // override, unless allowPrimaryOverrideAll is set. At which point, this code path
+          // should not be reached.
+          conflicts.add(primary.foundResourceConflict(entry.getKey(), entry.getValue()));
+        } else {
+          // If it's in none of the of sources, add it.
+          overwritableDeps.put(entry.getKey(), entry.getValue());
+        }
       }
-    }
 
-    // assets
-    for (Map.Entry<DataKey, DataAsset> entry : direct.iterateAssetEntries()) {
-      // Direct dependencies are simply overwritten, no conflict.
-      if (!primary.containsAsset(entry.getKey())) {
-        assets.put(entry.getKey(), entry.getValue());
-      }
-    }
-    for (Map.Entry<DataKey, DataAsset> entry : transitive.iterateAssetEntries()) {
-      // If the primary is considered to be intentional (usually at the binary level),
-      // skip.
-      if (primary.containsAsset(entry.getKey()) && allowPrimaryOverrideAll) {
-        continue;
+      // assets
+      for (Map.Entry<DataKey, DataAsset> entry : direct.iterateAssetEntries()) {
+        // Direct dependencies are simply overwritten, no conflict.
+        if (!primary.containsAsset(entry.getKey())) {
+          assets.put(entry.getKey(), entry.getValue());
+        }
       }
-      // If a transitive value is in the direct map report a conflict, as it is commonly
-      // unintentional.
-      if (direct.containsAsset(entry.getKey())) {
-        conflicts.add(direct.foundAssetConflict(entry.getKey(), entry.getValue()));
-      } else if (primary.containsAsset(entry.getKey())) {
-        // If overwriting a transitive value with a primary map, assume it's an unintentional
-        // override, unless allowPrimaryOverrideAll is set. At which point, this code path
-        // should not be reached.
-        conflicts.add(primary.foundAssetConflict(entry.getKey(), entry.getValue()));
-      } else {
-        // If it's in none of the of sources, add it.
-        assets.put(entry.getKey(), entry.getValue());
+      for (Map.Entry<DataKey, DataAsset> entry : transitive.iterateAssetEntries()) {
+        // If the primary is considered to be intentional (usually at the binary level),
+        // skip.
+        if (primary.containsAsset(entry.getKey()) && allowPrimaryOverrideAll) {
+          continue;
+        }
+        // If a transitive value is in the direct map report a conflict, as it is commonly
+        // unintentional.
+        if (direct.containsAsset(entry.getKey())) {
+          conflicts.add(direct.foundAssetConflict(entry.getKey(), entry.getValue()));
+        } else if (primary.containsAsset(entry.getKey())) {
+          // If overwriting a transitive value with a primary map, assume it's an unintentional
+          // override, unless allowPrimaryOverrideAll is set. At which point, this code path
+          // should not be reached.
+          conflicts.add(primary.foundAssetConflict(entry.getKey(), entry.getValue()));
+        } else {
+          // If it's in none of the of sources, add it.
+          assets.put(entry.getKey(), entry.getValue());
+        }
       }
-    }
 
-    if (!conflicts.isEmpty()) {
-      List<String> messages = new ArrayList<>();
-      for (MergeConflict conflict : conflicts) {
-        messages.add(conflict.toConflictMessage());
+      if (!conflicts.isEmpty()) {
+        List<String> messages = new ArrayList<>();
+        for (MergeConflict conflict : conflicts) {
+          if (!conflict.first().equals(conflict.second())
+              && !deDuplicator.checkEquality(
+                  conflict.first().source(), conflict.second().source())) {
+            messages.add(conflict.toConflictMessage());
+          }
+        }
+        if (!messages.isEmpty()) {
+          // TODO(corysmith): Turn these into errors.
+          logger.warning(Joiner.on("").join(messages));
+        }
       }
-      throw new MergingException(Joiner.on("\n").join(messages));
-    }
 
-    return UnwrittenMergedAndroidData.of(
-        primaryData.getManifest(),
-        primary,
-        ParsedAndroidData.of(
-            ImmutableSet.<MergeConflict>of(),
-            ImmutableMap.copyOf(overwritableDeps),
-            direct.mergeNonOverwritable(transitive),
-            ImmutableMap.copyOf(assets)));
+      return UnwrittenMergedAndroidData.of(
+          primaryData.getManifest(),
+          primary,
+          ParsedAndroidData.of(
+              ImmutableSet.<MergeConflict>of(),
+              ImmutableMap.copyOf(overwritableDeps),
+              direct.mergeNonOverwritable(transitive),
+              ImmutableMap.copyOf(assets)));
+    } catch (IOException e) {
+      throw new MergingException(e);
+    }
   }
 }
diff --git a/src/tools/android/java/com/google/devtools/build/android/DependencyAndroidData.java b/src/tools/android/java/com/google/devtools/build/android/DependencyAndroidData.java
index d5d5837258..a892ec817d 100644
--- a/src/tools/android/java/com/google/devtools/build/android/DependencyAndroidData.java
+++ b/src/tools/android/java/com/google/devtools/build/android/DependencyAndroidData.java
@@ -139,8 +139,9 @@ class DependencyAndroidData {
   }
 
   /**
-   * Adds all the resource directories as ResourceSets. This acts a loose merge
-   * strategy as it does not test for overrides.
+   * Adds all the resource directories as ResourceSets. This acts a loose merge strategy as it does
+   * not test for overrides.
+   *
    * @param resourceSets A list of resource sets to append to.
    */
   void addAsResourceSets(List<ResourceSet> resourceSets) {
@@ -152,8 +153,9 @@ class DependencyAndroidData {
   }
 
   /**
-   * Adds all the asset directories as AssetSets. This acts a loose merge
-   * strategy as it does not test for overrides.
+   * Adds all the asset directories as AssetSets. This acts a loose merge strategy as it does not
+   * test for overrides.
+   *
    * @param assetSets A list of asset sets to append to.
    */
   void addAsAssetSets(List<AssetSet> assetSets) {
@@ -212,4 +214,15 @@ class DependencyAndroidData {
       pathWalker.walkAssets(path);
     }
   }
+
+  public void deserialize(
+      AndroidDataSerializer serializer,
+      KeyValueConsumers consumers)
+      throws DeserializationException {
+    // Missing symbolsTxt means the resources where provided via android_resources rules.
+    if (symbolsTxt == null) {
+      throw new DeserializationException(true);
+    }
+    serializer.read(symbolsTxt, consumers);
+  }
 }
diff --git a/src/tools/android/java/com/google/devtools/build/android/MergeConflict.java b/src/tools/android/java/com/google/devtools/build/android/MergeConflict.java
index 8ce67fc559..a45993b497 100644
--- a/src/tools/android/java/com/google/devtools/build/android/MergeConflict.java
+++ b/src/tools/android/java/com/google/devtools/build/android/MergeConflict.java
@@ -19,7 +19,6 @@ import com.google.common.base.Preconditions;
 import com.android.annotations.VisibleForTesting;
 import com.android.annotations.concurrency.Immutable;
 
-import java.nio.file.Path;
 import java.util.Objects;
 
 /**
@@ -29,23 +28,23 @@ import java.util.Objects;
  */
 @Immutable
 public class MergeConflict {
-  private static final String CONFLICT_MESSAGE = "%s is provided from %s and %s";
+  private static final String CONFLICT_MESSAGE = "\n\u001B[31mCONFLICT:\u001B[0m"
+          + " %s is provided with ambigious priority from: \n\t%s\n\t%s";
 
   private final DataKey dataKey;
-  private final Path first;
-  private final Path second;
+  private final DataValue first;
+  private final DataValue second;
 
-  private MergeConflict(DataKey dataKey, Path first, Path second) {
+  private MergeConflict(DataKey dataKey, DataValue sortedFirst, DataValue sortedSecond) {
     this.dataKey = dataKey;
-    this.first = first;
-    this.second = second;
+    this.first = sortedFirst;
+    this.second = sortedSecond;
   }
 
   /**
    * Creates a MergeConflict between two DataResources.
    *
-   * The {@link DataKey} must match the first.dataKey() and second
-   * .dataKey().
+   * The {@link DataKey} must match the first.dataKey() and second .dataKey().
    *
    * @param dataKey The dataKey name that both DataResources share.
    * @param first The first DataResource.
@@ -54,25 +53,34 @@ public class MergeConflict {
    */
   public static MergeConflict between(DataKey dataKey, DataValue first, DataValue second) {
     Preconditions.checkNotNull(dataKey);
-    return of(dataKey, first.source(), second.source());
+    return of(dataKey, first, second);
   }
 
   @VisibleForTesting
-  static MergeConflict of(DataKey key, Path first, Path second) {
+  static MergeConflict of(DataKey key, DataValue first, DataValue second) {
     // Make sure the paths are always ordered.
-    Path sortedFirst = first.compareTo(second) > 0 ? first : second;
-    Path sortedSecond = sortedFirst != first ? first : second;
+    DataValue sortedFirst = first.source().compareTo(second.source()) > 0 ? first : second;
+    DataValue sortedSecond = sortedFirst != first ? first : second;
     return new MergeConflict(key, sortedFirst, sortedSecond);
   }
 
   public String toConflictMessage() {
-    return String.format(CONFLICT_MESSAGE, dataKey, first, second);
+    return String.format(
+        CONFLICT_MESSAGE, dataKey.toPrettyString(), first.source(), second.source());
   }
 
   public DataKey dataKey() {
     return dataKey;
   }
 
+  DataValue first() {
+    return first;
+  }
+
+  DataValue second() {
+    return second;
+  }
+
   @Override
   public String toString() {
     return MoreObjects.toStringHelper(this)
diff --git a/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java b/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
index 50c62e9001..891dbe04c1 100644
--- a/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
+++ b/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.android;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -30,9 +29,9 @@ import java.nio.file.Path;
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.HashMap;
-import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -52,6 +51,72 @@ import javax.xml.stream.XMLStreamException;
  */
 @Immutable
 public class ParsedAndroidData {
+
+  static class Builder {
+    private final Map<DataKey, DataResource> overwritingResources;
+    private final Map<DataKey, DataResource> nonOverwritingResources;
+    private final Map<DataKey, DataAsset> assets;
+    private final Set<MergeConflict> conflicts;
+    private final List<Exception> errors = new ArrayList<>();
+
+    public Builder(
+        Map<DataKey, DataResource> overwritingResources,
+        Map<DataKey, DataResource> nonOverwritingResources,
+        Map<DataKey, DataAsset> assets,
+        Set<MergeConflict> conflicts) {
+      this.overwritingResources = overwritingResources;
+      this.nonOverwritingResources = nonOverwritingResources;
+      this.assets = assets;
+      this.conflicts = conflicts;
+    }
+
+    static Builder newBuilder() {
+      final Map<DataKey, DataResource> overwritingResources = new LinkedHashMap<>();
+      final Map<DataKey, DataResource> nonOverwritingResources = new LinkedHashMap<>();
+      final Map<DataKey, DataAsset> assets = new LinkedHashMap<>();
+      final Set<MergeConflict> conflicts = new LinkedHashSet<>();
+      return new Builder(overwritingResources, nonOverwritingResources, assets, conflicts);
+    }
+
+    private void checkForErrors() throws MergingException {
+      if (!errors.isEmpty()) {
+        StringBuilder messageBuilder = new StringBuilder();
+        for (Exception e : errors) {
+          messageBuilder.append("\n").append(e.getMessage());
+        }
+        throw new MergingException(messageBuilder.toString());
+      }
+    }
+
+    ParsedAndroidData build() throws MergingException {
+      checkForErrors();
+      return ParsedAndroidData.of(
+          ImmutableSet.copyOf(conflicts),
+          ImmutableMap.copyOf(overwritingResources),
+          ImmutableMap.copyOf(nonOverwritingResources),
+          ImmutableMap.copyOf(assets));
+    }
+
+    ResourceFileVisitor resourceVisitor() {
+      return new ResourceFileVisitor(
+          new OverwritableConsumer<>(overwritingResources, conflicts),
+          new NonOverwritableConsumer(nonOverwritingResources),
+          errors);
+    }
+
+    AssetFileVisitor assetVisitorFor(Path path) {
+      return new AssetFileVisitor(
+          RelativeAssetPath.Factory.of(path), new OverwritableConsumer<>(assets, conflicts));
+    }
+
+    public KeyValueConsumers consumers() {
+      return KeyValueConsumers.of(
+          new OverwritableConsumer<>(overwritingResources, conflicts),
+          new NonOverwritableConsumer(nonOverwritingResources),
+          new OverwritableConsumer<>(assets, conflicts));
+    }
+  }
+
   /** A Consumer style interface that will appendTo a DataKey and DataValue. */
   interface KeyValueConsumer<K extends DataKey, V extends DataValue> {
     void consume(K key, V value);
@@ -62,7 +127,7 @@ public class ParsedAndroidData {
 
     private Map<DataKey, DataResource> target;
 
-    public NonOverwritableConsumer(Map<DataKey, DataResource> target) {
+    NonOverwritableConsumer(Map<DataKey, DataResource> target) {
       this.target = target;
     }
 
@@ -98,67 +163,31 @@ public class ParsedAndroidData {
   /**
    * An AndroidDataPathWalker that collects DataAsset and DataResources for an ParsedAndroidData.
    */
-  private static final class ParsedAndroidDataBuildingPathWalker implements AndroidDataPathWalker {
-    private final Set<MergeConflict> conflicts;
-    private final Map<DataKey, DataAsset> assets;
-    private final ResourceFileVisitor resourceVisitor;
+  static final class ParsedAndroidDataBuildingPathWalker implements AndroidDataPathWalker {
     private static final ImmutableSet<FileVisitOption> FOLLOW_LINKS =
         ImmutableSet.of(FileVisitOption.FOLLOW_LINKS);
-    private Map<DataKey, DataResource> overwritingResources;
-    private Map<DataKey, DataResource> nonOverwritingResources;
-
-    private static ParsedAndroidDataBuildingPathWalker create() {
-      final Map<DataKey, DataResource> overwritingResources = new HashMap<>();
-      final Map<DataKey, DataResource> nonOverwritingResources = new HashMap<>();
-      final Map<DataKey, DataAsset> assets = new HashMap<>();
-      final Set<MergeConflict> conflicts = new HashSet<>();
-
-      final ResourceFileVisitor resourceVisitor =
-          new ResourceFileVisitor(
-              new OverwritableConsumer<>(overwritingResources, conflicts),
-              new NonOverwritableConsumer(nonOverwritingResources));
-      return new ParsedAndroidDataBuildingPathWalker(
-          conflicts, assets, overwritingResources, nonOverwritingResources, resourceVisitor);
+    private final Builder builder;
+
+    private ParsedAndroidDataBuildingPathWalker(Builder builder) {
+      this.builder = builder;
     }
 
-    private ParsedAndroidDataBuildingPathWalker(
-        Set<MergeConflict> conflicts,
-        Map<DataKey, DataAsset> assets,
-        Map<DataKey, DataResource> overwritingResources,
-        Map<DataKey, DataResource> nonOverwritingResources,
-        ResourceFileVisitor resourceVisitor) {
-      this.conflicts = conflicts;
-      this.assets = assets;
-      this.overwritingResources = overwritingResources;
-      this.nonOverwritingResources = nonOverwritingResources;
-      this.resourceVisitor = resourceVisitor;
+    static ParsedAndroidDataBuildingPathWalker create(Builder builder) {
+      return new ParsedAndroidDataBuildingPathWalker(builder);
     }
 
     @Override
     public void walkResources(Path path) throws IOException {
-      Files.walkFileTree(path, FOLLOW_LINKS, Integer.MAX_VALUE, resourceVisitor);
+      Files.walkFileTree(path, FOLLOW_LINKS, Integer.MAX_VALUE, builder.resourceVisitor());
     }
 
     @Override
     public void walkAssets(Path path) throws IOException {
-      Files.walkFileTree(
-          path,
-          FOLLOW_LINKS,
-          Integer.MAX_VALUE,
-          new AssetFileVisitor(
-              RelativeAssetPath.Factory.of(path), new OverwritableConsumer<>(assets, conflicts)));
+      Files.walkFileTree(path, FOLLOW_LINKS, Integer.MAX_VALUE, builder.assetVisitorFor(path));
     }
 
-    /**
-     * Creates an {@link ParsedAndroidData} from {@link DataAsset} and {@link DataResource}.
-     */
-    public ParsedAndroidData createParsedAndroidData() throws MergingException {
-      resourceVisitor.checkForErrors();
-      return ParsedAndroidData.of(
-          ImmutableSet.copyOf(conflicts),
-          ImmutableMap.copyOf(overwritingResources),
-          ImmutableMap.copyOf(nonOverwritingResources),
-          ImmutableMap.copyOf(assets));
+    ParsedAndroidData createParsedAndroidData() throws MergingException {
+      return builder.build();
     }
   }
 
@@ -169,7 +198,7 @@ public class ParsedAndroidData {
     private final RelativeAssetPath.Factory dataKeyFactory;
     private KeyValueConsumer<DataKey, DataAsset> assetConsumer;
 
-    public AssetFileVisitor(
+    AssetFileVisitor(
         RelativeAssetPath.Factory dataKeyFactory,
         KeyValueConsumer<DataKey, DataAsset> assetConsumer) {
       this.dataKeyFactory = dataKeyFactory;
@@ -191,18 +220,20 @@ public class ParsedAndroidData {
    * A FileVisitor that walks a resource tree and extract FullyQualifiedName and resource values.
    */
   private static class ResourceFileVisitor extends SimpleFileVisitor<Path> {
-    private final List<Exception> errors = new ArrayList<>();
-    private final OverwritableConsumer<DataKey, DataResource> overwritingConsumer;
-    private final NonOverwritableConsumer nonOverwritingConsumer;
+    private final KeyValueConsumer<DataKey, DataResource> overwritingConsumer;
+    private final KeyValueConsumer<DataKey, DataResource> nonOverwritingConsumer;
+    private final List<Exception> errors;
     private boolean inValuesSubtree;
     private FullyQualifiedName.Factory fqnFactory;
     private final XMLInputFactory xmlInputFactory = XMLInputFactory.newFactory();
 
-    public ResourceFileVisitor(
-        OverwritableConsumer<DataKey, DataResource> overwritingConsumer,
-        NonOverwritableConsumer nonOverwritingConsumer) {
+    ResourceFileVisitor(
+        KeyValueConsumer<DataKey, DataResource> overwritingConsumer,
+        KeyValueConsumer<DataKey, DataResource> nonOverwritingConsumer,
+        List<Exception> errors) {
       this.overwritingConsumer = overwritingConsumer;
       this.nonOverwritingConsumer = nonOverwritingConsumer;
+      this.errors = errors;
     }
 
     private static String deriveRawFullyQualifiedName(Path path) {
@@ -222,37 +253,19 @@ public class ParsedAndroidData {
       return pathWithExtension;
     }
 
-    private void checkForErrors() throws MergingException {
-      if (!getErrors().isEmpty()) {
-        StringBuilder errors = new StringBuilder();
-        for (Exception e : getErrors()) {
-          errors.append("\n").append(e.getMessage());
-        }
-        throw new MergingException(errors.toString());
-      }
-    }
-
     @Override
     public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs)
         throws IOException {
       final String[] dirNameAndQualifiers = dir.getFileName().toString().split("-");
       inValuesSubtree = "values".equals(dirNameAndQualifiers[0]);
-      fqnFactory = FullyQualifiedName.Factory.from(getQualifiers(dirNameAndQualifiers));
+      fqnFactory = FullyQualifiedName.Factory.fromDirectoryName(dirNameAndQualifiers);
       return FileVisitResult.CONTINUE;
     }
 
-    private List<String> getQualifiers(String[] dirNameAndQualifiers) {
-      if (dirNameAndQualifiers.length == 1) {
-        return ImmutableList.of();
-      }
-      return Arrays.asList(
-          Arrays.copyOfRange(dirNameAndQualifiers, 1, dirNameAndQualifiers.length));
-    }
-
     @Override
     public FileVisitResult visitFile(Path path, BasicFileAttributes attrs) throws IOException {
       try {
-        if (!Files.isDirectory(path)) {
+        if (!Files.isDirectory(path) && !path.getFileName().toString().startsWith(".")) {
           if (inValuesSubtree) {
             DataResourceXml.parse(
                 xmlInputFactory, path, fqnFactory, overwritingConsumer, nonOverwritingConsumer);
@@ -267,10 +280,6 @@ public class ParsedAndroidData {
       }
       return super.visitFile(path, attrs);
     }
-
-    public List<Exception> getErrors() {
-      return errors;
-    }
   }
 
   /** Creates ParsedAndroidData of conflicts, assets overwriting and nonOverwriting resources. */
@@ -295,7 +304,7 @@ public class ParsedAndroidData {
   public static ParsedAndroidData from(UnvalidatedAndroidData primary)
       throws IOException, MergingException {
     final ParsedAndroidDataBuildingPathWalker pathWalker =
-        ParsedAndroidDataBuildingPathWalker.create();
+        ParsedAndroidDataBuildingPathWalker.create(Builder.newBuilder());
     primary.walk(pathWalker);
     return pathWalker.createParsedAndroidData();
   }
@@ -314,7 +323,7 @@ public class ParsedAndroidData {
   public static ParsedAndroidData from(List<DependencyAndroidData> dependencyAndroidDataList)
       throws IOException, MergingException {
     final ParsedAndroidDataBuildingPathWalker pathWalker =
-        ParsedAndroidDataBuildingPathWalker.create();
+        ParsedAndroidDataBuildingPathWalker.create(Builder.newBuilder());
     for (DependencyAndroidData data : dependencyAndroidDataList) {
       data.walk(pathWalker);
     }
@@ -369,28 +378,30 @@ public class ParsedAndroidData {
   /**
    * Returns a list of resources that would overwrite other values when defined.
    *
-   * <p>Example:
+   * <p>
+   * Example:
    *
    * A string resource (string.Foo=bar) could be redefined at string.Foo=baz.
    *
    * @return A map of key -&gt; overwriting resources.
    */
   @VisibleForTesting
-  public Map<DataKey, DataResource> getOverwritingResources() {
+  Map<DataKey, DataResource> getOverwritingResources() {
     return overwritingResources;
   }
 
   /**
    * Returns a list of resources that would not overwrite other values when defined.
    *
-   * <p>Example:
+   * <p>
+   * Example:
    *
    * A id resource (id.Foo) could be redefined at id.Foo with no adverse effects.
    *
    * @return A map of key -&gt; non-overwriting resources.
    */
   @VisibleForTesting
-  public Map<DataKey, DataResource> getNonOverwritingResources() {
+  Map<DataKey, DataResource> getNonOverwritingResources() {
     return nonOverwritingResources;
   }
 
@@ -398,7 +409,8 @@ public class ParsedAndroidData {
    * Returns a list of assets.
    *
    * Assets always overwrite during merging, just like overwriting resources.
-   * <p>Example:
+   * <p>
+   * Example:
    *
    * A text asset (foo/bar.txt, containing fooza) could be replaced with (foo/bar.txt, containing
    * ouza!) depending on the merging process.
@@ -417,7 +429,7 @@ public class ParsedAndroidData {
     return overwritingResources.entrySet();
   }
 
-  public Iterable<Entry<DataKey, DataResource>> iterateDataResourceEntries() {
+  Iterable<Entry<DataKey, DataResource>> iterateDataResourceEntries() {
     return Iterables.concat(overwritingResources.entrySet(), nonOverwritingResources.entrySet());
   }