[darcs-users] darcs patch: make rollback smarter about breaking up ... (and 5 more)

[Trent W. Buck]

I have only commented on the patches I understand.

Eric Kow <kowey at darcs.net> writes:

> fix bug in put of darcs-1 format repositories.
> ----------------------------------------------------------------------
> hunk ./src/Darcs/Commands/Put.lhs 96
>                  else if format_has HashedInventory rf &&
>                          not (UseOldFashionedInventory `elem` opts)
>                       then UseHashedInventory:filter (/= UseFormat2) opts
> -                     else filter (/= UseFormat2) opts
> +                     else UseOldFashionedInventory:filter (/= UseFormat2) opts

This one makes sense to me, I guess it was broken by Darcs 2.2.0, which
made darcs-2 the default format.

> use exitWith under record, when no patches are selected (to not run posthook).
> ------------------------------------------------------------------------------

I'm not keen on calling exit(3) all over the place, because it
effectively gives your function/command/whatever multiple exit points
and makes it harder to reason about.

As for whether we want the posthook to be skipped when no changes are
recorded: I don't know.  Can anyone think of a situation where the
posthook *should* be run when record is a noop?  For example, maybe
you want a posthook to log all record *attempts* to syslog.  Does the
test preference get run on a noop record?

Can a clever posthook examine, an environment variable and decide to do
nothing?  For example, instead of

    record posthook 'darcs dist -d ../latest'

you could have

    record posthook 'if test -n "$THINGY"; then darcs dist -d ../latest; fi'

If you can opt-out of post-hooks on noop records, but after this change
you can't opt-in to running the post-hook on a noop record, then that is
a bad thing (but possibly only negligibly bad).

> clean up Depends a wee tad
> --------------------------
> hunk ./src/Darcs/Patch/Depends.hs 34
>                 ) where
>  import Data.List ( delete, intersect )
>  import Control.Monad ( liftM2 )
> -import Control.Monad.Error (Error(..), MonadError(..))
> +import Control.Monad.Error ( Error(..) )
>  
>  import Darcs.Patch ( RepoPatch, Named, getdeps, commutex,
>                       commuteFL,
> hunk ./src/Darcs/Patch/Depends.hs 301
>                           $$ human_friendly (info $ n2pia hpc)
>        where ep = case hopefullyM hp of
>                   Right p' -> return p'
> -                 Left e -> throwError (MissingPatch (info hp) e)
> +                 Left e -> Left (MissingPatch (info hp) e)

Does this make the code more obscure?  I don't know how strong the
association of Left with "this is an error" is within the Haskell
community, but to me the change looks to make things harder to
understand merely to avoid importing one extra thingy.