[Freeciv-Dev] Re: (PR#12677) CVS: Spies get built foul, so cannot make embassy

["Brendon" <yautja@xxxxxxxxxxxxxxx>]

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



Jason Short wrote:

> Good report.  The problem is setting 'foul' in create_unit_full is quite 
> a hack.  It assumes any newly-created unit will have -1 (default = full) 
> movement while a recreated unit will have the same MP as the unit it is 
> being copied from.  The problem with this is that after the 
> alternating-movement patch newly-created units always have 0 MP so it is 
> impossible to distinguish based on this.
> 

As I unsuccessfully tried to explain, create_unit is never called when a 
spy successfully escapes, I think this is also true in beta8.

Therefore they are never made foul by that (or any) piece of code.
I tested this by putting a log entry to make 100% sure (I guess printf 
only works if you start the client and server seperately ?)

There are two bugs being reported here, only found the second because of 
the one in the subject, but they both share a common solution.

> Simply removing the line will improve behavior but not fix it. 
> Sometimes create_unit is called to create a unit that isn't newly built. 
>   This happens when transferring units (generally through bribery).
> It's ugly but doing it any other way is hard.
> 

Well personally I think bribed diplomats and spies should become instant 
elite, as even Sun Tzu reckons doubled agents are capable of things 
native spies are not. Having spies (as diplomats never escape) lose 
their foulness when bribed is a small step in this direction.

> So, I see two possible fixes:
> 
> * Add a new parameter to create_unit_full, specifying whether the unit 
> is "foul".  Probably create_unit() can default to FALSE here.
> 

I just looked at city_transfer, If I'm correct every spy unit transfered 
with this would be made foul with current cvs.

But this is impossible to do with bribery as the spies would have to be 
killed first, although it is used to trade cities in diplomacy.

Only way around this is to give the transfered units full (-1) movement, 
and this could be abused by allies for infinite movement by transferring 
cities between them.

I think it's best for any transfered spy to be non-foul, as there is no 
way for the receiving party to know It's history. Hell, there is no easy 
way for you to tell which of your spies are foul, as I'm finding out 
testing this patch.

> * Change -1 MP in create_unit_full to mean 0 MP.  This would require 
> great care not to break other things, and would leave the ugliness of 
> punit->foul in create_unit.
> 
> -jason

Only places I can find -1 moves_left in CVS is for Hut mercenarys and 
Barbarian uprisings. Should they be 0 ?

--------

Attached a simple patch (-3 and +2 lines). It's my first, so may not 
even apply and probably has something wrong with it, diffed with cvs.

I assume I don't need to make punit->foul = 0 when creating a unit 
because it's set to FALSE in create_unit_virtual.

They now work as intended in the savegame, and now they're only made and 
checked if foul within diplomat.c, which is cleaner.

Index: server/diplomats.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/diplomats.c,v
retrieving revision 1.70
diff -u -r1.70 diplomats.c
--- server/diplomats.c  21 Mar 2005 12:21:28 -0000      1.70
+++ server/diplomats.c  29 Mar 2005 02:02:07 -0000
@@ -1276,6 +1276,9 @@
       return;
     }
 
+    /* will be executed if she ever tries to make an embassy */
+    pdiplomat->foul = TRUE;
+
     return;
   } else {
     if (pcity) {
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.330
diff -u -r1.330 unittools.c
--- server/unittools.c  24 Mar 2005 17:48:49 -0000      1.330
+++ server/unittools.c  29 Mar 2005 02:02:51 -0000
@@ -1483,10 +1483,6 @@
    * (Otherwise could pass moved arg too...)  --dwp */
   punit->moved = (moves_left >= 0);
 
-  /* See if this is a spy that has been moved (corrupt and therefore 
-   * unable to establish an embassy. */
-  punit->foul = (moves_left != -1 && unit_flag(punit, F_SPY));
-
   unit_list_prepend(pplayer->units, punit);
   unit_list_prepend(ptile->units, punit);
   if (pcity && !unit_type_flag(type, F_NOHOME)) {