This is the next bundle in my UI refactoring efforts. I have managed to get
rid of most of the flag list hacks and also bit the bullet and made the
option definitions local where possible.

Of particular interest is the whatsnew command for which the flags/options
business was really hard to disentangle. I think the result is a huge
improvement: the is now a single place where the differing defaults for -s
are determined.

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

patch 6c3edcb27ad68afe7258e1fb56a33686e284d354
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Apr  1 12:40:32 CEST 2017
  * remove --overview option, add -V as --version alias
  
  The --overview option is useless; it is the same as --help --verbose but
  help does not honor --verbose anywhere.
  The -V is a common alias for --version and better than -v which could be
  confused with --verbose.

patch 6b2ba3dd46198cf0c9897f8c134c29d03dda9541
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Apr  1 12:46:57 CEST 2017
  * removed all help options
  
  The help command does not in fact support any options, so why let users
  specify them?

patch 57ceb349710170d8698d42ef248da5bab83a8542
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Apr  1 13:29:02 CEST 2017
  * cleanup imports in diff command

patch 704f473f35d8c72b8037b641375e9a25ca100010
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Wed Mar 29 23:04:40 CEST 2017
  * get rid of almost all flag list hacking
  
  The only command where this was highly non-trivial was whatsnew. For the
  others, a few simple refactorings in the option definitions made the changes
  quite mechanical. For instance, I have changed options where the user code
  (the command implementations, mostly) had to supply a default, with separate
  options (e.g. conficts -> conflictsYes, conflictsNo). This makes it clearer
  which commands have which defaults and makes it less probable to
  accidentally use one default for the options definition and another one for
  parsing the option.
  
  The remaining few cases are those where we need to do processing of the raw
  flag list: the match option hack for init (--to-match is converted to
  --match), and the WorkRepo and NewRepo flags. My plan for removing these is:
  (1) change definition of the matching options darcs init takes, and
  (2) but IO into DarcsOptDescr, so we can do the needed processing on
  options, rather than the raw flag list

patch 21efdfd6bbc5fed9f2b71b1f92619d5fc94ad404
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Apr  1 13:12:21 CEST 2017
  * make option definitions local to commands where possible
  
  The rationale is that this allows to omit type signatures. These signatures
  are useless in practice and a hindrance whenever options are refactored. The
  change is completely mechanical except where we share option definitions
  between related commands.

patch daa5eb059ec0f68150c4ae632732de19299dcda3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Mar 31 12:21:54 CEST 2017
  * fixed type option argument to --siblings

Attachments

• patch-preview.txt
• remove-__overview-option_-add-_v-as-__version-alias.dpatch
• unnamed

>  * get rid of almost all flag list hacking

> data ExternalDiff = ExternalDiff
>   { _diffCmd :: Maybe String
>   , _diffOpts :: [String]
>   , _diffUnified :: Bool
>   } deriving (Eq, Show)

> __extDiffCmd :: PrimDarcsOption (Maybe String)
> __extDiffCmd = singleStrArg [] ["diff-command"] F.DiffCmd arg 
"COMMAND"

I know it's not all introduced by your patch, but what's the intention 
of the '_' prefix to symbols? I've always understood it to mean "this 
symbol won't be used, don't warn about it", but that's not the case 
here.

>  * remove --overview option, add -V as --version alias
>  * removed all help options
>  * cleanup imports in diff command
>  * fixed type option argument to --siblings

All fine.

>  * get rid of almost all flag list hacking

In whatsnew, lookForAdds etc and summary were nastily entangled as you 
mention, and I think the replacement is much clearer.

Some of the 'lookForAdds' behaviour used to be disabled if NoSummary 
was explicitly chosen:

>   isLookForAdds = lookForAdds opts == O.YesLookForAdds && parseFlags 
O.summary opts /= Just O.NoSummary

Your new code seems to be missing that effect, and from a quick try 
with old and new binaries, I think there's indeed a behaviour 
difference in 'darcs wh -l --no-summary'.

> Ganesh Sittampalam <ganesh@earth.li> added the comment:
> 
>>  * get rid of almost all flag list hacking
> 
>> data ExternalDiff = ExternalDiff
>>   { _diffCmd :: Maybe String
>>   , _diffOpts :: [String]
>>   , _diffUnified :: Bool
>>   } deriving (Eq, Show)
> 
>> __extDiffCmd :: PrimDarcsOption (Maybe String)
>> __extDiffCmd = singleStrArg [] ["diff-command"] F.DiffCmd arg 
> "COMMAND"
> 
> I know it's not all introduced by your patch, but what's the intention 
> of the '_' prefix to symbols? I've always understood it to mean "this 
> symbol won't be used, don't warn about it", but that's not the case 
> here.

Right, this should be cleaned up. (I guess this is historical: probably
the field selectors weren't used when the data type was introduced, then
later someone re-wrote some function and used them but did not change
the name back to no-underscore?)

Ganesh Sittampalam <ganesh@earth.li> added the comment:
> Some of the 'lookForAdds' behaviour used to be disabled if NoSummary 
> was explicitly chosen:
> 
>>   isLookForAdds = lookForAdds opts == O.YesLookForAdds && parseFlags 
> O.summary opts /= Just O.NoSummary
> 
> Your new code seems to be missing that effect, and from a quick try 
> with old and new binaries, I think there's indeed a behaviour 
> difference in 'darcs wh -l --no-summary'.

Hm, I though I had this case covered but apparently I did not. Will try
to fix.

Could you encode the old behavior as a test case?

Cheers Ben

adding a failing test to encode the old behaviour

1 patch for repository darcs-unstable@darcs.net:screened:

patch 6cae0d5d382a8a536c89c014d28fbcea93668844
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Apr 18 22:27:03 BST 2017
  * add test for current behaviour of darcs wh -l --no-summary

Attachments

• add-test-for-current-behaviour-of-darcs-wh-_l-__no_summary.dpatch
• patch-preview.txt
• unnamed

> Ganesh Sittampalam <ganesh@earth.li> added the comment:
>> Some of the 'lookForAdds' behaviour used to be disabled if NoSummary 
>> was explicitly chosen:
>>
>>>   isLookForAdds = lookForAdds opts == O.YesLookForAdds && parseFlags 
>> O.summary opts /= Just O.NoSummary
>>
>> Your new code seems to be missing that effect, and from a quick try 
>> with old and new binaries, I think there's indeed a behaviour 
>> difference in 'darcs wh -l --no-summary'.

I am not able to reproduce this. I have added a test that checks for
this special case, see Patch1557.

>> Ganesh Sittampalam <ganesh@earth.li> added the comment:
>>> Some of the 'lookForAdds' behaviour used to be disabled if NoSummary 
>>> was explicitly chosen:
>>>
>>>>   isLookForAdds = lookForAdds opts == O.YesLookForAdds && parseFlags 
>>> O.summary opts /= Just O.NoSummary
>>>
>>> Your new code seems to be missing that effect, and from a quick try 
>>> with old and new binaries, I think there's indeed a behaviour 
>>> difference in 'darcs wh -l --no-summary'.
> 
> I am not able to reproduce this. I have added a test that checks for
> this special case, see Patch1557.

I have obsoleted this test as it tests the wrong thing. Sorry.

It seems that indeed --no-summary does have an effect, but only for
files that have been added and not for the un-added files. I am
investigating...

> > I am not able to reproduce this. I have added a test that checks
> > for this special case, see Patch1557.

> I have obsoleted this test as it tests the wrong thing. Sorry.

> It seems that indeed --no-summary does have an effect, but only for
> files that have been added and not for the un-added files. I am
> investigating...

Did you see the test case I sent to this patch last night (I already 
pushed it to screened)?

>> I have obsoleted this test as it tests the wrong thing. Sorry.
> 
>> It seems that indeed --no-summary does have an effect, but only for
>> files that have been added and not for the un-added files. I am
>> investigating...
> 
> Did you see the test case I sent to this patch last night (I already 
> pushed it to screened)?

Yes, in fact it was your test that made me realize that mine was faulty.
The problem is fixed now, BTW, see patch1558. And as I write this I see
that I should have used '[patch1549]' in the subject. Will fix.

It took me a while to figure out that --look-for-adds plus --summary
(whether implied or not) is the one special case for whatsnew, everything
else can be handled generically using allInterestingChanges. The code should
make this clear(er) now.

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

patch 169d542a3b91ee01447590f447219cd31032156c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Apr 20 08:14:05 CEST 2017
  * whatsnew: fixed wrong use of term hunk in help texts and comments

patch bb0f962634b0dc75c99e4b9e36fa97ebdf613f06
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Apr 20 10:44:13 CEST 2017
  * whatsnew: minor cleanups (fix duplicate import, comments, indentation)

patch 47d44607a14efc3cb4ebe397f2a41c1b1ca70fe2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Apr 20 11:17:33 CEST 2017
  * fix whatsnew -l --no-summary

Attachments

• patch-preview.txt
• unnamed
• whatsnew_-fixed-wrong-use-of-term-hunk-in-help-texts-and-comments.dpatch

> 3 patches for repository http://darcs.net/screened:
> 
> patch 169d542a3b91ee01447590f447219cd31032156c
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Thu Apr 20 08:14:05 CEST 2017
>   * whatsnew: fixed wrong use of term hunk in help texts and comments
> 
> patch bb0f962634b0dc75c99e4b9e36fa97ebdf613f06
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Thu Apr 20 10:44:13 CEST 2017
>   * whatsnew: minor cleanups (fix duplicate import, comments, indentation)
> 
> patch 47d44607a14efc3cb4ebe397f2a41c1b1ca70fe2
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Thu Apr 20 11:17:33 CEST 2017
>   * fix whatsnew -l --no-summary

Note: these patches go on top of the already sent bundle, they are not a
replacement.

Follow-up patch in response to Ganesh' review question.

1 patch for repository http://darcs.net/screened:

patch fd3a71840e799f948244a3d22a61018064b6359d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Apr 20 22:39:02 CEST 2017
  * remove leading underscore from field names of ExternalDiff

Attachments

• patch-preview.txt
• remove-leading-underscore-from-field-names-of-externaldiff.dpatch
• unnamed

For what it's worth, it looks like the behaviour of '--look-for-moves --
no-summary' has also changed. But I think the old behaviour is just wrong 
and the new behaviour is much better.

From Ganesh' comments, I beliebe we can accept this bundle.

Also accepting "add test for current behaviour of darcs wh -l
--no-summary", and "remove leading underscore from field names of
ExternalDiff".