[Bug]: Overwriting seems to break starcraft mpq

[MikaMika]

mpqcli version

0.10.0

Host OS

Linux

Installation method

Prebuilt binary

mpqcli command

mpqcli add --game starcraft1 --update --overwrite temp.mpq mpq

Expected behaviour

I'm downgrading the Starcraft mod Cosmonarchy to a previous version, that deletes some files in mpq dir and modifies others.
I'm checking git diff to see which files have been touched.
If I delete everything from that diff list, then mpqcli add will only have to add and things are working well.
If I delete only files with a D marker and let the M files being overwritten, then the mod errors.

Actual behaviour / error output

Screenshot above:
"Data File Error"
The problem occured while trying to load a file
> unit\
> File not found.

Additional context

You may install Cosmonarchy using the Fraud Launcher with a Wine wrapper. I like to use Faugus Launcher for that. There is more than one way to do it, so let me know if you need more info.

update_mpq() {
    target=${1:-origin/HEAD}
    mpq=${2:-temp.mpq}
    dir=${3:-mpq}
    git fetch # in case target is origin/HEAD
    # this part uses tempfiles to make it easier to understand
    # pipes can be used instead:
    #git diff --name-status "HEAD..$target" | grep '^[DR]' | cut -f2 | grep "^$dir" | sed "s,$dir,," | tr / \\\\ | mpqcli remove "$mpq" -
    git diff --name-status "HEAD..$target" > gitdiff1.txt # list of diff files with status
    grep '^[DR]'              gitdiff1.txt > gitdiff2.txt # get Deleted and Renamed files only
    cut -f2                   gitdiff2.txt > gitdiff3.txt # get current path to file
    grep "^${dir%/}/"         gitdiff3.txt > gitdiff4.txt # get files only from the mpq dir
    sed "s,${dir%/}/,,"       gitdiff4.txt > gitdiff5.txt # remove mpq dir from path
    tr / \\\\               < gitdiff5.txt > gitdiff6.txt # "unix2dos" slash 2 backslash
    mpqcli remove "$mpq" -  < gitdiff6.txt
    #rm gitdiff*.txt                                       # remove tempfiles or keep as logs
    git restore .                                         # making sure switch will not complain
    git switch --detach "$target"                         # checking out files from target commit
    mpqcli add -g sc --update --overwrite "$mpq" "$dir" | grep -v ^.~ # hide [~] messages
    if [[ $? -ne 0 ]]; then # if mpqcli add failed is probably because archive needs to be created first
    mpqcli create --game starcraft1 --output "$mpq" "$dir" | grep -v ^.+ # hide [+] messages
    fi
}

COSMONARCHY=~/Faugus/default/drive_c/Cosmonarchy # Make sure you modify this according to your setup
UPGRADE=dc02de47bef61e82f7cbe7e17255ab4c7ee6b877
DOWNGRADE=f1ff5a71017a9498ac5d2b5a5df0ace78400b9bd

# Assuming you are doing this in a Wine prefix with StarCraft 1.16.1 already installed
# If not, download and unzip the following, then update the registry using the .bat file included
# https://mikamika.top/cm/dl/bw-lite.zip
mkdir -p "$COSMONARCHY"
git clone --filter=blob:none --no-checkout https://gitlab.com/the-no-frauds-club/cosmonarchy-bw-prerelease.git "$COSMONARCHY/Prerelease"
cd "$COSMONARCHY/Prerelease" || exit
# The following will create the archive since it does not exist yet
update_mpq $UPGRADE
# Launching the mod now would work fine
update_mpq $DOWNGRADE
# Launching after a downgrade would error
update_mpq $UPGRADE
# Going back to when the archive was created will work again

# Now let's try the opposite
rm temp.mpq
# Create an older archive works
update_mpq $DOWNGRADE
# Update it with newer version works
update_mpq $UPGRADE
# Downgrade back to when created works too
update_mpq $DOWNGRADE

I mentioned that deleting all files and then adding them instead of overwriting would work.
You can change the code above from grep '^[DR]' to grep '^[DRM]' to delete those Modified files and fix the script. Doing UPGRADE -> DOWNGRADE would work that way, but I'm highlighting the problem with --overwrite that I do not quite understand, only that it's giving different result.

Sorry again for a lengthy report. I hope the example will be clear enough. Thanks!

[sjoblomj]

That's strange. Could you please share the output of each mpqcli command, especially mpqcli add -g sc --update --overwrite "$mpq" "$dir"?

[MikaMika]

issue174.logs.zip

See the .sh file for the steps in order with comments.
removed-files.2.txt and removed-files.3.txt have 0 B because there was nothing to remove when upgrading.

[thomaslaurenson]

Hey @MikaMika - sorry about the delay in replying to your issue. Thanks for providing the additional information and logs. I will do some investigation.

[thomaslaurenson]

Thanks for the detailed repo script and logs. It made this much easier to track this bug. I am not 100% sure about the issue, but pretty sure it is a problem with how the --update argument works.

The --update argument only compares file size to decide whether to skip a file, not its content. If a file changes between two versions of your mod but happens to keep the exact same byte count, --update wrongly reports it as unchanged and skips it.

Using your reproduction script and the two commits you tested with, I diffed dc02de47 (upgrade target) against f1ff5a71 (downgrade target) and found 5 files that changed content but not size:

• arr/flingy.datex: 15000
• arr/images.datex: 114000
• arr/sprites.datex: 14000
• arr/units.datex: 291000
• arr/weapons.datex: 10752

Your log.03.add.txt confirms it, as all 5 show:

[~] Skipping unchanged file: ...

I am not sure how to fix this in the --update subcommand. If we add more logic to the update feature, it will become slow (e.g., crc or md5 hashing to check file content). Which exactly the problem it attempts to solve. I thought about timestamp check (e.g., modified time), but this wouldn't work with git. I am going to have to think more about this.

In the meantime, a workaround is to drop --update and let --overwrite alone handle it (slower, since it re-adds every file, but avoids the skip logic entirely).

[MikaMika]

I don't mind the wait, no worries.
Oh I answered to Ojan in Cosmonarchy Discord and forgot to copy it here:

Seeing how the error message ends with unit\ these should be the relevant lines

[+] Adding file: unit\cmdbtns\cmdicons.grp
[+] File already exists in MPQ archive: unit\cmdbtns\uniticons.grp - Overwriting...
[+] Adding file: unit\cmdbtns\uniticons.grp
[+] File already exists in MPQ archive: unit\wirefram\grpwire.grp - Overwriting...
[+] Adding file: unit\wirefram\grpwire.grp
[+] File already exists in MPQ archive: unit\wirefram\wirefram.grp - Overwriting...
[+] Adding file: unit\wirefram\wirefram.grp

I'm wondering if there is a character escape with \c or \w
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Character_escape#c

According to that, \cm would be ASCII character 13 "CR" so there is a Carriage return in the middle of the path.

[thomaslaurenson]

I looked into the escape-character idea. I'm confident it's not the cause. I checked the three places in mpqcli that touch these file paths:

• The code that converts / to \
• The code that reads the file list you pipe in
• The command-line parsing library

None of them treat backslashes as special "escape codes." They just copy the characters through as-is, so \c and \w are plain text to mpqcli, not commands or any other representation.

To be extra sure, I also pulled the actual files mpqcli wrote into the archive after the update and compared them byte-for-byte against the originals in git, they matched exactly, so nothing is getting mangled on the way in.

Little unsure what to check next. Have you been able to make patches with mpcli using other subcommands? Such as create from a directory of files and it worked? Or created them with another MPQ editor, such as Ludiks?

[MikaMika]

Ah! I didn't pay enough attention to what you said

found 5 files that changed content but not size

I am sorry I made you work more.
I understand the limitation, and the workaround for me is simply to remove all files found in git diff, then add --update so that no overwrite happens.

I would "Close as not planned", but I let you do it in case you prefer else.
Thank you both for looking into it, love the support 😄

[sjoblomj]

Apologies, I have not had any time at all to look at this before. My understanding of the discussion is that it appears that the --update flag can trick the user - files that were supposed to be added were in fact not, because the updated files had the same size as the files in the archive. I believe this will be fairly common, since many commonly modded files for at least StarCraft I and WarCraft II have fixed file sizes (the *.dat files in the arr directory for StarCraft I for example).

What we might do is this: the (attributes) file in the archives can contain timestamps, MD5 sums and CRC32 checksums of the files in the archive. The (attributes) file is however not always present (I believe it is not for the games starcraft1, warcraft2, diablo1, diablo2 and lordsofmagic), and the three values therein are also not always present even if (attributes) itself is there. When present, StormLib exposes functions to access this. I have created PR #177 to extend the --update flag to check the file size, and if it matches it compares the Timestamps, MD5 sum or CRC32 if present. If these three values are not present (or if any of the checks don't match), then we add the file again.

However, rather than merging PR #177, I'm sort of in favour of removing --update altogether. While it does seem convenient, it adds complexity to mpqcli that I think belongs at user-level. For users who know their Unix tools (or who can ask AI provide the commands), it should be easy enough to locally do the bookkeeping of keeping track of checksums and only add files that have been updated since the last run.

---

If we do want to keep the --update flag though, and if PR #177 is merged, your script @MikaMika, needs to be updated - otherwise passing in --update will do nothing after #177 is merged:

• The archive would need to have an (attributes) file. I believe this command would create a StarCraft I-compatible MPQ with an (attributes) file containing MD5, CRC32 and timestamps: mpqcli create --game starcraft1 --attr-flags 7 -o "$mpq" "$dir"
• If your MPQ contains an (attributes) file (as it would if you pass --attr-flags 7), then for the best speed gain, you should try to make the timestamps match. For this, you'd need to update your script, so that when it pulls data from git (which does not store original timestamps), it modifies the timestamps of the files to be e.g. the timestamp of the commit that last changed that file. See for example here.

[MikaMika]

For a moment I forgot what --update and --overwrite do, so maybe they are confusing indeed.
Quick summary for myself:

• add will skip already existing files [!].
• --update will skip similar files [~] but skips because file already exists [!].
• --overwrite will force replace existing files [!] except similar files [~].

I wanted to use --update to skip already existing files, but add does that already.
If I have a list of modified files, I can use remove + add, or add --overwrite.
If I do not have such list, that's where --update would be helpful.
Note that --update without --overwrite is almost like a dry-run but new files will still be written, so it's a bit awkward.
For me, I don't mind if --update is kept or not. I see pros and cons, but I don't need it personally at the moment.

Thanks to git diff --name-status HEAD..$COMMIT, I have all the information I need for mpqcli.

D     mpq/dir/deleted.txt
A     mpq/dir/added.txt
M     mpq/dir/modified.txt
R95   mpq/dir/renamed1.txt  mpq/dir/95percent.txt
R100  mpq/dir/renamed2.txt  mpq/other/exactmatch.txt

• D remove
• A add
• M add --overwrite
• R95 remove field2 + add field3
• R100 same as above, unless rename exist
  Would be nice to have mpqcli rename mpq.mpq dir\renamed2.txt other\exactmatch.txt.
  Should this be a new issue for feature request?