[Freeciv-Dev] (PR#6751) Cleanup of transfer_unit()

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

This cleanup of transfer unit removes a bug that would cause transfered
gameloss units lose you the game, and also removes a potential buglet
where you would be given the wrong notify message.

Premature optimalization of the day: Mass transfer of units should now be
much faster.

  - Per

Index: server/citytools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.c,v
retrieving revision 1.239
diff -u -r1.239 citytools.c
--- server/citytools.c  4 Nov 2003 10:38:00 -0000       1.239
+++ server/citytools.c  4 Nov 2003 11:07:34 -0000
@@ -562,14 +562,14 @@
 }
 
 /*********************************************************************
-Note: the old unit is not wiped here.
+  Transfer unit ownership and support to given city and its owner.
 ***********************************************************************/
 static void transfer_unit(struct unit *punit, struct city *tocity,
                          bool verbose)
 {
   struct player *from_player = unit_owner(punit);
   struct player *to_player = city_owner(tocity);
-  struct unit *punit2;
+  struct city *homecity = find_city_by_id(punit->homecity);
 
   if (from_player == to_player) {
     freelog(LOG_VERBOSE, "Changed homecity of %s's %s to %s",
@@ -580,7 +580,9 @@
     }
   } else {
     struct city *in_city = map_get_city(punit->x, punit->y);
-    if (in_city) {
+
+    assert(pplayers_allied(city_owner(in_city), to_player));
+    if (in_city && in_city == tocity) {
       freelog(LOG_VERBOSE, "Transfered %s in %s from %s to %s",
              unit_name(punit->type), in_city->name,
              from_player->name, to_player->name);
@@ -599,15 +601,27 @@
                      from_player->name, to_player->name);
       }
     }
+    remove_unit_sight_points(punit);
+    unit_list_insert(&to_player->units, punit);
+    unit_list_unlink(&from_player->units, punit);
+    punit->owner = to_player->player_no;
+    maybe_make_contact(punit->x, punit->y, to_player);
+    if (map_has_special(punit->x, punit->y, S_FORTRESS)
+        && unit_profits_of_watchtower(punit)) {
+      unfog_area(to_player, punit->x, punit->y, get_watchtower_vision(punit));
+    } else {
+      unfog_area(to_player, punit->x, punit->y, 
unit_type(punit)->vision_range);
+    }
   }
-
-  /* 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. */
-  punit2 = create_unit_full(to_player, punit->x, punit->y, punit->type,
-                           punit->veteran, tocity->id, punit->moves_left,
-                           punit->hp);
-  punit2->transported_by = punit->transported_by;
+  punit->homecity = tocity->id;
+  unit_list_insert(&tocity->units_supported, punit);
+  if (homecity) {
+    unit_list_unlink(&homecity->units_supported, punit);
+    city_refresh(homecity);
+    send_city_info(from_player, homecity);
+  }
+  city_refresh(tocity);
+  send_city_info(to_player, tocity);
 }
 
 /*********************************************************************
@@ -643,7 +657,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 */
@@ -656,26 +669,28 @@
      cities or maybe destroyed */
   unit_list_iterate_safe(*units, vunit) {
     struct city *new_home_city = map_get_city(vunit->x, vunit->y);
+
     if (new_home_city && new_home_city != exclude_city) {
       /* unit is in another city: make that the new homecity,
         unless that city is actually the same city (happens if disbanding) */
       transfer_unit(vunit, new_home_city, verbose);
+      continue;
     } else if (kill_outside == -1
               || real_map_distance(vunit->x, vunit->y, x, y) <= kill_outside) {
       /* else transfer to specified city. */
       transfer_unit(vunit, pcity, verbose);
-    } else {
-      /* The unit is lost.  Call notify_player (in all other cases it is
-       * called autmatically). */
-      freelog(LOG_VERBOSE, "Lost %s's %s at (%d,%d) when %s was lost.",
-             unit_owner(vunit)->name, unit_name(vunit->type),
-             vunit->x, vunit->y, pcity->name);
-      if (verbose) {
-       notify_player_ex(unit_owner(vunit), vunit->x, vunit->y,
-                        E_UNIT_LOST,
-                        _("Game: %s lost along with control of %s."),
-                        unit_name(vunit->type), pcity->name);
-      }
+      continue;
+    }
+
+    /* The unit is lost.  Call notify_player (in all other cases it is
+     * called autmatically). */
+    freelog(LOG_VERBOSE, "Lost %s's %s at (%d,%d) when %s was lost.",
+            unit_owner(vunit)->name, unit_name(vunit->type),
+            vunit->x, vunit->y, pcity->name);
+    if (verbose) {
+      notify_player_ex(unit_owner(vunit), vunit->x, vunit->y, E_UNIT_LOST,
+                       _("Game: %s lost along with control of %s."),
+                       unit_name(vunit->type), pcity->name);
     }
 
     /* not deleting cargo as it may be carried by units transferred later.
@@ -1097,22 +1112,17 @@
      But I couldn't see a nice way to make them use the same code */
   unit_list_iterate(pcity->units_supported, punit) {
     struct city *new_home_city = map_get_city(punit->x, punit->y);
+
     x = punit->x; y = punit->y;
     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, _("Game: Changed homecity of %s in %s."),
-                   unit_name(punit->type), new_home_city->name);
-      (void) create_unit_full(city_owner(new_home_city), x, y,
-                      punit->type, punit->veteran, new_home_city->id,
-                      punit->moves_left, punit->hp);
+      /* Unit is in another city: make that the new homecity, unless 
+       * that city is actually the same city (happens if disbanding) */
+      transfer_unit(punit, new_home_city, TRUE);
+    } else {
+      wipe_unit(punit);
     }
-
-    wipe_unit(punit);
   } unit_list_iterate_end;
 
   x = pcity->x;