Subject: [PATCH 0/4] GNU Mcron and the (ice-9 getopt-long) module

From: Dale Mellor <[email-masked]>
Date: Sun, 02 Aug 2020 10:19:03 +0100

/Mcron/ is a GNU package which runs unattended jobs in the operating
system at dynamically computed times; it is 99% Guile but currently
shrouded in a thin veneer of C code for historical reasons, which
have by now vanished.

The Guile /getopt-long/ module parses a command lineʼs arguments for
options and their values according to a provided grammar.

In the process of removing the thin veneer of C code from around the
/GNU Mcron/ package, I am running up against niggles in the
implementation of the /(ice-9 getopt-long)/ module.  The intention
with /mcron/ has always been that a command-line argument be
provided which allows the user to request the display of the next
eight jobs to run, or allows the user to specify the number of such
jobs.  Thus the intention was that command-lines like ‘mcron -s4
file’, ‘mcron -s 4 file’, and ‘mcron -s file’ would all work; alas,
the last one, actually the most important case, doesnʼt with the
current module, which issues a fatal exit on the grounds that ‘file’
fails to meet predicated requirements of the option for ‘-s’ that it
should represent a decimal number.

It is clear that /getopt-long/ can do better than this, especially
if the consumer of the module provides predicates on values which
options can take, e.g. value should be numerical.  It can then
objectively decide that an argument should be taken to be a value,
an option itself, or a ‘loose’ argument.

There are other problems which can be cleared up with the enhanced
logic, as outlined in Point 2 below.

The following patches clear up the situation.

1) The first patch introduces some 28 new tests of the existing
   /getopt-long/ module; these are non-controversial and the current
   code passes all the tests, but they exercise more of the corner
   cases and provide confidence that a new implementation does not
   break existing behaviour.

2) The second patch inverts one test which I disagree with (see
   Point 3, below), and introduces 18 more tests which represent
   currently indeterminate and unsupported behaviour, some
   nevertheless desired by /mcron/; all of these create either test
   FAIL cases with the current code-base, or total panic-escape from
   the calling application.

   Some specific test failures:

   1. A command-line like ‘foo --test=’ produces a /test/ result with
      the empty string as value; I would expect /#t/ as the value
      (which indicates that the option is there but has no given
      value).

   2. A command-line with a negative number always errors.  According
      to the in-line documentation negative numbers canʼt ever appear
      loose on the command-line, but this seems like a case which
      might be realistic in real life and there is no reason to
      reject them.

   3. A command like ‘foo -abc d’ in which /b/ takes a mandatory
      argument and /c/ is an allowed option, errors out, but in my
      opinion in this case /b/ should take “c” as its value and the
      command-line as a whole is *not* erroneous.  If /b/ takes an
      optional value things are more tricky to deal with, but if
      there is a predicate on the values which /b/ can take, then the
      parser can make a clearer decision on taking /c/ as a value or
      another option.

      This might seem picky, but the problem is that command-lines
      are supplied by (possibly hostile) end-users, *not* by the
      /getopt-long/ module, and not by the application which consumes
      the module, either.  Thus this might be regarded as a security
      issue.

   4. The command ‘mcron -s file’, where /s/ takes an optional
      numeric value, errors out.

3) The third patch fixes up the /getopt-long/ module to pass all the
   new tests, as well as all of the existing ones (with the single
   exception outlined in Point 2.3 above).  Considering that the
   entire Guile build also depends on /getopt-long/, we can have
   some confidence that the changes do not bring any incompatibility
   with existing code.

4) The final patch fixes up various commentary and doc-strings in
   the code to emphasise the importance of predicates on optional
   values, and generally make things more concrete.