This cleans up the remaining few bits where we still accessed and
manipulated the list of DarcsFlags manually i.e. without going through an
Option. This means we can finally get rid of the commandParsedFlags member
of DarcsFlag and thus the parsedFlags type parameter of DarcsCommand, the
WrappedCommand type, and the associated complications in the code.

8 patches for repository http://darcs.net/screened:

patch c7c565400f96649bc88838fd3bc5f6b9ead1902b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 22 23:40:47 CEST 2020
  * eliminate DarcsFlag list hacking for NewRepo and WorkRepoDir
  
  To make this more uniform, the init command now takes the O.reponame option
  instead of O.repoDir, like all other commands that create a new repo (clone,
  convert darcs2, convert import). This is even compatible since we still
  support --repodir as an alias for --repo-name.
  
  Converting a normal argument to a DarcsFlag now uses the standard option API
  i.e. ounparse, see the new function Darcs.UI.Flags.withNewRepo.

patch e943a574ee71258731c56e459e5861881ce2d086
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 23 00:10:09 CEST 2020
  * replace combined option workRepo with a plain function
  
  This option was artificial in that no command actually specified it. I
  always found its presence in Darcs.UI.Options.All confusing.

patch 82764afae894ee45e1901c27206859d02aa63d0b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 23 00:21:15 CEST 2020
  * rename option reponame to newRepo

patch 289db1878ecea935bc29c4523a319dbc9a66d21b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 23 00:48:58 CEST 2020
  * remove DarcsFlag list hacking for match options
  
  This was used internally to convert to-xxx flags to plain xxx for the clone
  command. This is now done after converting to MatchFlags. This means
  Darcs.UI.Flags no longer has to export any DarcsFlag data constructors.

patch cdc4f80e519e11912b46f0ba84ae91363d44ce0f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 23 14:29:52 CEST 2020
  * avoid use of matchAny in commands

patch 2165988e5457bce25154814f1437f86682760239
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 23 14:33:47 CEST 2020
  * fix: add missing cases in getMatchPattern

patch 5d69a523b55fb16cbcdb3543e524f3cf7d513af6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 23 14:39:55 CEST 2020
  * add comments about use of RemoteRepo DarcsFlag constructor

patch 548ab845da24163440e28bc4c34fe5b96830497a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 23 15:36:45 CEST 2020
  * remove option type parameter from DarcsCommand
  
  This is basically a complete rollback of
  
  patch 6ef3e49796ef64ae23bde58bfc4ea6720e8fd14d
  Author: Ganesh Sittampalam <ganesh@earth.li>
  Date:   Fri Nov 14 07:18:20 CET 2014
    * Make the options type used by a command into a type parameter.
  
  Made possible since we no longer do any low-level DarcsFlag hacking.

Attachments

• eliminate-darcsflag-list-hacking-for-newrepo-and-workrepodir.dpatch
• patch-preview.txt
• unnamed

On 23/06/2020 17:43, Ben Franksen wrote:

>   * eliminate DarcsFlag list hacking for NewRepo and WorkRepoDir

>   To make this more uniform, the init command now takes the O.reponame option
>   instead of O.repoDir, like all other commands that create a new repo (clone,
>   convert darcs2, convert import). This is even compatible since we still
>   support --repodir as an alias for --repo-name.
>   
>   Converting a normal argument to a DarcsFlag now uses the standard option API
>   i.e. ounparse, see the new function Darcs.UI.Flags.withNewRepo.

Looks ok - I haven't thought it through in detail but it sounds plausible.

>   * replace combined option workRepo with a plain function
>   
>   This option was artificial in that no command actually specified it. I
>   always found its presence in Darcs.UI.Options.All confusing.

OK

>   * rename option reponame to newRepo

OK

>   * remove DarcsFlag list hacking for match options

OK

>   * avoid use of matchAny in commands

OK (I'll assume the change is correct)

>   * fix: add missing cases in getMatchPattern

OK

>   * add comments about use of RemoteRepo DarcsFlag constructor

Thanks!

> +-- use of RemoteRepo data constructor is harmless here, if not ideal

This is a bit unclear without looking at the comment below about
accessing the flag list, and could get out of date if that gets changed.
But not a big deal.

>   * remove option type parameter from DarcsCommand
>   
>   This is basically a complete rollback of
>   
>   patch 6ef3e49796ef64ae23bde58bfc4ea6720e8fd14d
>   Author: Ganesh Sittampalam <ganesh@earth.li>
>   Date:   Fri Nov 14 07:18:20 CET 2014
>     * Make the options type used by a command into a type parameter.
>   
>   Made possible since we no longer do any low-level DarcsFlag hacking.

I barely remember the history of this, although the new state looks fine
to me. From what I recall/understand from this patch:

- We started transitioning from [DarcsFlag] to strongly typed option
records for each command, but didn't get very far and it's now on hold
or abandoned, but not rolled back. An example of a migrated command is
'amend'.

- I added this type parameter/explicit config translation step to make
it hard to forget to normalise [DarcsFlag] for commands that still use
it directly.

- Now we still have lots of commands that use [DarcsFlag] for options,
but because they treat it opaquely, the explicit normalise step isn't
needed.

Is that right?

I also find onormalise/defaultFlags in D.UI.Options.Core a bit confusing
now, although it's not directly touched by this patch. onormalise is now
only used by defaultFlags. Both of them have specifications that are
identical to their bodies. Perhaps there's some scope for simplification
here.

>>   * avoid use of matchAny in commands
> 
> OK (I'll assume the change is correct)

It is. The change means we interpret only those MatchFlags that the
command specifies (and documents!).

>> +-- use of RemoteRepo data constructor is harmless here, if not ideal
> 
> This is a bit unclear without looking at the comment below about
> accessing the flag list, and could get out of date if that gets changed.
> But not a big deal.

Good point though, see patch2060.

>>   * remove option type parameter from DarcsCommand
>>   
>>   This is basically a complete rollback of
>>   
>>   patch 6ef3e49796ef64ae23bde58bfc4ea6720e8fd14d
>>   
>>   Made possible since we no longer do any low-level DarcsFlag hacking.
> 
> I barely remember the history of this, although the new state looks fine
> to me. From what I recall/understand from this patch:
> 
> - We started transitioning from [DarcsFlag] to strongly typed option
> records for each command, but didn't get very far and it's now on hold
> or abandoned, but not rolled back. An example of a migrated command is
> 'amend'.

Yes. My take is that the final verdict here is still open, but I tend to
think that the effort should be abandoned. I found that this approach is
just too inflexible. Removing an option or merely changing the order of
options (which affects only the help output) entails a lot of churn, and
the benefits are negligable in practice. I have indeed thought about
rolling back this part, i.e. the config records in the amend and record
commands, but couldn't get myself motivated enough to actually do it.
Perhaps things would be different if we had anonymous records and at
some point in the future we might actually get them, or a good enough
approximation, which is another reason I haven't done a rollback yet.

> - I added this type parameter/explicit config translation step to make
> it hard to forget to normalise [DarcsFlag] for commands that still use
> it directly.
> 
> - Now we still have lots of commands that use [DarcsFlag] for options,
> but because they treat it opaquely, the explicit normalise step isn't
> needed.
> 
> Is that right?

Yes, that sums it up pretty well.

> I also find onormalise/defaultFlags in D.UI.Options.Core a bit confusing
> now, although it's not directly touched by this patch. onormalise is now
> only used by defaultFlags. Both of them have specifications that are
> identical to their bodies. Perhaps there's some scope for simplification
> here.

It would be possible to inline onormalize now, but I don't think there
is much scope for simplification beyond that, because oparse and
ounparse are primitives. We'd end up with

  defaultFlags opts = (oparse opts . ounparse opts) id opts []

which I find harder to grok than the two separate functions. I am
usually keen to inline functions that are used only in one place, but in
this case I think keeping them separate makes it easier to understand
what goes on.

However, the docs for onormalize are outdated, i.e. the half-sentence

"which is useful as long as there is legacy code that circumvents the
'OptSpec' abstraction and directly tests for flag membership"

can and should be removed, see patch2061.