Date: Mon, 9 Nov 2009 14:17:28 -0800
From: Stefan O'Rear <stefanor@cox.net>
To: crawl-ref-discuss@lists.sourceforge.net
Message-ID: <20091109221728.GA7668@localhost.localdomain>
Subject: [Crawl-ref-discuss] Savefile compatibility: how and why

Robert asked me to write something for the list on the subject of
savefile compatibility, so here goes.

1. WHY

Having a savefile compatibility procedure is crucial in the stable
branches, as it allows applying bug fix releases in mid-game.  It's
also important in the development trees, although we currently give
it far less attention than it deserves.  The reason for this is that
properly testing many features, such as the recent portal refactors
that happened to make leaving Zot impossible, require a long game.
If it is impossible to upgrade in the middle of a game, then all bugs
for late-game features will be reported against old versions, need-
lessly complicating the bug fixing process.

The situation is exacerbated by the public beta server (Marc Thoben's
crawl.develz.org), since it can only be updated site-wide; it cannot
be updated between games as there are almost always saved games on
the server, so it is simply updated (with loss of saves) rarely.  And
rarely is bad.

2. HOW

If you are changing any of the code in tags.cc (which implements load
and save for savefiles), you probably need to be concerned about save
compatibility.  Any code which uses the marshall* and unmarshall*
functions is, likewise, probably unsafe to simply change.  Changing
the values of existing enums (for instance, adding a new option
before existing options) is not generally safe, as saves store the
numeric values of enum variables.

By way of example, suppose you wanted to add a new field to the player
structure, you.xyzzy.  Now you want to save this field, and without
breaking saves.  To do this, find the functions tag_read_you and
tag_construct_you in tags.cc.  Unfortunately, you can't just add
marshall and unmarshall calls, since if you get an old save, the wrong
value will be unmarshalled, synchronization is lost and things
fall apart.  So first, add a new option to the tag_minor_version enum
in tags.h.  Make sure the new option is at the end, and that you
update TAG_MINOR_VERSION to correspond to the new option.  Now, any
savefile created by the new code, or any later savefile, will have a
minor version >= your new version number.  So, make the marshall and
unmarshall code conditional on minorVersion, like so:

    if (minorVersion >= TAG_MINOR_JIYVA)
        you.second_god_name = unmarshallString(th);

...

    marshallString(th, you.second_god_name);

Is this clear?  If you want to change the representation of an existing
field, or delete an old field, use a check in the other direction for
saves created by *older* versions of Crawl; for instance, take this
code, which reads in either a Subversion or a Git revision number,
depending on version:

    if (minorVersion <  TAG_MINOR_GITREV)
    {
        std::string rev_str = unmarshallString(th);
        int rev_int = unmarshallLong(th);

        UNUSED(rev_str);
        UNUSED(rev_int);
    }

    if (minorVersion >= TAG_MINOR_GITREV)
    {
        std::string rev_str = unmarshallString(th);
        UNUSED(rev_str);
    }

If you want to change an enum without breaking savefile compatibility,
the cardinal rule is that the numeric values of all existing constants
must not change.  If you are adding an option, add it at the end; if
you are removing a value, leave a placeholder of some kind.  Be sure
to test your code well, as there are sometimes obscure requirements on
what needs to be done for a placeholder.  Case in point: if a
MUT_UNUSED_n placeholder exists in the mutation_type enumeration, but
no mutation definition exists in mutation_defs, it will work fine until
somebody plays with a Vampire, as generating the list of fully active
mutations requires looking at mutation definitions for all mutations.

In extreme cases, it may not be possible to preserve load compatibility.
If this is the case, and please do this sparingly, you should increase
TAG_MAJOR_VERSION.  This will ensure that old save files are correctly
rejected, instead of causing a crash.

Historically TAG_MAJOR_VERSION has always been equal to the submajor
version of Crawl itself; however, this buys us very little, as users
do not need to identify Crawl save versions by contents under any
normal circumstance, and has cost us dearly in time spent debugging
startup crashes.  Therefore we should not do it; TAG_MAJOR_VERSION
should be incremented on all incompatible changes.

When TAG_MAJOR_VERSION is incremented, all existing TAG_MINOR_ checks
are no longer necessary and can be removed, along with deleting the
values of TAG_MINOR_ and restarting minor numbering from 1.

Thank you for listening, I hope this was useful.

Stefan