fix write_faction_reference, check for f->alive in other places

TODO:
1. I added some new TODOs
2. What happens to morale when region owners die?
3. Needs tests

src/kernel/faction.c

@@ -333,7 +333,8 @@ variant read_faction_reference(struct storage * store)
 
 void write_faction_reference(const faction * f, struct storage *store)
 {
-    WRITE_INT(store, (f && f->_alive) ? f->no : 0);
+    assert(!f || f->_alive);
+    WRITE_INT(store, f ? f->no : 0);
 }
 
 static faction *dead_factions;

src/kernel/group.c

@@ -203,12 +203,12 @@ void write_groups(struct storage *store, const faction * f)
         WRITE_INT(store, g->gid);
         WRITE_STR(store, g->name);
         for (a = g->allies; a; a = a->next) {
-            if (a->faction) {
+            if (a->faction && a->faction->_alive) {
                 write_faction_reference(a->faction, store);
                 WRITE_INT(store, a->status);
             }
         }
-        WRITE_INT(store, 0);
+        write_faction_reference(NULL, store);
         a_write(store, g->attribs, g);
         WRITE_SECTION(store);
     }
@@ -233,7 +233,7 @@ void read_groups(struct storage *store, faction * f)
             ally *a;
             variant fid;
 
-            READ_INT(store, &fid.i);
+            fid = read_faction_reference(store);
             if (fid.i <= 0)
                 break;
             a = ally_add(pa, findfaction(fid.i));

src/kernel/group.test.c

@@ -5,6 +5,8 @@
 #include "faction.h"
 #include "unit.h"
 #include "region.h"
+#include <util/attrib.h>
+#include <attributes/key.h>
 #include <stream.h>
 #include <filestream.h>
 #include <storage.h>
@@ -22,34 +24,47 @@ static void test_group_readwrite(CuTest * tc)
     storage store;
     FILE *F;
     stream strm;
+    int i;
 
-    F = fopen("test.dat", "w");
+    F = fopen("test.dat", "wb");
     fstream_init(&strm, F);
     binstore_init(&store, &strm);
     test_cleanup();
     test_create_world();
     f = test_create_faction(0);
-    g = new_group(f, "test", 42);
+    g = new_group(f, "NW", 42);
+    g = new_group(f, "Egoisten", 43);
+    a_add(&g->attribs, make_key(44));
     al = ally_add(&g->allies, f);
     al->status = HELP_GIVE;
     write_groups(&store, f);
+    WRITE_INT(&store, 47);
     binstore_done(&store);
     fstream_done(&strm);
 
-    F = fopen("test.dat", "r");
+    F = fopen("test.dat", "rb");
     fstream_init(&strm, F);
     binstore_init(&store, &strm);
     f->groups = 0;
-    free_group(g);
     read_groups(&store, f);
+    READ_INT(&store, &i);
     binstore_done(&store);
     fstream_done(&strm);
 
+    CuAssertIntEquals(tc, 47, i);
     CuAssertPtrNotNull(tc, f->groups);
-    CuAssertPtrNotNull(tc, f->groups->allies);
+    CuAssertIntEquals(tc, 42, f->groups->gid);
-    CuAssertPtrEquals(tc, 0, f->groups->allies->next);
+    CuAssertStrEquals(tc, "NW", f->groups->name);
-    CuAssertPtrEquals(tc, f, f->groups->allies->faction);
+    CuAssertPtrNotNull(tc, f->groups->next);
-    CuAssertIntEquals(tc, HELP_GIVE, f->groups->allies->status);
+    CuAssertIntEquals(tc, 43, f->groups->next->gid);
+    CuAssertStrEquals(tc, "Egoisten", f->groups->next->name);
+    CuAssertPtrEquals(tc, 0, f->groups->allies);
+    g = f->groups->next;
+    CuAssertPtrNotNull(tc, find_key(g->attribs, 44));
+    CuAssertPtrNotNull(tc, g->allies);
+    CuAssertPtrEquals(tc, 0, g->allies->next);
+    CuAssertPtrEquals(tc, f, g->allies->faction);
+    CuAssertIntEquals(tc, HELP_GIVE, g->allies->status);
     remove("test.dat");
     test_cleanup();
 }

src/kernel/save.c

@@ -454,6 +454,7 @@ void write_alliances(struct gamedata *data)
     alliance *al = alliances;
     while (al) {
         if (al->_leader) {
+            assert(al->_leader->_alive);
             WRITE_INT(data->store, al->id);
             WRITE_STR(data->store, al->name);
             WRITE_INT(data->store, (int)al->flags);
@@ -462,7 +463,7 @@ void write_alliances(struct gamedata *data)
         }
         al = al->next;
     }
-    WRITE_INT(data->store, 0);
+    write_faction_reference(NULL, data->store);
     WRITE_SECTION(data->store);
 }
 
@@ -535,10 +536,14 @@ static void read_owner(struct gamedata *data, region_owner ** powner)
 static void write_owner(struct gamedata *data, region_owner * owner)
 {
     if (owner) {
+        faction *f;
         WRITE_INT(data->store, owner->since_turn);
         WRITE_INT(data->store, owner->morale_turn);
         WRITE_INT(data->store, owner->flags);
-        write_faction_reference(owner->last_owner, data->store);
+        f = owner->last_owner;
+        write_faction_reference((f && f->_alive) ? f : NULL, data->store);
+        // TODO: check that destroyfaction does the right thing.
+        // TODO: What happens to morale when the owner dies?
         write_faction_reference(owner->owner, data->store);
     }
     else {
@@ -612,8 +617,8 @@ unit *read_unit(struct gamedata *data)
         uhash(u);
     }
 
-    READ_INT(data->store, &n);
+    resolve_faction(read_faction_reference(data->store), &f);
-    f = findfaction(n);
+    assert(f);
     if (f != u->faction) {
         u_setfaction(u, f);
     }
@@ -1162,14 +1167,14 @@ faction *readfaction(struct gamedata * data)
 {
     ally **sfp;
     int planes, n;
-    faction *f;
+    faction *f = NULL;
     char name[DISPLAYSIZE];
+    variant var;
 
-    READ_INT(data->store, &n);
+    resolve_faction(var = read_faction_reference(data->store), &f);
-    f = findfaction(n);
     if (f == NULL) {
         f = (faction *)calloc(1, sizeof(faction));
-        f->no = n;
+        f->no = var.i;
     }
     else {
         f->allies = NULL;           /* mem leak */
@@ -1354,20 +1359,18 @@ void writefaction(struct gamedata *data, const faction * f)
     WRITE_SECTION(data->store);
 
     for (sf = f->allies; sf; sf = sf->next) {
-        int no;
+        faction *fa = sf->faction;
-        int status;
-
-        assert(sf->faction);
-
-        no = sf->faction->no;
-        status = alliedfaction(NULL, f, sf->faction, HELP_ALL);
 
+        assert(fa);
+        if (fa->_alive) {
+            int status = alliedfaction(NULL, f, fa, HELP_ALL);
             if (status != 0) {
-            WRITE_INT(data->store, no);
+                write_faction_reference(fa, data->store);
                 WRITE_INT(data->store, sf->status);
             }
         }
-    WRITE_INT(data->store, 0);
+    }
+    write_faction_reference(NULL, data->store);
     WRITE_SECTION(data->store);
     write_groups(data->store, f);
     write_spellbook(f->spellbook, data->store);
@@ -1381,6 +1384,32 @@ static int cb_sb_maxlevel(spellbook_entry *sbe, void *cbdata) {
     return 0;
 }
 
+void write_watchers(storage *store, plane *pl) {
+    watcher *w = pl->watchers;
+    while (w) {
+        if (w->faction && w->faction->_alive) {
+            write_faction_reference(w->faction, store);
+            WRITE_INT(store, w->mode);
+        }
+        w = w->next;
+    }
+    write_faction_reference(NULL, store);       /* mark the end of the list */
+}
+
+void read_watchers(storage *store, plane *pl) {
+    variant fno = read_faction_reference(store);
+    while (fno.i) {
+        int n;
+        watcher *w = (watcher *)malloc(sizeof(watcher));
+        ur_add(fno, &w->faction, resolve_faction);
+        READ_INT(store, &n);
+        w->mode = (unsigned char)n;
+        w->next = pl->watchers;
+        pl->watchers = w;
+        fno = read_faction_reference(store);
+    }
+}
+
 int readgame(const char *filename, bool backup)
 {
     int n, p, nread;
@@ -1463,7 +1492,6 @@ int readgame(const char *filename, bool backup)
     READ_INT(&store, &nread);
     while (--nread >= 0) {
         int id;
-        variant fno;
         plane *pl;
 
         READ_INT(&store, &id);
@@ -1500,16 +1528,7 @@ int readgame(const char *filename, bool backup)
             }
         }
         else {
-            fno = read_faction_reference(&store);
+            read_watchers(&store, pl);
-            while (fno.i) {
-                watcher *w = (watcher *)malloc(sizeof(watcher));
-                ur_add(fno, &w->faction, resolve_faction);
-                READ_INT(&store, &n);
-                w->mode = (unsigned char)n;
-                w->next = pl->watchers;
-                pl->watchers = w;
-                fno = read_faction_reference(&store);
-            }
         }
         a_read(&store, &pl->attribs, pl);
         if (pl->id != 1094969858) { // Regatta
@@ -1789,7 +1808,6 @@ int writegame(const char *filename)
     WRITE_SECTION(&store);
 
     for (pl = planes; pl; pl = pl->next) {
-        watcher *w;
         WRITE_INT(&store, pl->id);
         WRITE_STR(&store, pl->name);
         WRITE_INT(&store, pl->minx);
@@ -1797,15 +1815,7 @@ int writegame(const char *filename)
         WRITE_INT(&store, pl->miny);
         WRITE_INT(&store, pl->maxy);
         WRITE_INT(&store, pl->flags);
-        w = pl->watchers;
+        write_watchers(&store, pl);
-        while (w) {
-            if (w->faction) {
-                write_faction_reference(w->faction, &store);
-                WRITE_INT(&store, w->mode);
-            }
-            w = w->next;
-        }
-        write_faction_reference(NULL, &store);       /* mark the end of the list */
         a_write(&store, pl->attribs, pl);
         WRITE_SECTION(&store);
     }