On Fri, Jul 16, 2010 at 21:28:59 +0000, Petr Ročkai wrote: > Thu Jul 15 02:08:22 CEST 2010 Petr Rockai <m...@mornfall.net> > * Remove --nolinks, since its scope and usefulness is very limited.
Still waiting on Kevin to comment. If he blesses this work, and I'm not
around, feel free to push this along with the RemoteDarcs and
Compression patches.
I might also just push this after a couple of days if he doesn't get a
chance to comment by then. We could always open a ticket and
reintroduce the option in the future. While I'm a big fan of
slowly-slowly, I also think it makes sense to adjust to context. So if
it's something relatively obscure like --nolinks, we can afford to move
a little more swiftly.
> Thu Jul 15 02:33:20 CEST 2010 Petr Rockai <m...@mornfall.net>
> * Replace some [DarcsFlag] uses with newly introduced RemoteDarcs.
This patch has a semantic dependency on the no-links patch (discovered
by compile-time error, as there was no textual dependency or explicit
ask-deps).
Note that it would not have been reasonable IMHO to expect Petr to
think to use --ask-deps.
This is one of those cases where Darcs textual-dependency based
cherry-picking gives you a false positive, which leads some people to
claim that the Darcs Theory of Patches "isn't useful in practice".
I'd say that our claim is that our colleagues in the rest of the DVCS
world, the not-so-cherry-pick-friendly ones, would have the "opposite"
problem of giving way too many false negatives (no cherry pick for you!)
and that the cost of those false negatives far outweighs the occasional
false positive from Darcs. In other words, being able to cherry pick
without rebasing is just too compelling a feature to give up... that or
we're just using the wrong DVCS :-)
"Cherry picking a waste of time; Darcs team switches to Mercurial"
> Thu Jul 15 14:31:40 CEST 2010 Petr Rockai <m...@mornfall.net>
> * Fix "head: empty list" bug in Darcs.Flags.RemoteDarcs.
>
> Thu Jul 15 02:34:49 CEST 2010 Petr Rockai <m...@mornfall.net>
> * Use Compression more widely, suppressing further [DarcsFlag] uses.
This patch has a textual dependency on the RemoteDarcs patch,
so while I'm happy with it as a reviewer, I'll hold off on
applying it until nolinks is in.
> Thu Jul 15 10:19:08 CEST 2010 Petr Rockai <m...@mornfall.net>
> * Remove [DarcsFlag] usage from Darcs.Patch.Bundle.
I'll leave this to Florent as part of another bundle if he does not
object.
> Fri Jul 16 22:53:11 CEST 2010 Petr Rockai <m...@mornfall.net>
> * Remove [DarcsFlag] parameters from apply.
Use Compression more widely, suppressing further [DarcsFlag] uses.
------------------------------------------------------------------
> - repository' <- tentativelyRemovePatches repository
> opts (oldp :>: NilFL)
> + repository' <- tentativelyRemovePatches repository
> (compression opts)
Snipping a lot of these lines which are just a straightforward
consequence of the meaty part of the your patch.
> -writePatch :: RepoPatch p => [DarcsFlag] -> Named p C(x y) -> IO FilePath
> -writePatch opts p =
> - do let writeFun = if NoCompress `elem` opts
> - then Patch.writePatch
> - else Patch.gzWritePatch
> +writePatch :: RepoPatch p => Compression -> Named p C(x y) -> IO FilePath
> +writePatch compr p =
> + do let writeFun = case compr of
> + NoCompression -> Patch.writePatch
> + GzipCompression -> Patch.gzWritePatch
Ah, that's more like it. One thing I noticed along the way is that
the compression is one of those things that could benefit from a
slight cleanup in the mutual exclusion handling.
Current implementation is
compression :: [DarcsFlag] -> Compression
compression f | NoCompress `elem` f = NoCompression
| otherwise = GzipCompression
whereas a safer option might be
compression = lastWord choices default
where
choices = [ (NoCompress, NoCompression)
, (Compress, GzipCompression) ]
default = GzipCompression
Or then again, I suppose this would also be handled for free by your
nubOptions work assuming we sweep through Darcs.Arguments and set the
right things to DarcsMutuallyExclusive. Hmm, there is no notion of
defaults in nubOptions. Maybe DarcsMutuallyExclusive constructor
actually needs to track the default too.
> +-- TODO re-add a safety catch for --dry-run? Maybe using a global, like
> dryRun
> +-- :: Bool, with dryRun = unsafePerformIO $ readIORef ...
Note how cmdargs keeps track of verbosity using unsafePerformIO $
readIORef ... too (I think)
> tentativelyAddPatch_ :: RepoPatch p
> hunk ./src/Darcs/Repository/Internal.hs 474
> - => UpdatePristine -> Repository p C(r u t) ->
> [DarcsFlag]
> + => UpdatePristine -> Repository p C(r u t) ->
> Compression
> -> PatchInfoAnd p C(t y) -> IO (Repository p C(r u y))
> hunk ./src/Darcs/Repository/Internal.hs 476
> -tentativelyAddPatch_ _ _ opts _
> - | DryRun `elem` opts = bug "tentativelyAddPatch_ called when --dry-run
> is specified"
Hmm now that's a bit interesting. We gain a lot of safety by being able
to see from the types what options we're actually using, but here we've
lost the opportunity to do a sanity check on a global-ish option.
Maybe this sort of global-ish option needs to be tracked in the
oft-proposed-never-gotten-around-to Darcs monad or something?
One could also argue from a keep-it-simple perspective that it's not the
job of tentativelyAddPatch and friends to do this sort of sanity
checking (that it would be absurd because then why do this particular
sanity check and not a 1000 others one could possible think of). I
don't know! How does people even manage to write software in the first
place?
Remove [DarcsFlag] parameters from apply.
-----------------------------------------
This seems to be the same as before with the following amendments.
> -trackBisect :: (Invert p, ShowPatch p, Apply p) => [DarcsFlag] -> IO
> ExitCode -> RL p C(x y) -> IO ()
> +trackBisect :: (Conflict p, Patchy p) => [DarcsFlag] -> IO ExitCode -> RL p
> C(x y) -> IO ()
> -trackNextBisect :: (Invert p, ShowPatch p, Apply p) => [DarcsFlag] ->
> BisectState -> IO ExitCode -> BisectDir -> PatchTree p C(x y) -> IO ()
> +trackNextBisect :: (Conflict p, Patchy p) => [DarcsFlag] -> BisectState ->
> IO ExitCode -> BisectDir -> PatchTree p C(x y) -> IO ()
> trackNextBisect opts (dnow, dtotal) test dir (Fork l r) = do
>
> -jumpHalfOnRight :: (Invert p, ShowPatch p, Apply p) => [DarcsFlag] ->
> PatchTree p C(x y) -> IO ()
> -jumpHalfOnRight opts l = unapplyRL opts (patchTree2RL l)
> +jumpHalfOnRight :: (Conflict p, Patchy p) => [DarcsFlag] -> PatchTree p C(x
> y) -> IO ()
> +jumpHalfOnRight opts l = unapplyRL ps >> makeScriptsExecutable opts ps
> + where ps = patchTree2RL l
>
> hunk ./src/Darcs/Commands/TrackDown.lhs 174
> -jumpHalfOnLeft :: (Invert p, ShowPatch p, Apply p) => [DarcsFlag] ->
> PatchTree p C(x y) -> IO ()
> -jumpHalfOnLeft opts r = applyRL opts (patchTree2RL r)
> +jumpHalfOnLeft :: (Conflict p, Patchy p) => [DarcsFlag] -> PatchTree p C(x
> y) -> IO ()
> +jumpHalfOnLeft opts r = applyRL p >> makeScriptsExecutable opts p
> + where p = patchTree2RL r
Basically that we're now setting executable in trackdown --bisect.
If I may continue my grumbling a bit [ ;-) ], since you did take the
opportunity to amend the patch, maybe it would have been better to seize
the chance to minimise a bit more by seeing if you could do without the
changes to the constraints (to use Patchy). Anyway, still happy to
apply this patch after reading your clarifications.
Thanks!
--
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
