[Freeciv-Dev] (PR#11415) generic_city_refresh(pcity, FALSE, ...) is very dangerous

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

I'm not sure how much of a problem this really is.

cm_query_result seems in my tests to not change any of the city fields
(except for refreshing).  I'm not quite sure how this is, so maybe I'm
mistaken.

I reviewed all the other callers and lack of callers of
generic_city_refresh.  In this patch:

- I add a new refresh to cm_query_result at the start.
- I assert that cm_query_result doesn't change the city (I expeted this
to always fail but it hasn't failed yet).
- I changed many generic_city_refresh(pcity, FALSE) to
generic_city_refresh(pcity, TRUE).
- I removed the refresh calls from the client (replaced by the
cm_query_result refresh).
- I changed the savegame loading.  I think this could just use
generic_city_refresh(pcity, TRUE) but this could be dangerous since
cities are basically unitialized at the start.  Instead I just have a
double-loop to refresh each city twice.

Now I *think* in this patch cities will never be unrefreshed in the
server for any length of time.  I also think it solves the traderoute
problem.  However there may still be bugs.

jason

Index: ai/aihand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aihand.c,v
retrieving revision 1.92.2.1
diff -u -r1.92.2.1 aihand.c
--- ai/aihand.c 8 Dec 2004 20:48:34 -0000       1.92.2.1
+++ ai/aihand.c 9 Dec 2004 06:52:46 -0000
@@ -304,7 +304,7 @@
     /* Now reset our gov to it's real state. */
     pplayer->government = current_gov;
     city_list_iterate(pplayer->cities, acity) {
-      generic_city_refresh(acity, FALSE, NULL);
+      generic_city_refresh(acity, TRUE, NULL);
       auto_arrange_workers(acity);
     } city_list_iterate_end;
     ai->govt_reeval = CLIP(5, city_list_size(&pplayer->cities), 20);
Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.408.2.15
diff -u -r1.408.2.15 packhand.c
--- client/packhand.c   8 Dec 2004 20:48:35 -0000       1.408.2.15
+++ client/packhand.c   9 Dec 2004 06:52:47 -0000
@@ -579,10 +579,6 @@
 {
   int i;
 
-  if (city_owner(pcity) == game.player_ptr) {
-    generic_city_refresh(pcity, FALSE, 0);
-  }
-
   if(is_new) {
     unit_list_init(&pcity->units_supported);
     unit_list_init(&pcity->info_units_supported);
Index: client/agents/cma_core.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/agents/cma_core.c,v
retrieving revision 1.64.2.1
diff -u -r1.64.2.1 cma_core.c
--- client/agents/cma_core.c    29 Nov 2004 16:19:59 -0000      1.64.2.1
+++ client/agents/cma_core.c    9 Dec 2004 06:52:49 -0000
@@ -388,7 +388,6 @@
     }
 
     pcity = find_city_by_id(city_id);
-    generic_city_refresh(pcity, FALSE, 0);
 
     cm_query_result(pcity, &parameter, &result);
     if (!result.found_a_valid) {
Index: common/aicore/cm.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/aicore/cm.c,v
retrieving revision 1.43.2.1
diff -u -r1.43.2.1 cm.c
--- common/aicore/cm.c  19 Nov 2004 03:44:40 -0000      1.43.2.1
+++ common/aicore/cm.c  9 Dec 2004 06:52:50 -0000
@@ -1824,9 +1824,25 @@
                     struct cm_result *result)
 {
   struct cm_state *state = cm_init_state(pcity);
+#ifdef DEBUG
+  struct city old_city;
+#endif
+
+  /* Refresh the city.  Otherwise the CM can give wrong results or just be
+   * slower than necessary.  Note that cities are often passed in in an
+   * unrefreshed state (which should probably be fixed). */
+  generic_city_refresh(pcity, TRUE, NULL);
+
+#ifdef DEBUG
+  old_city = *pcity;
+#endif
 
   cm_find_best_solution(state, param, result);
   cm_free_state(state);
+
+#ifdef DEBUG
+  assert(memcmp(&old_city, pcity, sizeof(old_city)) == 0);
+#endif
 }
 
 
Index: server/savegame.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/savegame.c,v
retrieving revision 1.197.2.9
diff -u -r1.197.2.9 savegame.c
--- server/savegame.c   29 Nov 2004 20:03:39 -0000      1.197.2.9
+++ server/savegame.c   9 Dec 2004 06:52:52 -0000
@@ -3445,10 +3445,15 @@
                  technology_order_size); 
     }
 
+    /* Update all city information.  This must come after all cities are
+     * loaded (in player_load) but before player (dumb) cities are loaded
+     * (in player_map_load).  Cities are refreshed twice to account for
+     * trade routes: the first refresh initializes all cities tile_trade
+     * values; the second correctly updates all trade routes. */
+    cities_iterate(pcity) {
+      generic_city_refresh(pcity, FALSE, NULL);
+    } cities_iterate_end;
     cities_iterate(pcity) {
-      /* Update all city information.  This must come after all cities are
-       * loaded (in player_load) but before player (dumb) cities are loaded
-       * (in player_map_load). */
       generic_city_refresh(pcity, FALSE, NULL);
     } cities_iterate_end;