Commit e3cf1e32 (v4.3.27) created make_stale_if_command_arg() to
permit catching more potential yields on input. Unfortunately, the
implementation of navigate and march sub-commands 'r', 'l' and 's'
breaks it.
do_unit_move() reads units into a unit list at the beginning and at
each stop. It writes them back when they move or sweep. If a unit
changed in the file in between, the changes would get wiped out.
Therefore, do_unit_move() must not yield between stops.
do_unit_move() parses sub-commands into player->argp[], then supplies
defaults for missing arguments, so that code using them (radar(),
do_look(), sona(), mine(), landmine()) won't prompt for missing
arguments. Unclean and brittle. See also commit 28cc236e and commit
45106ab9.
Unfortunately, make_stale_if_command_arg() doesn't recognize the
difference between these defaulted arguments and parsed arguments, so
it makes objects stale, even though the defaulted arguments can't be
missing. If a move or sweep follows, it triggers a false positive
generation oops.
To fix, test "points into argument buffer" (only true for parsed
arguments) instead of "is in player->argp[]". Requires making the
argument buffer accessible: new struct player member argbuf[]. Use it
for parsing commands, in command(), execute(), do_unit_move(). Don't
use it in emp_config(), player_login(), move_ground(), because these
parse something else.
verify_row() refrains from rejecting zero uids, because some tables
may contain blank entries, with zero uid.
Change it to check only header sanity for entries that are not in use.
This filters out all legitimately blank entries. Tighten up the uid
check.
For computing "in use", factor empobj_in_use() out of xdvisible().
Note that xdvisible()'s case EF_COUNTRY doesn't bother to check
nat_stat, because that's implied by what it does check. It's not
implied in empobj_in_use(), so add it there.
Spy units are now enabled when a land unit type with capability spy
exists. To disable them, deities have to customize table land-chr.
Before, spy units types were ignored when option LANDSPIES was
disabled. Except for xdump land-chr, which happily dumped unusable
spy unit types.
Trade ships are now enabled when a ship type with capability trade
exists. No such type exists by default; to enable trade ships,
deities have to customize table ship-chr.
Before, trade ship types were ignored when option TRADESHIPS was
disabled. Except for xdump ship-chr, which happily dumped unusable
trade ship types.
Configuration table entries not defined by builtin and custom
configuration files remain blank. They get misinterpreted as sentinel
in tables that use one. Affected are tables product, ship-chr,
plane-chr, land-chr and nuke-chr. Tables item, sect-chr and
infrastructure are immune despite using a sentinel, because omitting
entries is not permitted there.
Code relying on the sentinel fails to pick up entries after the first
blank one. They don't get set up correctly, they're invisible to
build and show, and not recognized as symbolic selector values (the
frg in ship ?type=frg). xdump is fine, because it doesn't rely on
sentinels. It dumps blank entries normally.
The bugs don't bite in the stock game, because the builtin
configuration files are all dense.
The sentinels are all null strings. Set them to "" in the affected
tables' oninit callback. Fix up code iterating over the tables to
ignore such entries. This is precisely the code relying on sentinels,
plus xdump's xdvisible().
To enable that, make build_ship() & friends all take the same int type
argument instead of each one its own pointer. Passing pointers
triggered "may be used uninitialized" compiler warnings (the code was
safe despite the warnings).
For drnuke_const 0.33, research level 92.4 now suffices for a tech 280
nuke. Before, you needed 93, which was inconsistent with what
version's promise "need 0.33 times the tech level in research".
buy() reads the lot, prompts for input, then writes back the lot,
triggering a generation oops. Any updates made by other threads while
buy() waits for input are wiped out, triggering a seqno mismatch oops.
Since commodities are taken from the seller when he puts them on the
market, and given to the buyer when the trade executes, the wiped out
lot's seller loses his goods without compensation, the other seller
gets to keep his goods, and the buyer receives their duplicates.
This can be abused by two conspiring countries to duplicate
commodities. The seller puts them on the market (say 100 gold bars).
The buyer starts a buy command, and waits at its last prompt for the
lot to be replaced. The seller takes them off the market (possible,
since there's no bid, yet), and sells something else (say one food)
quickly enough to get the same lot number assigned. The buyer then
completes the buy command. The seller loses one food, the buyer gains
100 gold bars.
Replaces a partial fix from v4.0.1, which only caught lots gone away,
not lots replaced by new ones.
setsector() reads the sector, prompts for input, then writes back the
sector, triggering a generation oops. Any updates made by other
threads while setsector() waits for input are wiped out, triggering a
seqno mismatch oops.
Same for setres().
give() reads the sector, prompts for input, updates the sector and
writes it back, triggering a generation oops. Any updates made by
other threads during the yield are wiped out, triggering a seqno
mismatch oops.
Split with parse() and pass first two arguments instead of the raw
tail to the map() callback. Advantages:
* Consistent with do_unit_move().
* Does the right thing when the tail is just spaces. Before, the
spaces got passed to the map() callback, which complained about
syntax. Now, they are ignored. This is what the commit I just
reverted tried to fix.
* Works better when the tail splits into more than two arguments.
Except for explore_map(), which ignores the argument(s), the map()
callbacks use display_region_map(), which split the tail at the
first space, and complained about any spaces in the second part.
Now, display_region_map() takes two argument strings instead of a
single, unsplit argument string, and extra arguments get silently
ignored, as usual.
buil() complains about the argument when snxtsct() fails. Misleading
when the argument is fine, but snxtsct() fails due to bad conditional
argument.
Same for radar() with snxtitem().
It misuses snxtsct() and snxtitem() to find out whether the first
argument looks like sectors or like ships, which doesn't work with a
bad conditional argument.
Not worth fixing now; it's been disabled since 4.0.1, and broken at
least since commit 2fc1e74a (v4.3.0) broke its sector/ship
disambiguation via third argument.
It assumes snxtsct() fails only when the argument can't be parsed. It
can also fail when the condition argument has errors. `map # ?xxx'
first complains about xxx, then maps around ship#0. Broken since
Chainsaw 2 introduced smap, pmap and lmap.
Use sarg_type() to recognize sectors vs. unit argument. `map # ?xxx'
now fails as it should.
Subtle side effect: do_map() no longer prompts for argument "",
because snxtsct() is now guarded by sarg_type(). Impact on callers:
* display_region_map() is not affected, because it never passes "".
* map() passes on "" arguments. Change it to prompt in that case.
Consistent with how other commands behave. No functional change.
* do_unit_move() passes on "" arguments. Keep it that way. This
changes navigate and march sub-commands 'M' and 'B' not to prompt
for "" arguments, which is consistent with sub-command 'm' of move,
test and transport.
snxtsct() and snxtitem() fail when the condition argument is bad.
satmap() didn't check for failure. Due to the way snxtsct() and
snxtitem() work, bad condition arguments were reported and otherwise
ignored.
Destination arguments can be a path or sector coordinates.
do_unit_move() passes the argument buffer to unit_path() to convert
coordinates to a path. If unit_path() fails, do_unit_move() still
interprets the argument as path.
This is correct when unit_path() fails because the argument is not
coordinates. But it can also fail when it is coordinates, namely when
the destination isn't reachable, when the path to it is too long, or
when the ships or land units aren't together. Then do_unit_move()
interprets coordinates as path, and rejects them with "Legal
directions are:".
Except when a land unit's destination read from a march prompt isn't
reachable, because then unit_path() empties the argument buffer that
do_unit_move() uses.
Change unit_path() to succeed when the argument is not coordinates.
Make do_unit_move() discard the argument when unit_path() fails,
i.e. when it is bad coordinates. unit_path() emptying the argument no
longer has an effect, drop it.
Don't compute the distance from the path, use the path cost. The
actual path is no longer needed, and we can use path_find() instead of
BestShipPath().
Destinations are no longer treated as unreachable when the best path
is longer than 1023 characters.
