Merge pull request #1844 from colsonrice/fix/gen2-save-refusal-message

Refuse a Gen 2 cart save as a Gen 2 save, not as a broken Gen 1 one

src/import/SaveFileIO.lua

@@ -78,6 +78,12 @@ function SaveFileIO.importToSlot(source, version, force)
   version = version or GameVersion.get()
   local bytes, readErr = readSource(source)
   if not bytes then return false, readErr end
+  -- The GAME decides before the BYTES do.  Everything below this line judges a
+  -- save by Gen 1's rules -- the size test, and mainChecksumValid, which is
+  -- pokered's checksum -- so a Gen 2 cart save reaching it is measured against
+  -- a rule that cannot match and comes back "checksum invalid" (#1832).
+  local supported, unsupportedWhy = SaveConvert.importSupported(version)
+  if not supported then return false, unsupportedWhy end
   if #bytes ~= SAVE_SIZE then
     local check = SaveConvert.mainChecksumValid(bytes)
     if check == nil then

src/save_convert/SaveConvert.lua

@@ -235,6 +235,35 @@ local function gen2CartName(gameVersion)
   return GameVersion.info(gameVersion).displayName
 end
 
+-- Can a cart save for this game cross in or out at all?  Public because the
+-- launcher has to ask about the GAME before it measures the BYTES.
+--
+-- SaveFileIO.importToSlot judges anything that is not exactly SAVE_SIZE with
+-- mainChecksumValid, which is pokered's main-data checksum.  A Gen 2 cart is
+-- MBC3+TIMER, so a real Gold/Silver/Crystal .sav carries an RTC footer and is
+-- 32786 bytes: it misses the size test and is then measured against a rule
+-- that was never going to match it.  The player is told "save data checksum
+-- invalid" about a save that is perfectly good (#1832).
+--
+-- Returns true, or false plus the same sentence importSav/exportSav would have
+-- answered with, so a caller that asks early and a caller that does not cannot
+-- describe the same game two different ways.
+function SaveConvert.importSupported(gameVersion)
+  local gen2Name = gen2CartName(gameVersion)
+  if gen2Name then
+    return false, gen2Name .. " uses a Gen 2 cart save; importing one is not supported yet."
+  end
+  return true
+end
+
+function SaveConvert.exportSupported(gameVersion)
+  local gen2Name = gen2CartName(gameVersion)
+  if gen2Name then
+    return false, gen2Name .. " uses a Gen 2 cart save; exporting one is not supported yet."
+  end
+  return true
+end
+
 -- importSav(bytes, version, gameVersion) -> saveTable, err
 -- bytes: the raw 32768-byte SRAM string. Validates size and the main-data
 -- checksum, decodes through GenSave, and returns a save table fully merged
@@ -247,10 +276,8 @@ function SaveConvert.importSav(bytes, version, gameVersion)
   if type(bytes) ~= "string" then
     return nil, "expected raw save bytes as a string"
   end
-  local gen2Name = gen2CartName(gameVersion)
+  local supported, unsupportedWhy = SaveConvert.importSupported(gameVersion)
-  if gen2Name then
+  if not supported then return nil, unsupportedWhy end
-    return nil, gen2Name .. " uses a Gen 2 cart save; importing one is not supported yet."
-  end
   if #bytes ~= GenSave.SAVE_SIZE then
     return nil, ("save must be %d bytes, got %d"):format(GenSave.SAVE_SIZE, #bytes)
   end
@@ -283,10 +310,8 @@ function SaveConvert.exportSav(saveTable, gameVersion)
   if type(saveTable) ~= "table" then
     return nil, "expected a save table"
   end
-  local gen2Name = gen2CartName(gameVersion)
+  local supported, unsupportedWhy = SaveConvert.exportSupported(gameVersion)
-  if gen2Name then
+  if not supported then return nil, unsupportedWhy end
-    return nil, gen2Name .. " uses a Gen 2 cart save; exporting one is not supported yet."
-  end
   local data, derr = ensureData(gameVersion)
   if not data then return nil, derr end
   local ok, bytes = pcall(GenSave.encode, saveTable, data, nil)

tests/engine/save_import_retry_bug420.lua

@@ -221,6 +221,11 @@ do
   local seen = {}
   package.loaded["src.save_convert.SaveConvert"] = {
     SAVE_SIZE = 32768,
+    -- importToSlot asks this before it measures the bytes, so a save for a
+    -- game with no codec is refused as that rather than as a bad checksum.
+    -- The double has to answer it; "yes" is what keeps this case about the
+    -- cache-name contract below and nothing else.
+    importSupported = function() return true end,
     importSav = function(_, version, gameVersion)
       seen.import = { version = version, gameVersion = gameVersion }
       return nil, "stub"

tests/gen2_save_import_message_test.lua

@@ -0,0 +1,96 @@
+-- A Gen 2 cart save must be refused as a Gen 2 cart save, not as a corrupt
+-- Gen 1 one (#1832).
+--   luajit tests/gen2_save_import_message_test.lua
+-- Also dofile'd by tests/run_tests.lua.
+--
+-- SaveFileIO.importToSlot judges anything that is not exactly SAVE_SIZE with
+-- SaveConvert.mainChecksumValid, which is pokered's main-data checksum.  Gen 2
+-- carts are MBC3+TIMER, so a real Gold/Silver/Crystal battery save carries an
+-- RTC footer and is 32786 bytes: it misses the size test, is then measured
+-- against a checksum rule written for a different generation, and the launcher
+-- tells the player their save is corrupt.  It is not -- there is simply no Gen
+-- 2 codec yet, which is a different sentence and an actionable one.
+package.path = "./?.lua;./?/init.lua;" .. package.path
+
+love = love or require("tests.love_stub")
+
+local S = require("tests.harness").suite("gen2 save import message")
+local check = S.check
+
+local SaveConvert = require("src.save_convert.SaveConvert")
+local SaveFileIO = require("src.import.SaveFileIO")
+
+-- The size a real Gen 2 cart save actually is: 32768 bytes of SRAM plus the
+-- 18-byte RTC footer an MBC3+TIMER cart writes.
+local GEN2_CART_SAVE_SIZE = 32786
+
+local function blob(n) return string.rep("\0", n) end
+
+-- readSource only takes a raw string when it is EXACTLY 32768 bytes; anything
+-- else is treated as a picker path (its own comment says so).  A real Gen 2
+-- cart save is 32786, so it can only ever reach importToSlot as a FILE -- which
+-- is exactly how the player in #1832 supplied theirs.  Write one and hand over
+-- the path, so this exercises the route the report came from.
+local function savFile(n)
+  local path = os.tmpname()
+  local f = assert(io.open(path, "wb"))
+  f:write(blob(n))
+  f:close()
+  return path
+end
+
+-- ------------------------------------------------------------------
+-- The report: a real Gen 2 save is not "checksum invalid"
+-- ------------------------------------------------------------------
+
+for _, version in ipairs({ "gold", "silver", "crystal" }) do
+  local ok, err = SaveFileIO.importToSlot(savFile(GEN2_CART_SAVE_SIZE), version, true)
+  check(ok == false, version .. ": a Gen 2 cart save is still refused")
+  check(type(err) == "string" and err:find("Gen 2 cart save", 1, true) ~= nil,
+    version .. ": refused AS a Gen 2 save -- got: " .. tostring(err))
+  check(type(err) == "string" and err:find("checksum", 1, true) == nil,
+    version .. ": never blamed on a checksum it was never measured by -- got: "
+      .. tostring(err))
+end
+
+-- ------------------------------------------------------------------
+-- The predicate both callers share
+-- ------------------------------------------------------------------
+
+for _, version in ipairs({ "red", "blue", "yellow" }) do
+  check(SaveConvert.importSupported(version) == true,
+    version .. ": Gen 1 import is unaffected")
+  check(SaveConvert.exportSupported(version) == true,
+    version .. ": Gen 1 export is unaffected")
+end
+
+for _, version in ipairs({ "gold", "silver", "crystal" }) do
+  local impOk, impWhy = SaveConvert.importSupported(version)
+  local expOk, expWhy = SaveConvert.exportSupported(version)
+  check(impOk == false and expOk == false, version .. ": both directions say no")
+  -- One sentence per direction, wherever it is asked from: the early gate in
+  -- SaveFileIO and the late one inside importSav must not describe the same
+  -- game two different ways.
+  local _, lateWhy = SaveConvert.importSav(blob(32768), version, version)
+  check(impWhy == lateWhy,
+    version .. ": the early gate and importSav answer identically")
+  check(expWhy:find("exporting", 1, true) ~= nil,
+    version .. ": the export sentence is about exporting")
+end
+
+-- ------------------------------------------------------------------
+-- Gen 1 keeps its own diagnosis
+-- ------------------------------------------------------------------
+--
+-- A Gen 1 save that really is the wrong size AND fails pokered's checksum must
+-- still say so: this fix moves the generation check in front of that test, it
+-- does not remove it.
+
+do
+  local ok, err = SaveFileIO.importToSlot(savFile(GEN2_CART_SAVE_SIZE), "red", true)
+  check(ok == false, "red: a corrupt oversize save is still refused")
+  check(type(err) == "string" and err:find("checksum", 1, true) ~= nil,
+    "red: still diagnosed by pokered's checksum -- got: " .. tostring(err))
+end
+
+S.finish()

tests/run_tests.lua

@@ -3616,6 +3616,7 @@ runSuites(orderedGlob(
   -- name resolution, and the .sav converter refusing a Gen 2 save table.
   "tests/gen2_sound_alias_test.lua",
   "tests/gen2_save_convert_cli_test.lua",
+  "tests/gen2_save_import_message_test.lua",
   -- The wall radios (`special MapRadio`).  gen2_save_export_test cannot share a
   -- process (LEAKS_SAVE_SLOT_STATE above); tests/run_gen2.lua runs it alone.
   "tests/gen2_map_radio_test.lua",