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 <m...@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 <m...@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.
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list darcs-users@darcs.net http://lists.osuosl.org/mailman/listinfo/darcs-users
