Check (attributes) file when using --update flag

[sjoblomj]

The --update flag for the add subcommand intends to only add the files if the local file is deemed "updated" compared to the file in the archive. A file is currently considered "updated" when the file sizes aren't equal.

However, as issue #174 points out, files can be updated yet still have the same file size. This is not uncommon - some of the files that mods for StarCraft I and WarCraft II change most often always have fixed sizes.

While not mandatory, some MPQ archives contains an (attributes) file, which can contain MD5 sums, CRC32 checksums and timestamps of the files inside the archives. I believe that starcraft1, warcraft2, diablo1, diablo2 and lordsofmagic does not contain (attributes) but that newer games do.

This PR will check the file sizes, and if they are the same it proceeds to check against the values of (attributes) if present. It compares the MD5 if present, if not it checks the CRC32 if present, if not it checks the timestamps if present. It compares the timestamps if present, if not or if not matching, it checks the MD5 if present, if not it checks the CRC32 if present. So if file sizes and timestamps, MD5 or CRC32 are matching, the file is not considered "updated"; if file sizes, MD5 or CRC32 are not matching, the file is considered "updated" (but timestamps are allowed to be different).

If (attributes) is not present in the archive (which again, it would not be for starcraft1, warcraft2, diablo1, diablo2 and lordsofmagic), passing in --update will basically do nothing with this PR.

---

The code for CRC32 and MD5 was written by AI and is by its nature quite messy to read. We need to ask ourselves if this is complexity we want in mpqcli. I myself am somewhat leaning towards answering No to that question - and I'd then say that the --update flag should be removed altogether. In my opinion, it is not unreasonable to delegate the decision of whether to add a file to the archive to the user itself.

[sjoblomj]

It seems to me that MD5 is the most reliable metric but also the most expensive. CRC32 seems a little less reliable but a bit faster than MD5. Timestamps seem reliable for the matching case, i.e. if the timestamps and the file sizes match, it ought to be a strong sign of equality, but as #174 shows, timestamps that don't match don't need to mean that the files are not equal.

I'm thinking that we should thus start by comparing file sizes + timestamps. If the timestamps don't match, we look at MD5 if present, then resort to CRC32. So I'm proposing something like this:

flowchart TD
    A[Start] --> FS{File size equals}
    FS -->|No| U[Update file]
    FS -->|Yes| TE{Timestamps exist and equals} -->|Yes| DU
    TE -->|No| MX
    DU[Don't update file]
    MX{MD5 exists} -->|Yes| FB
    MX -->|No| CX
    FB{MD5 equals} -->|Yes| DU
    FB -->|No| U
    CX{CRC32 exists} -->|Yes| CE
    CX -->|No| U
    CE{CRC32 equals} -->|Yes| DU
    CE -->|No| U

Loading

I'll push a new commit with that implementation.

[sjoblomj]

I made a benchmark (code). The baseline is when we don't pass in --update at all:

Scenario	Min	Avg	Max	vs baseline
No --update (baseline)	3.30s	3.43s	3.78s	—
--update, no attributes	3.36s	3.50s	3.73s	—
--update, Timestamp matches	0.01s	0.01s	0.01s	430× faster
--update, MD5 (TS mismatch)	1.01s	1.04s	1.29s	3.3× faster
--update, CRC32 (TS mismatch, no MD5)	0.70s	0.71s	0.76s	4.8× faster

500 files · 512 KB each · 100 runs

---

So this implementation offers a clear speedup compared to dropping the --update flag. If the timestamps match, that speeds things up 430 times. Comparing MD5 is more reliable than CRC32 I believe, but a bit more expensive. It still offers 3.3 times speedup compared to no --update though.

[thomaslaurenson]

Hey @sjoblomj - many thanks for this PR. Sorry it has taken so long for me to look at it. I have two weeks off work, without much to do, so going to look at it now. Couple very quick notes:

• I like the approach, it looks good at first glance, and I want to merge in this functionality
• For the CRC and MD5 implementations, I would rather "vendor" some public domain implementations, I will do some research and find suitable implementations
• Do you have the code for the flowchart you included? It would be awesome to include that in the docs, and having the code would help regenerate if things change (no problem if it is not code-generated)

[thomaslaurenson]

Changes:

• Replaced the hand-written CRC32/MD5 in helpers.cpp (~190 lines) with stbrumme/hash-library as a submodule
• ComputeFileCrc32 and ComputeFileMd5 keep their signatures, so mpq.cpp logic is untouched
• LocalFileTimestamp now uses _wstat64 on Windows so non-ASCII paths aren't mangled (AI mentioned this)
• Added some clarifying comments - mainly for me in the future

Decisions:

• Choose hash-library because of zlib license (MIT-compatible), and well known

[sjoblomj]

Glad you like it :) The commits you added were good improvements, thanks for that.

Replacing the checksum code with a dependency is very reasonable, though I find it unfortunate to introduce an additional dependency.

---

The flowchart was made with Mermaid code, which GitHub has built-in support for:

flowchart TD
    A[Start] --> FS{File size equals}
    FS -->|No| U[Update file]
    FS -->|Yes| TE{Timestamps exist and equals} -->|Yes| DU
    TE -->|No| MX
    DU[Don't update file]
    MX{MD5 exists} -->|Yes| FB
    MX -->|No| CX
    FB{MD5 equals} -->|Yes| DU
    FB -->|No| U
    CX{CRC32 exists} -->|Yes| CE
    CX -->|No| U
    CE{CRC32 equals} -->|Yes| DU
    CE -->|No| U

[thomaslaurenson]

I agree about the additional dependency, I always try keep dependencies to a minimum. Interestingly, Stormlib does have CRC32 and MD5 included - well, kind of!

• extern/StormLib/src/libtomcrypt/src/hashes/md5.c has globally exported symbols from libtomcrypt for MD5
• Stormlib also uses zlib, which has CRC32 - but this can be system zlib, or the zlib provided by Stormlib

But they are not really public functions, and it seemed kind of hacky. I thought the middle option of another dependency, but ultra light and well trusted might be the best option.

I will make another commit to include the Mermaid diagram in the docs - I really like this. It is so easy to follow!

As usual, thanks Johan!

[sjoblomj]

Yes, I also saw that StormLib does include code for MD5/CRC. I don't think it would make sense for StormLib to expose it though - it seems like it would mess up its API and be a scope creep. So I think we need to reach for our own solution.

---

Another question: As pointed out in the first paragraph here, the --update and --overwrite are a bit confusingly named. I realize you want short names for flags (as do I), but maybe change --overwrite to --force-replace, and change --update to --replace-if-changed? That seems more self-explanatory to me.

[thomaslaurenson]

Thanks for the feedback:

• Agree about Stormlib, asking them to making hashing functions public is not really part of their scope, and trying to leverage the hashing functions provided is possible, but very hacky
• Also agree about the argument naming conventions. I did some thinking and looking at other unix-like tools to try find a solution

Proposed solution:

• Make --overwrite and --update mutually exclusive. Currently, --update alone can't actually update anything - which doesn't make sense. Changed files hit the "already exists, skipping" path and are counted as failures, so it only works with --overwrite.
• Proposed new logic:
  • default: no flag, which skips any file collisions
  • --overwrite replaces everything
  • --update replaces only what changed
• I want to keep the short names, and I think these make much more sense with the logic change. They also seem to match other unix-like tools, such as unzip -o/-u
• Modify it so that --update works on single files too, not just directories.

I am putting together the changes, but want to relook at them when I am fresh tomorrow. Then will push. Always open to feedback.

[sjoblomj]

That sounds like a very sensible improvement. Thanks for iterating on this! I still find the flag names to be not self-explanatory, but oh well 🙂

[sjoblomj]

I contributed this to StormLib, which was just merged. We can thus simplify the code somewhat. Let me get back with another commit to fix this please, before you merge this PR.

[sjoblomj]

I pushed a new commit that bumps the StormLib commit and now fetches MD5 info through SFileGetFileInfo. Feel free to re-add comments if you feel like I removed too much.