Merge pull request #575 from ennorehling/develop

remove_unit called by add_recruits crashed the preview

scripts/tests/common.lua

@@ -1139,3 +1139,16 @@ function test_prefix()
     u1.faction.locale = "en"
     assert_not_nil(u1:show():find("archelf"))
 end
+
+function test_recruit()
+    local r = region.create(0, 0, "plain")
+    local f = faction.create("noreply@eressea.de", "human", "de")
+    local u = unit.create(f, r, 1)
+
+    u:add_item("money", 1000)
+    set_order(u, "REKRUTIERE 5")
+    process_orders()
+    for u in f.units do
+        assert_equal(6, u.number)
+    end
+end

src/economy.c

@@ -244,7 +244,7 @@ static recruitment *select_recruitment(request ** rop,
     return recruits;
 }
 
-static void add_recruits(unit * u, int number, int wanted)
+void add_recruits(unit * u, int number, int wanted)
 {
     region *r = u->region;
     assert(number <= wanted);

src/economy.h

@@ -61,7 +61,7 @@ extern "C" {
     void give_control(struct unit * u, struct unit * u2);
     void tax_cmd(struct unit * u, struct order *ord, struct request ** taxorders);
     void expandtax(struct region * r, struct request * taxorders);
-
+    void add_recruits(struct unit * u, int number, int wanted);
     struct message * check_steal(const struct unit * u, struct order *ord);
 
 #ifdef __cplusplus

src/economy.test.c

@@ -290,6 +290,22 @@ static void test_maintain_buildings(CuTest *tc) {
     test_cleanup();
 }
 
+static void test_recruit(CuTest *tc) {
+    unit *u;
+    faction *f;
+
+    test_setup();
+    f = test_create_faction(0);
+    u = test_create_unit(f, test_create_region(0, 0, 0));
+    CuAssertIntEquals(tc, 1, u->number);
+    add_recruits(u, 1, 1);
+    CuAssertIntEquals(tc, 2, u->number);
+    CuAssertPtrEquals(tc, u, f->units);
+    CuAssertPtrEquals(tc, NULL, u->nextF);
+    CuAssertPtrEquals(tc, NULL, u->prevF);
+    test_cleanup();
+}
+
 CuSuite *get_economy_suite(void)
 {
     CuSuite *suite = CuSuiteNew();
@@ -302,5 +318,6 @@ CuSuite *get_economy_suite(void)
     SUITE_ADD_TEST(suite, test_heroes_dont_recruit);
     SUITE_ADD_TEST(suite, test_tax_cmd);
     SUITE_ADD_TEST(suite, test_maintain_buildings);
+    SUITE_ADD_TEST(suite, test_recruit);
     return suite;
 }

src/kernel/unit.c

@@ -439,6 +439,9 @@ int remove_unit(unit ** ulist, unit * u)
         *ulist = u->next;
     }
 
+    if (u->faction && u->faction->units == u) {
+        u->faction->units = u->nextF;
+    }
     if (u->prevF) {
         u->prevF->nextF = u->nextF;
     }

src/kernel/unit.test.c

@@ -405,7 +405,7 @@ static void test_unit_description(CuTest *tc) {
 
 static void test_remove_unit(CuTest *tc) {
     region *r;
-    unit *u;
+    unit *u1, *u2;
     faction *f;
     int uno;
     const resource_type *rtype;
@@ -415,19 +415,42 @@ static void test_remove_unit(CuTest *tc) {
     rtype = get_resourcetype(R_SILVER);
     r = test_create_region(0, 0, 0);
     f = test_create_faction(0);
-    u = test_create_unit(f, r);
-    uno = u->no;
+    u2 = test_create_unit(f, r);
+    u1 = test_create_unit(f, r);
+    CuAssertPtrEquals(tc, u1, f->units);
+    CuAssertPtrEquals(tc, u2, u1->nextF);
+    CuAssertPtrEquals(tc, u1, u2->prevF);
+    CuAssertPtrEquals(tc, 0, u2->nextF);
+    uno = u1->no;
     region_setresource(r, rtype, 0);
-    i_change(&u->items, rtype->itype, 100);
-    remove_unit(&r->units, u);
-    CuAssertIntEquals(tc, 100, region_getresource(r, rtype));
-    CuAssertIntEquals(tc, 0, u->number);
-    CuAssertPtrEquals(tc, 0, u->region);
-    CuAssertPtrEquals(tc, 0, u->items);
-    CuAssertPtrEquals(tc, 0, u->nextF);
-    CuAssertPtrEquals(tc, 0, r->units);
+    i_change(&u1->items, rtype->itype, 100);
+    remove_unit(&r->units, u1);
+    CuAssertIntEquals(tc, 0, u1->number);
+    CuAssertPtrEquals(tc, 0, u1->region);
+    // money is given to a survivor:
+    CuAssertPtrEquals(tc, 0, u1->items);
+    CuAssertIntEquals(tc, 0, region_getresource(r, rtype));
+    CuAssertIntEquals(tc, 100, i_get(u2->items, rtype->itype));
+
+    // unit is removed from f->units:
+    CuAssertPtrEquals(tc, 0, u1->nextF);
+    CuAssertPtrEquals(tc, u2, f->units);
+    CuAssertPtrEquals(tc, 0, u2->nextF);
+    CuAssertPtrEquals(tc, 0, u2->prevF);
+    // unit is no longer in r->units:
+    CuAssertPtrEquals(tc, u2, r->units);
+    CuAssertPtrEquals(tc, 0, u2->next);
+
+    // unit is in deleted_units:
     CuAssertPtrEquals(tc, 0, findunit(uno));
     CuAssertPtrEquals(tc, f, dfindhash(uno));
+
+    remove_unit(&r->units, u2);
+    // no survivor, give money to peasants:
+    CuAssertIntEquals(tc, 100, region_getresource(r, rtype));
+    // there are now no more units:
+    CuAssertPtrEquals(tc, 0, r->units);
+    CuAssertPtrEquals(tc, 0, f->units);
     test_cleanup();
 }