5
\$\begingroup\$

I'm ultimately trying to compare the MD5 hashes for files in two disparate directory trees to see if files are missing from one directory or the other.

More broadly, I'm pushing photos from my DSLR camera to Google Drive, and need to know which files need to be synced up. Pictures from my phone are automatically being synced with Google Drive, and I'd also like to detect which ones I need to sync down. I have separate scripts for getting the MD5 hashes from Google Drive and for comparing the MD5 hashes to see which files need to be synced, up or down.

#!/usr/bin/python

import os
import sys
import hashlib
import csv

src_dir = '/Volumes/Archive/Pictures'

with open('checksums_archive.tsv', 'wb') as fout:
    writer = csv.writer(fout, delimiter='\t', quotechar='\"', quoting=csv.QUOTE_MINIMAL)

    for root, subdirs, files in os.walk(src_dir):
        for file in files:
            file_path = os.path.join(root, file)
            checksum = hashlib.md5(open(file_path, 'rb').read()).hexdigest()

            writer.writerow([root, file, checksum])
\$\endgroup\$
3
\$\begingroup\$

Four things:

  1. You should put global constants in CAPITAL_LETTERS, according to PEP8.
  2. I would make the file_name for the checksums a constant
  3. You never close the files you iterate over.
  4. In general, doing all writes at the same time is faster.

So for 2, 3 & 4, maybe use:

for root, subdirs, files in os.walk(SRC_DIR):
    checksums = []
    for file in files:
        with open(os.path.join(root, file), 'rb') as _file:
            checksums.append([root, file, hashlib.md5(_file.read()).hexdigest()])
    writer.writerows(checksums)

This will perform one write per subdir, which should be faster than a write after every file. If you have a lot of files this will take more memory, which should be slightly mitigated by doing a write for each subdir. You can even pull the checksums list outside of the os.walk loop to have only one (possibly giant) write.

Otherwise, clear and readable code!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks Graipher! Code looks, and performs, a lot better now. I've about 20k files to write, but have a lot of memory, so will give one big write a try. \$\endgroup\$ – saxman Jul 4 '16 at 18:31
  • \$\begingroup\$ You're welcome! However, it is customary to wait a bit before accepting an answer, to allow multiple people to give an answer (even though I do like that green checkmark :) ) \$\endgroup\$ – Graipher Jul 4 '16 at 18:34
  • \$\begingroup\$ Fixed ;) Will check back in a few days... \$\endgroup\$ – saxman Jul 4 '16 at 19:05
  • \$\begingroup\$ @saxman That was quite a few days :D \$\endgroup\$ – Graipher Oct 21 '16 at 21:20
2
\$\begingroup\$

This is a relatively straightforward task, and you did well.

The result file should be opened in text mode rather than binary mode. In fact, in Python 3, it would be an error to write textual fields instead of byte strings to a binary file handle.

You used a with block to open the result file. You also use a with block to open each image file, so that you aren't relying on Python's garbage collector to help you prevent a file descriptor leak.

If you do that, then the nesting would become excessive. I would recommend defining two functions, to recognize the fact that your program is doing three things: walking the directory tree recursively, computing MD5 checksums, and writing a TSV file.

import csv
import hashlib
import os

def file_hash_hex(file_path, hash_func):
    with open(file_path, 'rb') as f:
        return hash_func(f.read()).hexdigest()

def recursive_file_listing(base_dir):
    for directory, subdirs, files in os.walk(base_dir):
        for filename in files:
            yield directory, filename, os.path.join(directory, filename)

src_dir = '/Volumes/Archive/Pictures'

with open('checksums_archive.tsv', 'w') as f:
    writer = csv.writer(f, delimiter='\t', quotechar='"', quoting=csv.QUOTE_MINIMAL)
    for directory, filename, path in recursive_file_listing(src_dir):
        writer.writerow((directory, filename, file_hash_hex(path, hashlib.md5)))

I recommend listing imports alphabetically. Note that import sys was superfluous.

I should probably mention that MD5 is no longer considered cryptographically secure. It's probably OK for use here as a checksum, where you don't expect there to be an adversary who would deliberately cause MD5 collisions. You might as well just switch to at least SHA1, though. For that reason, I've made the hash algorithm swappable in file_hash_hex() above.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.