[Freeciv-Dev] (PR#12899) Core unit/city cleanup

["Per I. Mathisen" <per@xxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=12899 >

CHANGES
 - Transfer of units no longer creates and destroys units, leading to
   potential game loss for F_GAMELOSS units, and is much nicer.
 - When transfering a city, comment about trying to preserve ferries was
   wrong; we use wipe_unit() which will kill ferries if unlucky about
   order of deletion. However, this is the only sane way to do it. Comment
   fixed.
 - Insane unit iteration in remove_city() using goto and safe iterator
   fixed. Here we could leak memory and screw up stuff. Luckily the
   function was rarely called with units still in the city.
 - In same function, ensure we do actually delete left over units, not
   assume calling functions do this for us. (They may not.)
 - Fix PR#5073 in same function, about removing sea cities and trapped
   land units.
 - During activity resolving, readd a number of safeguards that had
   withered away by new and unsafe code that people have added.

  - Per

Index: server/citytools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.c,v
retrieving revision 1.314
diff -u -r1.314 citytools.c
--- server/citytools.c  25 Apr 2005 19:28:22 -0000      1.314
+++ server/citytools.c  26 Apr 2005 14:36:02 -0000
@@ -490,7 +490,7 @@
 }
 
 /*********************************************************************
-Note: the old unit is not wiped here.
+  Change home city of a unit with verbose output.
 ***********************************************************************/
 static void transfer_unit(struct unit *punit, struct city *tocity,
                          bool verbose)
@@ -527,14 +527,7 @@
       }
     }
   }
-
-  /* FIXME: Creating a new unit and deleting the old one is a gross hack.
-   * instead we should make the change on the existing unit, even though
-   * it's more work. */
-  (void) create_unit_full(to_player, punit->tile, punit->type,
-                         punit->veteran, tocity->id, punit->moves_left,
-                         punit->hp,
-                         find_unit_by_id(punit->transported_by));
+  real_unit_change_homecity(punit, tocity);
 }
 
 /*********************************************************************
@@ -569,7 +562,6 @@
       /* Don't transfer units already owned by new city-owner --wegge */ 
       if (unit_owner(vunit) == pvictim) {
        transfer_unit(vunit, pcity, verbose);
-       wipe_unit(vunit);
        unit_list_unlink(units, vunit);
       } else if (!pplayers_allied(pplayer, unit_owner(vunit))) {
         /* the owner of vunit is allied to pvictim but not to pplayer */
@@ -603,13 +595,8 @@
                         _("%s lost along with control of %s."),
                         unit_name(vunit->type), pcity->name);
       }
+      wipe_unit(vunit);
     }
-
-    /* not deleting cargo as it may be carried by units transferred later.
-       no cargo deletion => no trouble with "units" list.
-       In cases where the cargo can be left without transport the calling
-       function should take that into account. */
-    wipe_unit(vunit);
   } unit_list_iterate_safe_end;
 }
 
@@ -1035,38 +1022,22 @@
     city_remove_improvement(pcity, i);
   } built_impr_iterate_end;
 
-  /* This is cutpasted with modifications from transfer_city_units. Yes, it is 
ugly.
-     But I couldn't see a nice way to make them use the same code */
+  /* Rehome units in other cities */
   unit_list_iterate_safe(pcity->units_supported, punit) {
     struct city *new_home_city = tile_get_city(punit->tile);
-    ptile = punit->tile;
 
     if (new_home_city
        && new_home_city != pcity
        && city_owner(new_home_city) == pplayer) {
-      /* unit is in another city: make that the new homecity,
-        unless that city is actually the same city (happens if disbanding) */
-      freelog(LOG_VERBOSE, "Changed homecity of %s in %s",
-             unit_name(punit->type), new_home_city->name);
-      notify_player(pplayer, _("Changed homecity of %s in %s."),
-                   unit_name(punit->type), new_home_city->name);
-      (void) create_unit_full(city_owner(new_home_city), ptile,
-                             punit->type, punit->veteran, new_home_city->id,
-                             punit->moves_left, punit->hp, NULL);
+      transfer_unit(punit, new_home_city, TRUE);
     }
-
-    wipe_unit(punit);
   } unit_list_iterate_safe_end;
 
-  ptile = pcity->tile;
-
   /* make sure ships are not left on land when city is removed. */
- MOVE_SEA_UNITS:
   unit_list_iterate_safe(ptile->units, punit) {
     bool moved;
-    if (!punit
-       || !same_pos(punit->tile, ptile)
-       || !is_sailing_unit(punit)) {
+
+    if (!is_sailing_unit(punit)) {
       continue;
     }
 
@@ -1081,12 +1052,10 @@
                             _("Moved %s out of disbanded city %s "
                               "to avoid being landlocked."),
                             unit_type(punit)->name, pcity->name);
-           goto OUT;
          }
        }
       }
     } adjc_iterate_end;
-  OUT:
     if (!moved) {
       notify_player_ex(unit_owner(punit), NULL, E_NOEVENT,
                       _("When %s was disbanded your %s could not "
@@ -1094,9 +1063,22 @@
                       pcity->name, unit_type(punit)->name);
       wipe_unit(punit);
     }
-    /* We just messed with the unit list. Avoid trouble by starting over.
-       Note that the problem is reduced by one unit, so no loop trouble. */
-    goto MOVE_SEA_UNITS;
+  } unit_list_iterate_safe_end;
+
+  /* Destroy final ineligible units (land units in ocean city) */
+  unit_list_iterate_safe(ptile->units, punit) {
+    if (is_ocean(tile_get_terrain(ptile)) && is_ground_unit(punit)) {
+      notify_player_ex(unit_owner(punit), NULL, E_NOEVENT,
+                      _("When %s was disbanded your %s could not "
+                        "get out, and it was therefore stranded."),
+                      pcity->name, unit_type(punit)->name);
+      wipe_unit(punit);
+    }
+  } unit_list_iterate_safe_end;
+
+  /* Any remaining supported units are destroyed */
+  unit_list_iterate_safe(pcity->units_supported, punit) {
+    wipe_unit(punit);
   } unit_list_iterate_safe_end;
 
   for (o = 0; o < NUM_TRADEROUTES; o++) {
Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.329
diff -u -r1.329 unithand.c
--- server/unithand.c   23 Apr 2005 17:40:29 -0000      1.329
+++ server/unithand.c   26 Apr 2005 14:36:03 -0000
@@ -281,14 +281,38 @@
 }
 
 /**************************************************************************
+  Transfer a unit from one homecity to another.
+**************************************************************************/
+void real_unit_change_homecity(struct unit *punit, struct city *new_pcity)
+{
+  struct city *old_pcity = find_city_by_id(punit->homecity);
+
+  unit_list_prepend(new_pcity->units_supported, punit);
+  if (old_pcity) {
+    unit_list_unlink(old_pcity->units_supported, punit);
+  }
+
+  punit->homecity = new_pcity->id;
+  punit->owner = unit_owner(punit)->player_no;
+  send_unit_info(unit_owner(punit), punit);
+
+  city_refresh(new_pcity);
+  send_city_info(city_owner(new_pcity), new_pcity);
+
+  if (old_pcity) {
+    city_refresh(old_pcity);
+    send_city_info(city_owner(old_pcity), old_pcity);
+  }
+}
+
+/**************************************************************************
 ...
 **************************************************************************/
 void handle_unit_change_homecity(struct player *pplayer, int unit_id,
                                 int city_id)
 {
   struct unit *punit = player_find_unit_by_id(pplayer, unit_id);
-  struct city *old_pcity, *new_pcity =
-      player_find_city_by_id(pplayer, city_id);
+  struct city *new_pcity = player_find_city_by_id(pplayer, city_id);
 
   if (!punit || !new_pcity || city_id == punit->homecity) {
     return;
@@ -303,24 +327,7 @@
             new_pcity->tile->y);
     return;
   }
-
-  old_pcity = player_find_city_by_id(pplayer, punit->homecity);
-
-  unit_list_prepend(new_pcity->units_supported, punit);
-  if (old_pcity) {
-    unit_list_unlink(old_pcity->units_supported, punit);
-  }
-
-  punit->homecity = new_pcity->id;
-  send_unit_info(pplayer, punit);
-
-  city_refresh(new_pcity);
-  send_city_info(pplayer, new_pcity);
-
-  if (old_pcity) {
-    city_refresh(old_pcity);
-    send_city_info(pplayer, old_pcity);
-  }
+  real_unit_change_homecity(punit, new_pcity);
 }
 
 /**************************************************************************
Index: server/unithand.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.h,v
retrieving revision 1.37
diff -u -r1.37 unithand.h
--- server/unithand.h   29 Sep 2004 02:24:24 -0000      1.37
+++ server/unithand.h   26 Apr 2005 14:36:03 -0000
@@ -22,5 +22,6 @@
                             bool igzoc, bool move_diplomat_city);
 void handle_unit_activity_request(struct unit *punit, 
                                  enum unit_activity new_activity);
+void real_unit_change_homecity(struct unit *punit, struct city *new_pcity);
 
 #endif  /* FC__UNITHAND_H */
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.339
diff -u -r1.339 unittools.c
--- server/unittools.c  25 Apr 2005 19:28:22 -0000      1.339
+++ server/unittools.c  26 Apr 2005 14:36:03 -0000
@@ -632,7 +632,7 @@
     /* settler may become veteran when doing something useful */
     if (activity != ACTIVITY_FORTIFYING && activity != ACTIVITY_SENTRY
        && maybe_settler_become_veteran(punit)) {
-      notify_player_ex(pplayer, punit->tile, E_UNIT_BECAME_VET,
+      notify_player_ex(pplayer, ptile, E_UNIT_BECAME_VET,
        _("Your %s became more experienced!"), unit_name(punit->type));
     }
     
@@ -661,11 +661,11 @@
       if (punit->activity_count >= 1) {
        enum tile_special_type what =
          get_preferred_pillage(
-              get_tile_infrastructure_set(punit->tile));
+              get_tile_infrastructure_set(ptile));
 
        if (what != S_NO_SPECIAL) {
-         tile_clear_special(punit->tile, what);
-         update_tile_knowledge(punit->tile);
+         tile_clear_special(ptile, what);
+         update_tile_knowledge(ptile);
          set_unit_activity(punit, ACTIVITY_IDLE);
          check_adjacent_units = TRUE;
        }
@@ -674,7 +674,7 @@
        if (what == S_FORTRESS) {
          freelog(LOG_VERBOSE, "Watchtower pillaged!");
          /* This could be a helper function. */
-         unit_list_iterate(punit->tile->units, punit2) {
+         unit_list_iterate(ptile->units, punit2) {
             struct player *owner = unit_owner(punit2);
 
             if (is_ground_unit(punit2)
@@ -688,30 +688,30 @@
        }
       }
     }
-    else if (total_activity_targeted(punit->tile, ACTIVITY_PILLAGE, 
+    else if (total_activity_targeted(ptile, ACTIVITY_PILLAGE, 
                                      punit->activity_target) >= 1) {
       enum tile_special_type what_pillaged = punit->activity_target;
 
-      tile_clear_special(punit->tile, what_pillaged);
-      unit_list_iterate (punit->tile->units, punit2) {
+      tile_clear_special(ptile, what_pillaged);
+      unit_list_iterate (ptile->units, punit2) {
         if ((punit2->activity == ACTIVITY_PILLAGE) &&
            (punit2->activity_target == what_pillaged)) {
          set_unit_activity(punit2, ACTIVITY_IDLE);
          send_unit_info(NULL, punit2);
        }
       } unit_list_iterate_end;
-      update_tile_knowledge(punit->tile);
+      update_tile_knowledge(ptile);
       
       /* If a watchtower has been pillaged, reduce sight to normal */
       if (what_pillaged == S_FORTRESS) {
        freelog(LOG_VERBOSE, "Watchtower(2) pillaged!");
        /* This could be a helper function. */
-       unit_list_iterate(punit->tile->units, punit2) {
+       unit_list_iterate(ptile->units, punit2) {
           struct player *owner = unit_owner(punit2);
 
           if (is_ground_unit(punit2)
               && player_knows_techs_with_flag(owner, TF_WATCHTOWER)) {
-            change_vision_range(owner, punit->tile,
+            change_vision_range(owner, ptile,
                                get_watchtower_vision(punit2),
                                 unit_type(punit2)->vision_range);
           }
@@ -722,25 +722,25 @@
   }
 
   if (activity==ACTIVITY_POLLUTION) {
-    if (total_activity (punit->tile, ACTIVITY_POLLUTION)
-       >= map_clean_pollution_time(punit->tile)) {
-      tile_clear_special(punit->tile, S_POLLUTION);
+    if (total_activity (ptile, ACTIVITY_POLLUTION)
+       >= map_clean_pollution_time(ptile)) {
+      tile_clear_special(ptile, S_POLLUTION);
       unit_activity_done = TRUE;
     }
   }
 
   if (activity==ACTIVITY_FALLOUT) {
-    if (total_activity (punit->tile, ACTIVITY_FALLOUT)
-       >= map_clean_fallout_time(punit->tile)) {
-      tile_clear_special(punit->tile, S_FALLOUT);
+    if (total_activity (ptile, ACTIVITY_FALLOUT)
+       >= map_clean_fallout_time(ptile)) {
+      tile_clear_special(ptile, S_FALLOUT);
       unit_activity_done = TRUE;
     }
   }
 
   if (activity==ACTIVITY_FORTRESS) {
-    if (total_activity (punit->tile, ACTIVITY_FORTRESS)
-       >= map_build_fortress_time(punit->tile)) {
-      tile_set_special(punit->tile, S_FORTRESS);
+    if (total_activity (ptile, ACTIVITY_FORTRESS)
+       >= map_build_fortress_time(ptile)) {
+      tile_set_special(ptile, S_FORTRESS);
       unit_activity_done = TRUE;
       /* watchtower becomes effective */
       /* This could be a helper function. */
@@ -749,7 +749,7 @@
 
         if (is_ground_unit(punit)
             && player_knows_techs_with_flag(owner, TF_WATCHTOWER)) {
-          change_vision_range(pplayer, punit->tile,
+          change_vision_range(pplayer, ptile,
                              unit_type(punit)->vision_range,
                               get_watchtower_vision(punit));
         }
@@ -759,65 +759,65 @@
   }
 
   if (activity==ACTIVITY_AIRBASE) {
-    if (total_activity (punit->tile, ACTIVITY_AIRBASE)
-       >= map_build_airbase_time(punit->tile)) {
-      tile_set_special(punit->tile, S_AIRBASE);
+    if (total_activity (ptile, ACTIVITY_AIRBASE)
+       >= map_build_airbase_time(ptile)) {
+      tile_set_special(ptile, S_AIRBASE);
       unit_activity_done = TRUE;
     }
   }
   
   if (activity==ACTIVITY_IRRIGATE) {
-    if (total_activity (punit->tile, ACTIVITY_IRRIGATE) >=
-        map_build_irrigation_time(punit->tile)) {
-      Terrain_type_id old = tile_get_terrain(punit->tile);
-      map_irrigate_tile(punit->tile);
-      solvency = check_terrain_ocean_land_change(punit->tile, old);
+    if (total_activity (ptile, ACTIVITY_IRRIGATE) >=
+        map_build_irrigation_time(ptile)) {
+      Terrain_type_id old = tile_get_terrain(ptile);
+      map_irrigate_tile(ptile);
+      solvency = check_terrain_ocean_land_change(ptile, old);
       unit_activity_done = TRUE;
     }
   }
 
   if (activity==ACTIVITY_ROAD) {
-    if (total_activity (punit->tile, ACTIVITY_ROAD)
-       + total_activity (punit->tile, ACTIVITY_RAILROAD) >=
-        map_build_road_time(punit->tile)) {
-      tile_set_special(punit->tile, S_ROAD);
+    if (total_activity (ptile, ACTIVITY_ROAD)
+       + total_activity (ptile, ACTIVITY_RAILROAD) >=
+        map_build_road_time(ptile)) {
+      tile_set_special(ptile, S_ROAD);
       unit_activity_done = TRUE;
     }
   }
 
   if (activity==ACTIVITY_RAILROAD) {
-    if (total_activity (punit->tile, ACTIVITY_RAILROAD)
-       >= map_build_rail_time(punit->tile)) {
-      tile_set_special(punit->tile, S_RAILROAD);
+    if (total_activity (ptile, ACTIVITY_RAILROAD)
+       >= map_build_rail_time(ptile)) {
+      tile_set_special(ptile, S_RAILROAD);
       unit_activity_done = TRUE;
     }
   }
   
   if (activity==ACTIVITY_MINE) {
-    if (total_activity (punit->tile, ACTIVITY_MINE) >=
-        map_build_mine_time(punit->tile)) {
-      Terrain_type_id old = tile_get_terrain(punit->tile);
-      map_mine_tile(punit->tile);
-      solvency = check_terrain_ocean_land_change(punit->tile, old);
+    if (total_activity (ptile, ACTIVITY_MINE) >=
+        map_build_mine_time(ptile)) {
+      Terrain_type_id old = tile_get_terrain(ptile);
+      map_mine_tile(ptile);
+      solvency = check_terrain_ocean_land_change(ptile, old);
       unit_activity_done = TRUE;
       check_adjacent_units = TRUE;
     }
   }
 
   if (activity==ACTIVITY_TRANSFORM) {
-    if (total_activity (punit->tile, ACTIVITY_TRANSFORM) >=
-        map_transform_time(punit->tile)) {
-      Terrain_type_id old = tile_get_terrain(punit->tile);
-      map_transform_tile(punit->tile);
-      solvency = check_terrain_ocean_land_change(punit->tile, old);
+    if (total_activity (ptile, ACTIVITY_TRANSFORM) >=
+        map_transform_time(ptile)) {
+      Terrain_type_id old = tile_get_terrain(ptile);
+      map_transform_tile(ptile);
+      solvency = check_terrain_ocean_land_change(ptile, old);
       unit_activity_done = TRUE;
       check_adjacent_units = TRUE;
     }
   }
 
   if (unit_activity_done) {
-    update_tile_knowledge(punit->tile);
-    unit_list_iterate (punit->tile->units, punit2) {
+    update_tile_knowledge(ptile);
+    unit_list_iterate (ptile->units, punit2) {
       if (punit2->activity == activity) {
        set_unit_activity(punit2, ACTIVITY_IDLE);
        send_unit_info(NULL, punit2);
@@ -827,7 +827,7 @@
 
   /* Some units nearby can not continue irrigating */
   if (check_adjacent_units) {
-    adjc_iterate(punit->tile, ptile2) {
+    adjc_iterate(ptile, ptile2) {
       unit_list_iterate(ptile2->units, punit2) {
         if (!can_unit_continue_current_activity(punit2)) {
           handle_unit_activity_request(punit2, ACTIVITY_IDLE);
@@ -853,14 +853,16 @@
     return;
   }
 
-  if (unit_has_orders(punit)) {
+  if (find_unit_by_id(id) && unit_has_orders(punit)) {
     if (!execute_orders(punit)) {
       /* Unit died. */
       return;
     }
   }
 
-  send_unit_info(NULL, punit);
+  if (find_unit_by_id(id)) {
+    send_unit_info(NULL, punit);
+  }
 
   unit_list_iterate(ptile->units, punit2) {
     if (!can_unit_continue_current_activity(punit2))
@@ -894,7 +896,7 @@
     unit_list_iterate(ptile->units, punit2) {
       if (is_ground_unit(punit2)) {
        /* look for nearby land */
-       adjc_iterate(punit->tile, ptile2) {
+       adjc_iterate(ptile, ptile2) {
          if (!is_ocean(ptile2->terrain)
              && !is_non_allied_unit_tile(ptile2, unit_owner(punit2))) {
            if (get_transporter_capacity(punit2) > 0)
@@ -914,7 +916,7 @@
          }
        } adjc_iterate_end;
        /* look for nearby transport */
-       adjc_iterate(punit->tile, ptile2) {
+       adjc_iterate(ptile, ptile2) {
          if (is_ocean(ptile2->terrain)
              && ground_unit_transporter_capacity(ptile2,
                                                  unit_owner(punit2)) > 0) {
@@ -952,7 +954,7 @@
     unit_list_iterate(ptile->units, punit2) {
       if (is_sailing_unit(punit2)) {
        /* look for nearby water */
-       adjc_iterate(punit->tile, ptile2) {
+       adjc_iterate(ptile, ptile2) {
          if (is_ocean(ptile2->terrain)
              && !is_non_allied_unit_tile(ptile2, unit_owner(punit2))) {
            if (get_transporter_capacity(punit2) > 0)
@@ -972,7 +974,7 @@
          }
        } adjc_iterate_end;
        /* look for nearby port */
-       adjc_iterate(punit->tile, ptile2) {
+       adjc_iterate(ptile, ptile2) {
          if (is_allied_city_tile(ptile2, unit_owner(punit2))
              && !is_non_allied_unit_tile(ptile2, unit_owner(punit2))) {
            if (get_transporter_capacity(punit2) > 0)
@@ -1984,6 +1986,8 @@
     dest = game.game_connections;
   }
 
+  CHECK_UNIT(punit);
+
   package_unit(punit, &info);
   package_short_unit(punit, &sinfo, UNIT_INFO_IDENTITY, FALSE, FALSE);