atc: changing altitude twice in a row triggers delayed turn

[Vadim Arshanskiy]

Ubuntu release: Ubuntu 22.04.2 LTS
bsdgames version: bsdgames/jammy,now 2.17-29 amd64

Steps to reproduce:
0. Have a beacon in the plane's flight path, far enough out to have ample time to execute the steps below.
1. Give a plane a command for a delayed turn. Example: "atxab0" ("a: turn to 180 at beacon #0")
2. Tell the plane to change altitude. Example (assuming plane is at 7000 feet): "aa3" ("a: altitude: 3000 feet")
3. Before the plane reaches the new altitude, issue the exact same altitude command again. Example: "aa3".
Expected result: plane continues traveling towards the beacon, and executes the turn once there.
Actual result: plane turns immediately, before reaching the beacon.

Root Cause TL;DR: an altitude command that passes all the checks but does not change new_altitude looks like a turn command.

Root Cause details:

1. The following code in setalt() treats the 2nd altitude command as actionable (telling the plane to do something new):
 if ((p.altitude == c - '0') && (p.new_altitude == p.altitude))
  return ("Already at that altitude");
This passes because (p.altitude != c - 0).

2. Meanwhile, the following code in getcommand() treats the result of setalt() as *not* actionable:
 if (pp->new_altitude != p.new_altitude)
  pp->new_altitude = p.new_altitude;
 else if (pp->status != p.status)
  pp->status = p.status;
 else {
  pp->new_dir = p.new_dir;
  pp->delayd = p.delayd;
  pp->delayd_no = p.delayd_no;
 }
As far as it is concerned, new_altitude has not changed, so the command must be for something else -- which falls into the last branch: a turn command.

3. That turn branch would be benign, except earlier setplane() clears p.delayd:
 memcpy(&p, pp, sizeof (p));
 p.delayd = 0;
So the turn branch erases the turn's delay-till-beacon qualifier with this line
  pp->delayd_no = p.delayd_no;

Proposed Fix: I believe the assumption being violated is: "If the command passed all the checks, then it must be actionable." Short of refactoring a bunch, setalt() should honor that assumption by rejecting any command that does not change new_altitude. setrelalt() has the same issue and the same fix. So I factored out the change check and replaced both those functions as follows:

const char *
setalt_numer(requested)
 int requested;
{
 if (p.new_altitude == requested) {
  if (p.altitude < requested) {
   return ("Already climbing to that altitude");
  } else if (p.altitude > requested) {
   return ("Already descending to that altitude");
  } else {
   return ("Already at that altitude");
  }
 }
 p.new_altitude = requested;
 return (NULL);
}

const char *
setalt(c)
 char c;
{
 return setalt_numer(c - '0');
}

const char *
setrelalt(c)
 char c;
{
 int requested;
 if (c == 0)
  return ("altitude not changed");

 switch (dir) {
 case D_UP:
  requested = p.altitude + c - '0';
  break;
 case D_DOWN:
  requested = p.altitude - (c - '0');
  break;
 default:
  return ("Unknown case in setrelalt! Get help!");
  break;
 }
 if (requested < 0)
  return ("Altitude would be too low");
 else if (requested > 9)
  return ("Altitude would be too high");
 return setalt_numer(requested);
}

Apologies for not attaching this as a patch: I am not yet skilled in generating machine-readable patches.

General Comments:
I would like to try to push this fix upstream, and I have other fixes/changes to share. What *is* the upstream for this repo?
The only thing I've found is https://sourceforge.net/p/bsd-games/, which is now on version 3.3 and atc there is almost unrecognizable in file structure after a sweeping refactor "10e1ef6 Cleanup atc" -- is that your upstream? If so, is it officially diverged into irrelevance?

Suppose in addition to bugfixes I want to push gameplay enhancements that arguably change the game balance (in particular: turn commands delayed until the plane reaches a line rather than a point). Is there anybody who can rule with authority on whether pulling them in is beneficial or detrimental?