[Freeciv-Dev] Re: (PR#4769) Re: Re: (PR#4761) civserver get_invention crash

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

On Wed, 6 Aug 2003, Jason Short wrote:
> Of course there are no UNDISBANDABLE units with upkeep in the default
> ruleset, so it's a bug that we hit this code at all.The logic in
> city_distribute_surplus_shields is faulty and will cause population loss
> any time you have to disband more than one unit from a city at a time.
>
> Fix attached; this is in addition to the other (memory corruption)
> patch.You can see the current code is faulty by adding in the 'assert'
> line from this patch all by itself: the assertion will trigger.

What about this? Untested.

  - Per

"This is the future for the world we're in at the moment,"
promised Lawrence Di Rita, special assistant to Rumsfeld.
"We'll get better as we do it more often."

Index: server/cityturn.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/cityturn.c,v
retrieving revision 1.218
diff -u -r1.218 cityturn.c
--- server/cityturn.c   4 Aug 2003 15:42:47 -0000       1.218
+++ server/cityturn.c   6 Aug 2003 22:36:48 -0000
@@ -56,7 +56,7 @@
 static bool worklist_change_build_target(struct player *pplayer,
                                         struct city *pcity);
 
-static void city_distribute_surplus_shields(struct player *pplayer,
+static bool city_distribute_surplus_shields(struct player *pplayer,
                                            struct city *pcity);
 static bool city_build_building(struct player *pplayer, struct city *pcity);
 static bool city_build_unit(struct player *pplayer, struct city *pcity);
@@ -427,14 +427,15 @@
 }
 
 /**************************************************************************
-...
+  Reduce the city size.  Return TRUE if the city survives the population
+  loss.
 **************************************************************************/
-void city_reduce_size(struct city *pcity, int pop_loss)
+bool city_reduce_size(struct city *pcity, int pop_loss)
 {
   if (pcity->size <= pop_loss) {
     remove_city_from_minimap(pcity->x, pcity->y);
     remove_city(pcity);
-    return;
+    return FALSE;
   }
   pcity->size -= pop_loss;
 
@@ -465,6 +466,8 @@
     auto_arrange_workers(pcity);
     sync_cities();
   }
+
+  return TRUE;
 }
 
 /**************************************************************************
@@ -607,7 +610,7 @@
       pcity->food_stock = city_granary_size(pcity->size - 1) / 2;
     else
       pcity->food_stock = 0;
-    city_reduce_size(pcity, 1);
+    (void) city_reduce_size(pcity, 1);
   }
 }
 
@@ -890,44 +893,48 @@
 }
 
 /**************************************************************************
-  Disband units if we don't have enough shields to support them.
+  Disband units if we don't have enough shields to support them.  Returns
+  FALSE if the _city_ is disbanded as a result.
 **************************************************************************/
-static void city_distribute_surplus_shields(struct player *pplayer,
+static bool city_distribute_surplus_shields(struct player *pplayer,
                                            struct city *pcity)
 {
   struct government *g = get_gov_pplayer(pplayer);
 
   while (pcity->shield_surplus < 0) {
     unit_list_iterate(pcity->units_supported, punit) {
-      if (utype_shield_cost(unit_type(punit), g) > 0
-          && !unit_flag(punit, F_UNDISBANDABLE)) {
-       struct packet_unit_request packet;
+      if (utype_shield_cost(unit_type(punit), g) > 0) {
+        if (!unit_flag(punit, F_UNDISBANDABLE)) {
+          struct packet_unit_request packet;
 
-       notify_player_ex(pplayer, pcity->x, pcity->y, E_UNIT_LOST,
-                        _("Game: %s can't upkeep %s, unit disbanded."),
-                        pcity->name, unit_type(punit)->name);
-        packet.unit_id = punit->id;
-        handle_unit_disband(pplayer, &packet);
-       break;
+         notify_player_ex(pplayer, pcity->x, pcity->y, E_UNIT_LOST,
+                          _("Game: %s can't upkeep %s, unit disbanded."),
+                          pcity->name, unit_type(punit)->name);
+          packet.unit_id = punit->id;
+          handle_unit_disband(pplayer, &packet);
+         break;
+        } else {
+          /* Special case: F_UNDISBANDABLE. This nasty unit won't go so easily.
+           * It would rather make the citizens pay in blood for their failure 
+           * to upkeep it! */
+         notify_player_ex(pplayer, pcity->x, pcity->y, E_UNIT_LOST,
+                          _("Game: Citizens in %s perish for their failure to "
+                          "upkeep %s!"), pcity->name, unit_type(punit)->name);
+          pcity->shield_surplus += utype_shield_cost(unit_type(punit), g);
+          if (!city_reduce_size(pcity, 1)) {
+            return FALSE;
+          }
+         break;
+        }
       }
     } unit_list_iterate_end;
-    /* Special case: F_UNDISBANDABLE. This nasty unit won't go so easily. It'd
-       rather make the citizens pay in blood for their failure to upkeep it! */
-    unit_list_iterate(pcity->units_supported, punit) {
-      if (utype_shield_cost(unit_type(punit), g) > 0) {
-       notify_player_ex(pplayer, pcity->x, pcity->y, E_UNIT_LOST,
-                        _("Game: Citizens in %s perish for their failure to "
-                        "upkeep %s!"), pcity->name, unit_type(punit)->name);
-        city_reduce_size(pcity, 1);
-       break;
-      }
-    }
-    unit_list_iterate_end;
   }
 
   /* Now we confirm changes made last turn. */
   pcity->shield_stock += pcity->shield_surplus;
   pcity->before_change_shields = pcity->shield_stock;
+
+  return TRUE;
 }
 
 /**************************************************************************
@@ -1138,7 +1145,9 @@
 **************************************************************************/
 static bool city_build_stuff(struct player *pplayer, struct city *pcity)
 {
-  city_distribute_surplus_shields(pplayer, pcity);
+  if (!city_distribute_surplus_shields(pplayer, pcity)) {
+    return FALSE;
+  }
   nullify_caravan_and_disband_plus(pcity);
 
   if (!pcity->is_building_unit) {