lookup_tech() and friends return an advance (etc) from a secfile entry.

In S1_14 we had:

with the following possibilities:

• Entry not in secfile: fatal (secfile_lookup_str() kills server)
• Entry exists in secfile:
  • It's the string "Never":
    • required: fatal (find_tech_by_name() won't find it)
    • !required: return A_LAST
  • It's an unmatchable string (garbage):
    • required: fatal
    • !required: return A_LAST (and moan at LOG_ERROR)
  • It's a matchable string (success case):
    • return its id

Advances became "struct advance *"s rather than ints by S2_0. In the process, I think the logic got broken. Today we have:

(where the old "required" is logically loglevel>LOG_FATAL (i.e., LOG_ERROR etc)

• Entry not in secfile: return A_NEVER (==NULL)
• Entry exists in secfile:
  • It's the string "Never":
    • required: fatal (advance_by_rule_name() won't find it, ruleset_error() dies)
    • !required: return A_NEVER
  • It's an unmatchable string (garbage):
    • required: fatal
    • !required: return A_NEVER (and moan at loglevel)
  • It's a matchable string (success case):
    • return its id

So, the old version could never return A_LAST if "required" was set, but the new version can return A_NEVER even in this case. I suspect callers aren't prepared for that. Indeed, commenting out tech_req for a unit causes a segfault.

It would seem more useful for the "required" behaviour to be never to return A_NEVER. The details are arguable, however.