Merge branch 'main' into nihtest

NEWS.md

@@ -1,6 +1,7 @@
 X.X (XXXX-XX-XX)
 ================
 
+* Add '--report-changes' to show changes between last and current run.
 * Add `--delete-unknown-pattern` to remove unknown files matching a pattern.
 
 2.0 (2022-05-31)

TODO.md

@@ -5,11 +5,11 @@
 ### Implement
 
 - Fix failing tests.
-- Remember ROM set state between runs, option to display changes (something like -3 +2 missing games)
 - Variables in config file (e.g. for collection root directory).
 - Empty directory in ArchiveDir is not cleaned up, which makes removing empty archive fail.
 - SHA256 support
 - mia="yes" support
+- speed up fixing 'complete-only' sets: stop checking a game when one entry is missing
 
 ### Write Tests for Config
 

docs/CMakeLists.txt

@@ -5,13 +5,15 @@ set(MAN_PAGES
   mkmamedb.1
 )
 
+include(GNUInstallDirs)
+
 foreach(MAN_PAGE ${MAN_PAGES})
   string(REGEX REPLACE "[1-9]$" "${DOCUMENTATION_FORMAT}" SOURCE_FILE ${MAN_PAGE})
   if(DOCUMENTATION_FORMAT MATCHES "html")
-    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${MAN_PAGE} DESTINATION share/doc/${PROJECT_NAME} RENAME ${SOURCE_FILE})
+    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${MAN_PAGE} DESTINATION ${CMAKE_INSTALL_DOCDIR} RENAME ${SOURCE_FILE})
   else()
     string(REGEX REPLACE ".*(.)$" "man\\1" SUBDIR ${MAN_PAGE})
-    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${MAN_PAGE} DESTINATION man/${SUBDIR})
+    install(FILES ${CMAKE_CURRENT_BINARY_DIR}/${MAN_PAGE} DESTINATION ${CMAKE_INSTALL_MANDIR}/${SUBDIR})
   endif()
   # configure_file does not find out about updates to the sources, and it does not provide a target
   #configure_file(${SOURCE_FILE} ${MAN_PAGE} COPYONLY)

regress/CMakeLists.txt

@@ -44,6 +44,7 @@ set(DBS
   mamedb-one-game-two-roms.db
   mamedb-parent-crcdiff.db
   mamedb-parent-no-common.db
+  mamedb-reversesorted.db
   mamedb-size-empty.db
   mamedb-small.db
   mamedb-two-games.db
@@ -108,6 +109,7 @@ set(XFAIL_TESTS
   file-no-crc-wrong-name-other-set.test
   rom-from-extra-loose-directory.test
   rom-from-extra-loose-toplevel.test
+  unknown-rename-failed.test
 )
 
 if(RUN_REGRESS)

regress/mamedb-reversesorted.dat

@@ -0,0 +1,10 @@
+clrmamepro (
+)
+
+game (
+	name 2-48
+	description 2-48
+	rom ( name 08.rom size 8 crc 3656897d md5 095ca6fcc1279865662b553147eb8f6d sha1 111bb8b7549e3386a996845405b02164f17c7b37 )
+	rom ( name 04.rom size 4 crc d87f7e0c md5 098f6bcd4621d373cade4e832627b4f6 sha1 a94a8fe5ccb19ba61c4c0873d391e987982fbbd3 )
+)
+

regress/mamedb-whitespace.dump

@@ -0,0 +1,9 @@
+>>> table dat (dat_idx, name, description, author, version)
+0|M.A.M.E.|<null>|<null>|1
+>>> table file (game_id, file_type, file_idx, name, merge, status, location, size, crc, md5, sha1)
+1|0|0|parent-1|<null>|0|0|8|305419896|<null>|<1234567890123456789012345678901234567890>
+1|0|1|parent-2|<null>|0|0|8|2427178479|<null>|<2345678901234567890123456789012345678901>
+>>> table game (game_id, name, parent, description, dat_idx)
+1|parent|<null>|Parent|0
+>>> table rule (rule_idx, start_offset, end_offset, operation)
+>>> table test (rule_idx, test_idx, type, offset, size, mask, value, result)

regress/mamedb-whitespace.xml

@@ -0,0 +1,20 @@
+<?xml version="1.0"?>
+<mame build="1" debug="no" mameconfig="10">
+	<game name="parent" sourcefile="parent.c">
+		<description>Parent</description>
+		<year>2014</year>
+		<manufacturer>Made Up Games</manufacturer>
+		<rom name="parent-1" size="
+				   8
+				   " crc="
+					  12345678
+					  " sha1="
+						  1234567890123456789012345678901234567890
+						  " region="
+							    maincpu
+							    " offset="
+								      8000
+								      "/>
+		<rom name="parent-2" size="8" crc="90abcdef" sha1="2345678901234567890123456789012345678901" region="maincpu" offset="a000"/>
+	</game>
+</mame>

regress/mkmamedb-broken-sha1.test

@@ -4,5 +4,5 @@ program mkmamedb
 arguments -o mamedb-test.db mamedb.dat
 file mamedb.dat mamedb-broken-sha1.dat
 stderr
-mamedb.dat:11: invalid argument for sha1
+mamedb.dat:11: invalid argument for sha1: '1234567890'
 end-of-inline-data

regress/mkmamedb-whitespace.test

@@ -0,0 +1,6 @@
+description game has data with leading/trailing whitespace inside the quotes
+return 0
+program mkmamedb
+args -o mamedb-test.db mamedb.xml
+file mamedb.xml mamedb-whitespace.xml mamedb-whitespace.xml
+file-new mamedb-test.db mamedb-whitespace.dump

regress/torrentzip-cache.test

@@ -0,0 +1,17 @@
+description test single-rom game (no parent), add missing rom, torrentzip it
+variants zip
+return 0
+args -D ../mamedb-reversesorted.db -Fjvc -e extra --use-torrentzip 2-48
+file-new roms/2-48.zip 2-48-ok.zip
+file-del extra/2-48.zip 2-48-ok.zip
+stdout-data
+In game 2-48:
+rom  08.rom        size       8  crc 3656897d: is in 'extra/2-48.zip/08.rom'
+rom  04.rom        size       4  crc d87f7e0c: is in 'extra/2-48.zip/04.rom'
+add 'extra/2-48.zip/08.rom' as '08.rom'
+add 'extra/2-48.zip/04.rom' as '04.rom'
+In archive extra/2-48.zip:
+delete used file '04.rom'
+delete used file '08.rom'
+remove empty archive
+end-of-data

regress/unknown-rename-failed.test

@@ -0,0 +1,19 @@
+description test copying file to garbage, failing (disk full) and not removing from set
+variants zip
+return 0
+setenv RENAME_ALWAYS_FAILS 1
+preload fwrite.so
+args -Fvcj -D ../mamedb-small.db
+file roms/unknown-file.zip unknown-file.zip
+stdout-data
+In game 1-4:
+game 1-4                                     : not a single file found
+In archive roms/unknown-file.zip:
+file unknown-file  size      21  crc f3b26226: unknown
+move unknown file 'unknown-file'
+remove empty archive
+end-of-data
+stderr-data
+unknown/unknown-file.zip: error closing zip: Renaming temporary file failed: Operation not permitted
+unknown/unknown-file.zip: error closing zip: Containing zip archive was closed
+end-of-data

src/ArchiveZip.cc

@@ -71,7 +71,7 @@ bool ArchiveZip::ensure_zip() {
 
     if (where == FILE_ROMSET && configuration.use_torrentzip) {
         if (zip_get_archive_flag(za, ZIP_AFL_IS_TORRENTZIP, ZIP_FL_UNCHANGED) == 0) {
-            cache_changed = true;
+            modified = true;
         }
         if (zip_set_archive_flag(za, ZIP_AFL_WANT_TORRENTZIP, 1) < 0) {
             output.error("can't torrentzip '%s'", name.c_str());
@@ -198,16 +198,50 @@ void ArchiveZip::commit_cleanup() {
 
     ensure_zip();
 
-    for (uint64_t i = 0; i < files.size(); i++) {
-	struct zip_stat st;
+    if (zip_get_archive_flag(za, ZIP_AFL_IS_TORRENTZIP, 0) == 1) {
+        auto names = std::unordered_map<std::string, size_t>{};
+        auto sorted_files = std::vector<File>{};
 
-	if (zip_stat_index(za, i, 0, &st) < 0) {
-	    output.set_error_archive(name);
-	    output.archive_error("cannot stat file %" PRIu64 ": %s", i, zip_strerror(za));
-	    continue;
-	}
+        auto index = size_t{0};
+        for (const auto& file: files) {
+            names[file.name] = index;
+            index += 1;
+        }
+
+        for (uint64_t i = 0; i < files.size(); i++) {
+            struct zip_stat st;
+
+            if (zip_stat_index(za, i, 0, &st) < 0) {
+                    output.set_error_archive(name);
+                    output.archive_error("cannot stat file %" PRIu64 ": %s", i, zip_strerror(za));
+                    continue;
+            }
+
+            auto it = names.find(st.name);
+            if (it == names.end()) {
+                    output.set_error_archive(name);
+                    output.archive_error("unexpected name %s in archive", st.name);
+                    continue;
+            }
+            sorted_files.push_back(files[names[st.name]]);
+
+            sorted_files[i].mtime = st.mtime;
+        }
 
-        files[i].mtime = st.mtime;
+        files = sorted_files;
+    }
+    else {
+        for (uint64_t i = 0; i < files.size(); i++) {
+            struct zip_stat st;
+
+            if (zip_stat_index(za, i, 0, &st) < 0) {
+                    output.set_error_archive(name);
+                    output.archive_error("cannot stat file %" PRIu64 ": %s", i, zip_strerror(za));
+                    continue;
+            }
+
+            files[i].mtime = st.mtime;
+        }
     }
 }
 

src/DatDb.h

@@ -35,6 +35,8 @@
 
 #include "DB.h"
 
+#include <optional>
+
 class DatDB;
 
 typedef std::shared_ptr<DatDB> DatDBPtr;

src/Hashes.cc

@@ -361,9 +361,19 @@ std::string Hashes::to_string(int type) const {
 }
 
 
-int Hashes::set_from_string(const std::string &s) {
+int Hashes::set_from_string(const std::string& s) {
     auto str = s;
 
+    /* remove leading & trailing whitespace */
+    auto data_start = str.find_first_not_of(" \t\n\r");
+    if (data_start != std::string::npos) {
+        str = str.substr(data_start);
+    }
+    auto data_end = str.find_last_not_of(" \t\n\r");
+    if (data_end != std::string::npos) {
+        str = str.substr(0, data_end + 1);
+    }
+
     if (str[0] == '0' && (str[1] == 'x' || str[1] == 'X')) {
         str = str.substr(2);
     }

src/Parser.cc

@@ -233,7 +233,7 @@ bool Parser::file_hash(filetype_t ft, int ht, const std::string& attr) {
     h = &r[ft]->hashes;
 
     if (h->set_from_string(attr) != ht) {
-        output.line_error(lineno, "invalid argument for %s", Hashes::type_name(ht).c_str());
+        output.line_error(lineno, "invalid argument for %s: '%s'", Hashes::type_name(ht).c_str(), attr.c_str());
         error = true;
         return false;
     }