[PATCH] D37099: Added optional validation of svn sources to Dockerfiles.

[Manuel Klimek via Phabricator via llvm-commits]

klimek added inline comments.


================
Comment at: utils/docker/scripts/llvm_checksum/llvm_checksum_utils.py:1
+"""Contains helper functions to compute checksums for LLVM checkouts.
+"""
----------------
Why 2 files? (generally, I dislike files named "util" :)



================
Comment at: utils/docker/scripts/llvm_checksum/llvm_checksum_utils.py:27-28
+                         is_ignored,
+                         content_hasher,
+                         hash_algo=hashlib.sha256):
+  """ Computes checksums for all files and symlinks under path.
----------------
Why's the algo not part of the content_hasher?
Can't we just use a hasher that has the exact interface we need?


================
Comment at: utils/docker/scripts/llvm_checksum/llvm_checksum_utils.py:48
+
+  def proc_file(fullpath):
+    if os.path.islink(fullpath):
----------------
Call it process_file? (proc_file sounds like a file in /proc :P)


================
Comment at: utils/docker/scripts/llvm_checksum/llvm_checksum_utils.py:82-83
+      # Don't recurse into ignored subdirectories.
+      next_dirs = [d for d in dirs if not is_ignored(os.path.join(root, d))]
+      dirs[:] = next_dirs
+      # Process files.
----------------
Can't we put that in the same line?


================
Comment at: utils/docker/scripts/llvm_checksum/llvm_checksum_utils.py:122-123
+  hasher = hash_algo()
+  # Feed the number of files to the hasher.
+  hasher.update(str(len(file_checksums)))
+  for kind, file_path, checksum in file_checksums:
----------------
Given that we feed in paths, this seems redundant?


================
Comment at: utils/docker/scripts/llvm_checksum/llvm_checksum_utils.py:131-133
+    elif kind == FileKind.VALID_SYMLINK:
+      hasher.update("@symlink")
+    else:
----------------
Why not just feed the content (file content or symlink target)?


https://reviews.llvm.org/D37099