This is work building up to my attempt at implementing the recommendations in
http://wiki.darcs.net/DefaultSwitches

So far, no actual changes, just some re-arranging of the furniture.
My hope is that we can introduce a
  DarcsMutuallyExclusiveOption
which will help us to express defaults a little bit more easily.

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

Mon Jun 21 01:02:07 BST 2010  Eric Kow <kowey@darcs.net>
  * Rename optionFromDarcsoption to optionFromDarcsOption.

Mon Jun 21 01:19:26 BST 2010  Eric Kow <kowey@darcs.net>
  * Flatten DarcsOption type.
  Distinguish between DarcsAtomicOptions and DarcsOption.

Mon Jun 21 01:22:00 BST 2010  Eric Kow <kowey@darcs.net>
  * Better use of Data.Maybe helpers in Darcs.Arguments.

Attachments

• rename-optionfromdarcsoption-to-optionfromdarcsoption_.dpatch
• unnamed

One more bit of moving furniture around...

Mon Jun 21 01:36:01 BST 2010  Eric Kow <kowey@darcs.net>
  * Refactor traversal of atomic options in DarcsOption.

Attachments

• rename-optionfromdarcsoption-to-optionfromdarcsoption_.dpatch
• unnamed

Hi Florent,

Would you mind reviewing this?  (I feel a bit guilty about shifting UI
stuff to you since I'd like to avoid hyper-specialisation and improve
bus-factor robustness).

Not sure if you see where I'm going with this.

Rejecting the main bits of this bundle may be the wisest move, as there
is a strong chance I'm barking up the wrong tree with this
DarcsMutuallyExclusiveOption business.

I was really just trying to find the best way (in the long term) to get
the default switches flipped.  On the one hand, this may be silly, since
one should not be flipping default switches very much.  On the other
hand, anything we could do to make the code more transparent should be a
bonus...

One bit of trouble I may run into is when you have flags with optional
or required arguments instead of NoArg.  You can't just compare equality
on DarcsFlag because the constructors you're passing around functions
:-/  I suppose I may end up having something that only works with NoArg
cases and not bother with the rest.  [I forget why I think this is a
problem, just making a note of it now]

In conjunction with patch259, I think this implements the recommendations
in http://wiki.darcs.net/DefaultSwitches

Along the way, it *tries* to make default behaviours a little bit more
explicit.

I also threw in universal --no-cache which has been bugging us for a while.

Note that one missing behaviour is that of reminding users how to set the
default repo.  The problem is that I'm not sure what the right advice to
give is.  Can we just tell them to run the command again.  Will it work if
(for example) you don't have any patches to push?

There's some messiness in this bundle, sorry!  Unfortunately, I'm offline till
10 July, so I think making your own corrections may be the fastest way to fix
anything I did wrong :-(

14 patches for repository http://darcs.net:

Mon Jun 21 01:02:07 BST 2010  Eric Kow <kowey@darcs.net>
  * Rename optionFromDarcsoption to optionFromDarcsOption.

Mon Jun 21 01:19:26 BST 2010  Eric Kow <kowey@darcs.net>
  * Flatten DarcsOption type.
  Distinguish between DarcsAtomicOptions and DarcsOption.

Mon Jun 21 01:22:00 BST 2010  Eric Kow <kowey@darcs.net>
  * Better use of Data.Maybe helpers in Darcs.Arguments.

Mon Jun 21 01:36:01 BST 2010  Eric Kow <kowey@darcs.net>
  * Refactor traversal of atomic options in DarcsOption.

Thu Jul  1 15:25:43 BST 2010  Eric Kow <kowey@darcs.net>
  * Update optimizeHTTP for new two-layer DarcsOption.

Thu Jul  1 15:49:16 BST 2010  Eric Kow <kowey@darcs.net>
  * Fix use of atomicOptions in Darcs.ArgumentDefaults.

Thu Jul  1 16:18:16 BST 2010  Eric Kow <kowey@darcs.net>
  * General purpose function for setting defaults.

Thu Jul  1 16:35:11 BST 2010  Eric Kow <kowey@darcs.net>
  * Fix missing type signature.

Thu Jul  1 16:50:41 BST 2010  Eric Kow <kowey@darcs.net>
  * Add a notion of mutually exclusive options with a default.

Thu Jul  1 16:50:45 BST 2010  Eric Kow <kowey@darcs.net>
  * Express --{no-,}edit-description as a mutually exclusive option.

Thu Jul  1 16:52:53 BST 2010  Eric Kow <kowey@darcs.net>
  * Make --edit-description the default.

Thu Jul  1 17:01:38 BST 2010  Eric Kow <kowey@darcs.net>
  * Express --{no-,}set-default as a mutually exclusive option.

Thu Jul  1 17:03:52 BST 2010  Eric Kow <kowey@darcs.net>
  * Do not set default repo by default in push, pull, send, fetch.
  As discussed during the 2010-03 sprint and documented in
  http://wiki.darcs.net/DefaultSwitches

Thu Jul  1 17:14:28 BST 2010  Eric Kow <kowey@darcs.net>
  * Make --no-cache an advanced option in all commands.

Attachments

• rename-optionfromdarcsoption-to-optionfromdarcsoption_.dpatch
• unnamed

Hi!

I have reviewed all of Eric's changes and found a couple of issues. One,
--no-edit was being ignored if --edit was ever given. This is a design issue,
with [DarcsFlag] style of handling options. I have added a more-or-less generic
solution for this that uses the new DarcsMutuallyExclusive option type to keep
at most one occurence of an option from the mutually-exlusive set. This is the
first patch I added.

The second, tests got pretty broken by the new defaults (this was the easier
part once the above was addressed: just adding --set-default in one place and
--no-edit to the defaults for all of the testsuite fixed things).

The last (minor) thing is that --no-cache appeared twice in some option lists
(caught by the testsuite as well). Fixed by a separate patch.

Other than those, I think the bundle can go in. I would appreciate a timely
review of the three patches I have tacked on to the end.

Btw., there's a problem with the change to ShellHarness: if people are using
just "cabal build" and friends, the change to ShellHarness won't trigger a
Setup rebuild and they will get test failures. Need to "touch Setup.lhs" to
remedy that. I don't know a good fix...

Yours,
   Petr.

17 patches for repository darcs-unstable@darcs.net:darcs:

Mon Jun 21 02:02:07 CEST 2010  Eric Kow <kowey@darcs.net>
  * Rename optionFromDarcsoption to optionFromDarcsOption.

Mon Jun 21 02:19:26 CEST 2010  Eric Kow <kowey@darcs.net>
  * Flatten DarcsOption type.
  Distinguish between DarcsAtomicOptions and DarcsOption.

Mon Jun 21 02:22:00 CEST 2010  Eric Kow <kowey@darcs.net>
  * Better use of Data.Maybe helpers in Darcs.Arguments.

Mon Jun 21 02:36:01 CEST 2010  Eric Kow <kowey@darcs.net>
  * Refactor traversal of atomic options in DarcsOption.

Thu Jul  1 16:25:43 CEST 2010  Eric Kow <kowey@darcs.net>
  * Update optimizeHTTP for new two-layer DarcsOption.

Thu Jul  1 16:49:16 CEST 2010  Eric Kow <kowey@darcs.net>
  * Fix use of atomicOptions in Darcs.ArgumentDefaults.

Thu Jul  1 17:18:16 CEST 2010  Eric Kow <kowey@darcs.net>
  * General purpose function for setting defaults.

Thu Jul  1 17:35:11 CEST 2010  Eric Kow <kowey@darcs.net>
  * Fix missing type signature.

Thu Jul  1 17:50:41 CEST 2010  Eric Kow <kowey@darcs.net>
  * Add a notion of mutually exclusive options with a default.

Thu Jul  1 17:50:45 CEST 2010  Eric Kow <kowey@darcs.net>
  * Express --{no-,}edit-description as a mutually exclusive option.

Thu Jul  1 17:52:53 CEST 2010  Eric Kow <kowey@darcs.net>
  * Make --edit-description the default.

Thu Jul  1 18:01:38 CEST 2010  Eric Kow <kowey@darcs.net>
  * Express --{no-,}set-default as a mutually exclusive option.

Thu Jul  1 18:03:52 CEST 2010  Eric Kow <kowey@darcs.net>
  * Do not set default repo by default in push, pull, send, fetch.
  As discussed during the 2010-03 sprint and documented in
  http://wiki.darcs.net/DefaultSwitches

Thu Jul  1 18:14:28 CEST 2010  Eric Kow <kowey@darcs.net>
  * Make --no-cache an advanced option in all commands.

Thu Jul  8 21:49:04 CEST 2010  Petr Rockai <me@mornfall.net>
  * Correctly handle conflicts arising from DarcsMutuallyExclusive options.

Thu Jul  8 21:50:14 CEST 2010  Petr Rockai <me@mornfall.net>
  * Avoid adding noCache twice to parameter lists.

Thu Jul  8 21:51:00 CEST 2010  Petr Rockai <me@mornfall.net>
  * Fix tests in light of recent default flag changes.

Attachments

• rename-optionfromdarcsoption-to-optionfromdarcsoption_.dpatch
• unnamed

Hey thanks for the review!

I'm going to push this (and to branch 2.5 since changing the defaults
is work we explicitly agreed to do for Darcs 2.5).

We chatted a bit on IRC
http://irclog.perlgeek.de/darcs/2010-07-12#i_2543135

But in the long term, we really really need that DarcsFlag rethink
and a 3rd party library

- http://bugs.darcs.net/issue1157
- http://bugs.darcs.net/issue1550

On Thu, Jul 08, 2010 at 19:56:33 +0000, Petr Ročkai wrote:
> I have reviewed all of Eric's changes and found a couple of issues. One,
> --no-edit was being ignored if --edit was ever given. This is a design issue,
> with [DarcsFlag] style of handling options. I have added a more-or-less generic
> solution for this that uses the new DarcsMutuallyExclusive option type to keep
> at most one occurence of an option from the mutually-exlusive set. This is the
> first patch I added.

I thought I might provide some background information on what I think
Petr means when he refers to design issues.

The big picture is that we want Darcs to support contradictory flags
appearing in the command line so that we can have defaults in the
following order of priority

1. command line
2. _darcs/prefs/defaults
3. ${HOME}/.darcs/defaults
4. command defaults
5. flag defaults (see Darcs.Flag.getBoolFlag)

Right now, we support this is by cramming all the flags into a list and
relying on the convention that in the case of mutually exclusive flags,
the first flag wins.

One design flaw is that is that we don't actually enforce this mutual
exclusivity anywhere, which is how bugs like the one Petr spotted show
up.

Instead we rely on the hapless Darcs programmer to remember that a flag
is part of a mutually exclusive set, and test not just that flag X
exists but that its counterparts not-X do not exist.  Yuck.  If said
programmer is lucky, she gets a helper function like

willIgnoreTimes :: [DarcsFlag] -> Bool
willIgnoreTimes = getBoolFlag IgnoreTimes DontIgnoreTimes

... but (A) there's no guarantee that all mutually exclusive flags provide
such a helper function and (B) she may still forget to run it.

By the way, for the interested, here's the back-end to the helper
function that implements this first flag wins behaviour

getBoolFlag :: DarcsFlag -> DarcsFlag -> [DarcsFlag] -> Bool
getBoolFlag t f = lastWord [(t, True), (f, False)] False

-- | @lastWord [(flag, value)] default opts@ scans @opts@ for a flag
-- in the list and returns the value of the first match, or @default@
-- if none is found.
lastWord :: [(DarcsFlag,a)] -> a -> [DarcsFlag] -> a
lastWord known_flags = foldr . flip $ \ def -> fromMaybe def . flip lookup known_flags

> Btw., there's a problem with the change to ShellHarness: if people are using
> just "cabal build" and friends, the change to ShellHarness won't trigger a
> Setup rebuild and they will get test failures. Need to "touch Setup.lhs" to
> remedy that. I don't know a good fix...

Thanks for pointing this out.

Correctly handle conflicts arising from DarcsMutuallyExclusive options.
-----------------------------------------------------------------------
OK so how my patch bundle and Petr's changes fit into this?  Basically,
my patch bundle introduces a way of explicitly encoding the fact that
some flag sets are mutually exclusive.  It does not actually use this
encoding for anything (except for some minor refactoring).

Petr's patch goes a step further by exploiting the
DarcsMutuallyExclusive knowledge to do a one-time nub of [DarcsFlag].
An alternative solution would been to write a willEditDescription
function using lastWord above, but Petr's solution is a bit more
general.

> +nubOptions [] opts = opts
> +nubOptions (DarcsMutuallyExclusive ch _:options) opts = nubOptions options $ collapse opts
> +  where collapse (x:xs) | x `elem` flags ch = x : clear xs
> +                        | otherwise = x : collapse xs
> +        collapse [] = []

The *first* time we see a flag in a mutually exclusive set, we clean
up the set to ignore any other flags from that set.

> +        clear (x:xs) | x `elem` flags ch = clear xs
> +                     | otherwise = x : clear xs
> +        clear [] = []

Is that just

  clear = filter (`notElem` flags ch)

Feel free to just push if you want to just trivially refactor this.

> +        flags (DarcsNoArgOption _ _ fl _:xs) = fl : flags xs
> +        flags (_:xs) = flags xs
> +        flags [] = []

Maybe a mapMaybe refactor here?

> hunk ./src/Darcs/RunCommand.hs 107
>     Right () -> do
> -    specops <- addCommandDefaults cmd opts
> +    specops <- nubopts `fmap` addCommandDefaults cmd opts

This does seem like the right place to do this.  I don't think there
is any other place.

> -       where nth_arg n = nth_of n (commandExtraArgHelp cmd)
> +       where nubopts = nubOptions (uncurry (++) $ commandAlloptions cmd)
> +             nth_arg n = nth_of n (commandExtraArgHelp cmd)

I'd be personally inclined to cut this code a different way, by just
saying let allOptions = uncurry (++) $ commandAlloptions cmd

Avoid adding noCache twice to parameter lists.
----------------------------------------------
> Petr Rockai <me@mornfall.net>**20100708195014
>  Ignore-this: 59cf4dc50edb4c08367cbc29f321e431
> ] hunk ./src/Darcs/Arguments.lhs 1665
>         [ DarcsNoArgOption [] ["no-http-pipelining"] NoHTTPPipelining
>                            "disable HTTP pipelining"
>         ]
> -   , noCache, remoteDarcs ]
> +   , remoteDarcs ]

This looks obviously right.

Fix tests in light of recent default flag changes.
--------------------------------------------------
> Petr Rockai <me@mornfall.net>**20100708195100
>  Ignore-this: b8764f2105ed6e7870f4853041b90f9e
> ] hunk ./Distribution/ShellHarness.hs 65
>                  ,("DARCS_DONT_ESCAPE_ANYTHING","1")]
>          shell = takeWhile (/= '\n') bash
>      putStrLn $ "Using bash shell in '"++shell++"'"
> -    catch (appendFile (".darcs/defaults") "\nALL --ignore-times\n")
> +    catch (appendFile (".darcs/defaults") "\nALL ignore-times\nsend no-edit-description\n")
>            (\e -> fail $ "Unable to set preferences: " ++ show e)
>      run_helper shell tests []  (set_env myenv env)
>  
> hunk ./tests/filepath.sh 106
>  
>  # can handle .. path
>  cd temp3
> -darcs pull ../temp2 -p1 --all | grep -i 'Finished pulling'
> +darcs pull ../temp2 --set-default -p1 --all | grep -i 'Finished pulling'
>  darcs pull --dry-run | grep hello2
>  cd a/b
>  #[issue268] repodir with subdir

Thanks for catching this.  There's a conflict with recent work that
I'll try to fix myself.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.

This patch bundle (with 14 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-20e11d35303b3d44635b6eb111fbe18fc92d2807

This patch bundle (with 17 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-3f5e8c03b08b5b271d723b03993d519e536d1656

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-1491d65b63432619a8a3e65e8618e415ec3e2e12

This patch bundle (with 4 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-805b7ed6d8638e5ccde4b8f751d949cc98f332ef

This patch bundle (with 4 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-805b7ed6d8638e5ccde4b8f751d949cc98f332ef

This patch bundle (with 17 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-3f5e8c03b08b5b271d723b03993d519e536d1656

This patch bundle (with 14 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-20e11d35303b3d44635b6eb111fbe18fc92d2807

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-1491d65b63432619a8a3e65e8618e415ec3e2e12