[darcs-devel] Darcs Argument Handling

[Ben Franksen]

Simon Michael wrote:
> As long the tests keep passing, this sounds very good.

Thanks! (Honestly I wasn't sure the silence might mean I offended everybody 
with the harsh critique...)

I have done some preliminary attempts with a Config record, but in order to 
make proper use of it I must change Darcs.UI.Commands.Command and this will 
affect every command. So I won't get a working (testable) version until all 
the command modules have been re-factored. That will be a lot of work and 
I'd *really* like to get some more opinions from the other core devs before 
I commit to this.

In the meantime I re-factored large parts of Darcs.UI.Arguments. The idea 
was to make more use of DarcsMutuallyExclusive options and therefore ensure 
that the flag set contains all the default flags, opening the possibility to 
simplify the flag testing code.

A large majority of the options should in fact be in a mutually exclusive 
group, among them (almost, see below) all options of the yes/no type. There 
are also some options that strongly suggest they belong to a mutually 
exclusive group, but when you test it or look at the implementation you find 
that the actual behavior is different from what the name suggests. (As an 
aside, this effort finally led me to some understanding of how all the 
different parts of the darcs UI work together.)

And yes: the test-suite. Very nice piece of work, that.

And quite useful, too. I have just been running it on my working branch and 
got a few failures. I investigated and found that I had misunderstood the 
semantics of one option pair. This is an interesting case, because it's a 
perfect example of what I hinted at above, so let me briefly explain.

The tests failed because I turned Darcs.UI.Arguments.patchIndex into a mutex 
group with PatchIndexFlag as the default flag. That broke the code in 
Darcs.UI.Commands.Optimize:

  if OptimizePristine `elem` opts then
    doOptimizePristine opts repository
  else if PatchIndexFlag `elem` opts then
    createOrUpdatePatchIndexDisk repository
  else if NoPatchIndexFlag `elem` opts then
    deletePatchIndex repodir
  else ...

because with my changes, PatchIndexFlag would now always be included in the 
flag list. Apparently, although PatchIndexFlag and NoPatchIndexFlag are 
indeed mutually exclusive, none of them is on by default and both cause 
different things to happen. So they actually aren't used as options, but 
rather as sub-commands of optimize, on par with OptimizePristine.

(The help text is a bit weak here, it just says
      --patch-index          Enable patch index
      --disable-patch-index  Disable patch index)

Anyway, the upshot is that optimize should be a super command with a number 
of sub-commands, rather than (ab-)using options for that. Similar 
observations apply to 'darcs convert'.

I think I will open a ticket to propose a UI change that will turn 'convert' 
and 'optimize' into super commands.

Meanwhile, I re-factored Darcs.UI.Arguments again to allow mutually 
exclusive groups without a default, removed the default marker from 
NoPatchIndexFlag, and now the tests all succeed. Yay!

One feature I am planning to implement is a runtime check that gives the 
user a warning (perhaps this should even be an error?) when mutually 
exclusive options are specified at the same level of precedence, e.g. both 
on the command line. The current behaviour is less than ideal: Sometimes one 
option is dominant, i.e. overrides its peers regardless of position. In 
other cases, which option wins depends on the position, i.e. options given 
earlier override those given later. (Some comments in the code suggest that 
at some point the intention was the opposite i.e. later overrides earlier.)

Unfortunately, System.Console.GetOpt has no direct support for mutually 
exclusive options, but I think I have an idea how to implement this in an 
elegant way.

On the side, I have also been investigating more advanced option parsing 
libraries, to see whether darcs could profit by using them instead of the 
somewhat out-dated System.Console.GetOpt. My favorite is optparse-
applicative, which I have used in a few tools I have written. It has 
borrowed many features from darcs and produces very similar usage/help 
output, except AFAIK it doesn't (yet?) support the distinction between basic 
and advanced options.

Enough for now. As always, comments are welcome, and sorry for the (again!) 
somewhat longer posting.

Cheers
Ben
-- 
"Make it so they have to reboot after every typo." -- Scott Adams