fix new harbor code, add tests.

- alliances were checked in the opposite direction
- code crashed when harbor has no owner

res/core/de/strings.xml

@@ -4754,6 +4754,11 @@
       Der Landstrich kann über Jahre hinaus von den Folgen einer 
       solchen Dürre betroffen sein.
       </text>
+      <text locale="en">This powerful ritual opens a gate to the elemental plane of 
+      fire. A great drought comes over the land. Farmers, animals and 
+      plants of the region are fighting for survival, but only half of 
+      all living things will be able to survive a drought like this. 
+      The region will suffer the consequences of such a drought for years to come.</text>
     </string>
     <string name="magic_roots">
       <text locale="de">

src/move.c

@@ -696,10 +696,14 @@ int check_ship_allowed(struct ship *sh, const region * r)
     }
 
     if (bt_harbour && buildingtype_exists(r, bt_harbour, true)) {
-        unit* hafenmeister = NULL;
+        unit* harbourmaster = NULL;
-        hafenmeister = owner_buildingtyp(r, bt_harbour);
+        harbourmaster = owner_buildingtyp(r, bt_harbour);
-        if ((sh->_owner->faction == hafenmeister->faction) || (ucontact(sh->_owner, hafenmeister)) || (alliedunit(sh->_owner, hafenmeister->faction, HELP_GUARD)))
+        if (!harbourmaster || !sh->_owner) {
             return SA_HARBOUR;
+        }
+        else if ((sh->_owner->faction == harbourmaster->faction) || (ucontact(harbourmaster, sh->_owner)) || (alliedunit(harbourmaster, sh->_owner->faction, HELP_GUARD))) {
+            return SA_HARBOUR;
+        }
     }
     if (fval(r->terrain, SEA_REGION)) {
         return SA_COAST;
@@ -2060,7 +2064,7 @@ sail(unit * u, order * ord, bool move_on_land, region_list ** routep)
      * Inland zu segeln versuchte */
 
     if (sh != NULL && fval(sh, SF_MOVED)) {
-        unit *hafenmeister;
+        unit *harbourmaster;
         /* nachdem alle Richtungen abgearbeitet wurden, und alle Einheiten
          * transferiert wurden, kann der aktuelle Befehl gelöscht werden. */
         cycle_route(ord, u, step);
@@ -2089,25 +2093,25 @@ sail(unit * u, order * ord, bool move_on_land, region_list ** routep)
 
         /* Hafengebühren ? */
 
-        hafenmeister = owner_buildingtyp(current_point, bt_find("harbour"));
+        harbourmaster = owner_buildingtyp(current_point, bt_find("harbour"));
-        if (sh && hafenmeister != NULL) {
+        if (sh && harbourmaster != NULL) {
             item *itm;
             unit *u2;
             item *trans = NULL;
 
             for (u2 = current_point->units; u2; u2 = u2->next) {
-                if (u2->ship == sh && !alliedunit(hafenmeister, u->faction, HELP_GUARD)) {
+                if (u2->ship == sh && !alliedunit(harbourmaster, u->faction, HELP_GUARD)) {
 
-                    if (effskill(hafenmeister, SK_PERCEPTION) > effskill(u2, SK_STEALTH)) {
+                    if (effskill(harbourmaster, SK_PERCEPTION) > effskill(u2, SK_STEALTH)) {
                         for (itm = u2->items; itm; itm = itm->next) {
                             const luxury_type *ltype = resource2luxury(itm->type->rtype);
                             if (ltype != NULL && itm->number > 0) {
-                                int st = itm->number * effskill(hafenmeister, SK_TRADE) / 50;
+                                int st = itm->number * effskill(harbourmaster, SK_TRADE) / 50;
                                 st = _min(itm->number, st);
 
                                 if (st > 0) {
                                     i_change(&u2->items, itm->type, -st);
-                                    i_change(&hafenmeister->items, itm->type, st);
+                                    i_change(&harbourmaster->items, itm->type, st);
                                     i_add(&trans, i_new(itm->type, st));
                                 }
                             }
@@ -2117,10 +2121,10 @@ sail(unit * u, order * ord, bool move_on_land, region_list ** routep)
             }
             if (trans) {
                 message *msg =
-                    msg_message("harbor_trade", "unit items ship", hafenmeister, trans,
+                    msg_message("harbor_trade", "unit items ship", harbourmaster, trans,
                     u->ship);
                 add_message(&u->faction->msgs, msg);
-                add_message(&hafenmeister->faction->msgs, msg);
+                add_message(&harbourmaster->faction->msgs, msg);
                 msg_release(msg);
                 while (trans)
                     i_remove(&trans, trans);

src/move.test.c

@@ -3,10 +3,13 @@
 #include <stdlib.h>
 #include "move.h"
 
+#include <kernel/ally.h>
 #include <kernel/building.h>
+#include <kernel/faction.h>
 #include <kernel/region.h>
 #include <kernel/ship.h>
 #include <kernel/terrain.h>
+#include <kernel/unit.h>
 
 #include <util/language.h>
 
@@ -15,77 +18,165 @@
 
 static void test_ship_not_allowed_in_coast(CuTest * tc)
 {
-  region *r1, *r2;
+    region *r1, *r2;
-  ship * sh;
+    ship * sh;
-  terrain_type *ttype, *otype;
+    terrain_type *ttype, *otype;
-  ship_type *stype;
+    ship_type *stype;
 
-  test_cleanup();
+    test_cleanup();
-  test_create_world();
+    test_create_world();
 
-  ttype = test_create_terrain("glacier", LAND_REGION|ARCTIC_REGION|WALK_INTO|SAIL_INTO);
+    ttype = test_create_terrain("glacier", LAND_REGION | ARCTIC_REGION | WALK_INTO | SAIL_INTO);
-  otype = test_create_terrain("ocean", SEA_REGION|SAIL_INTO);
+    otype = test_create_terrain("ocean", SEA_REGION | SAIL_INTO);
-  stype = test_create_shiptype("derp");
+    stype = test_create_shiptype("derp");
-  stype->coasts = (const struct terrain_type **)calloc(2, sizeof(const struct terrain_type *));
+    stype->coasts = (const struct terrain_type **)calloc(2, sizeof(const struct terrain_type *));
 
-  r1 = test_create_region(0, 0, ttype);
+    r1 = test_create_region(0, 0, ttype);
-  r2 = test_create_region(1, 0, otype);
+    r2 = test_create_region(1, 0, otype);
-  sh = test_create_ship(0, stype);
+    sh = test_create_ship(0, stype);
 
-  CuAssertIntEquals(tc, SA_COAST, check_ship_allowed(sh, r2));
+    CuAssertIntEquals(tc, SA_COAST, check_ship_allowed(sh, r2));
-  CuAssertIntEquals(tc, SA_NO_COAST, check_ship_allowed(sh, r1));
+    CuAssertIntEquals(tc, SA_NO_COAST, check_ship_allowed(sh, r1));
-  stype->coasts[0] = ttype;
+    stype->coasts[0] = ttype;
-  CuAssertIntEquals(tc, SA_COAST, check_ship_allowed(sh, r1));
+    CuAssertIntEquals(tc, SA_COAST, check_ship_allowed(sh, r1));
 }
 
-static void test_ship_allowed_with_harbor(CuTest * tc)
+typedef struct move_fixture {
+    region *r;
+    ship *sh;
+    building * b;
+    unit *u;
+} move_fixture;
+
+static void setup_harbor(move_fixture *mf) {
+    region *r;
+    ship * sh;
+    terrain_type * ttype;
+    building_type * btype;
+    building * b;
+    unit *u;
+
+    test_cleanup();
+    test_create_world();
+
+    ttype = test_create_terrain("glacier", LAND_REGION | ARCTIC_REGION | WALK_INTO | SAIL_INTO);
+    btype = test_create_buildingtype("harbour");
+
+    sh = test_create_ship(0, 0);
+    r = test_create_region(0, 0, ttype);
+
+    b = test_create_building(r, btype);
+    b->flags |= BLD_WORKING;
+
+    u = test_create_unit(test_create_faction(0), r);
+    u->ship = sh;
+    ship_set_owner(u);
+
+    mf->r = r;
+    mf->u = u;
+    mf->sh = sh;
+    mf->b = b;
+}
+
+static void test_ship_allowed_without_harbormaster(CuTest * tc)
 {
-  region *r;
+    move_fixture mf;
-  ship * sh;
-  terrain_type * ttype;
-  building_type * btype;
-  building * b;
 
-  test_cleanup();
+    setup_harbor(&mf);
-  test_create_world();
 
-  ttype = test_create_terrain("glacier", LAND_REGION|ARCTIC_REGION|WALK_INTO|SAIL_INTO);
+    CuAssertIntEquals(tc, SA_HARBOUR, check_ship_allowed(mf.sh, mf.r));
-  btype = test_create_buildingtype("harbour");
+}
 
-  r = test_create_region(0, 0, ttype);
+static void test_ship_blocked_by_harbormaster(CuTest * tc) {
-  sh = test_create_ship(0, 0);
+    unit *u;
+    move_fixture mf;
 
-  b = test_create_building(r, btype);
+    setup_harbor(&mf);
-  b->flags |= BLD_WORKING;
+
-  CuAssertIntEquals(tc, SA_HARBOUR, check_ship_allowed(sh, r));
+    u = test_create_unit(test_create_faction(0), mf.r);
+    u->building = mf.b;
+    building_set_owner(u);
+
+    CuAssertIntEquals_Msg(tc, "harbor master must contact ship", SA_NO_COAST, check_ship_allowed(mf.sh, mf.r));
+    test_cleanup();
+}
+
+static void test_ship_has_harbormaster_contact(CuTest * tc) {
+    unit *u;
+    move_fixture mf;
+
+    setup_harbor(&mf);
+
+    u = test_create_unit(test_create_faction(0), mf.r);
+    u->building = mf.b;
+    building_set_owner(u);
+    usetcontact(mf.b->_owner, mf.sh->_owner);
+
+    CuAssertIntEquals(tc, SA_HARBOUR, check_ship_allowed(mf.sh, mf.r));
+    test_cleanup();
+}
+
+static void test_ship_has_harbormaster_same_faction(CuTest * tc) {
+    unit *u;
+    move_fixture mf;
+
+    setup_harbor(&mf);
+
+    u = test_create_unit(mf.u->faction, mf.r);
+    u->building = mf.b;
+    building_set_owner(u);
+
+    CuAssertIntEquals(tc, SA_HARBOUR, check_ship_allowed(mf.sh, mf.r));
+    test_cleanup();
+}
+
+static void test_ship_has_harbormaster_ally(CuTest * tc) {
+    unit *u;
+    move_fixture mf;
+    ally *al;
+
+    setup_harbor(&mf);
+
+    u = test_create_unit(test_create_faction(0), mf.r);
+    u->building = mf.b;
+    building_set_owner(u);
+    al = ally_add(&u->faction->allies, mf.u->faction);
+    al->status = HELP_GUARD;
+
+    CuAssertIntEquals(tc, SA_HARBOUR, check_ship_allowed(mf.sh, mf.r));
+    test_cleanup();
 }
 
 static void test_building_type_exists(CuTest * tc)
 {
-  region *r;
+    region *r;
-  building *b;
+    building *b;
-  building_type *btype, *btype2;
+    building_type *btype, *btype2;
 
-  test_cleanup();
+    test_cleanup();
-  test_create_world();
+    test_create_world();
 
-  btype2 = bt_get_or_create("lighthouse");
+    btype2 = bt_get_or_create("lighthouse");
-  btype = bt_get_or_create("castle");
+    btype = bt_get_or_create("castle");
 
-  r = findregion(-1, 0);
+    r = findregion(-1, 0);
-  b = new_building(btype, r, default_locale);
+    b = new_building(btype, r, default_locale);
 
-  CuAssertPtrNotNull(tc, b);
+    CuAssertPtrNotNull(tc, b);
-  CuAssertTrue(tc, !buildingtype_exists(r, NULL, false));
+    CuAssertTrue(tc, !buildingtype_exists(r, NULL, false));
-  CuAssertTrue(tc, buildingtype_exists(r, btype, false));
+    CuAssertTrue(tc, buildingtype_exists(r, btype, false));
-  CuAssertTrue(tc, !buildingtype_exists(r, btype2, false));
+    CuAssertTrue(tc, !buildingtype_exists(r, btype2, false));
 }
 
 CuSuite *get_move_suite(void)
 {
-  CuSuite *suite = CuSuiteNew();
+    CuSuite *suite = CuSuiteNew();
-  SUITE_ADD_TEST(suite, test_building_type_exists);
+    SUITE_ADD_TEST(suite, test_building_type_exists);
-  SUITE_ADD_TEST(suite, test_ship_not_allowed_in_coast);
+    SUITE_ADD_TEST(suite, test_ship_not_allowed_in_coast);
-  SUITE_ADD_TEST(suite, test_ship_allowed_with_harbor);
+    SUITE_ADD_TEST(suite, test_ship_allowed_without_harbormaster);
-  return suite;
+    SUITE_ADD_TEST(suite, test_ship_blocked_by_harbormaster);
+    SUITE_ADD_TEST(suite, test_ship_has_harbormaster_contact);
+    SUITE_ADD_TEST(suite, test_ship_has_harbormaster_ally);
+    SUITE_ADD_TEST(suite, test_ship_has_harbormaster_same_faction);
+    return suite;
 }