[darcs-users] [issue1208] wish: darcs trackdown --bisect
ganesh at earth.li
Sat Dec 5 21:01:55 UTC 2009
A slightly more comprehensive commentary on this patch.
On Thu, 3 Dec 2009, Matthias Fischmann wrote:
> hunk ./src/Darcs/Commands/TrackDown.lhs 87
> - patches <- read_repo repository
> + patches <- read_repo repository >>= cropToMatchRangeRL opts . mapRL_RL hopefully . concatRL
This is really confusing, because the order things happen is read_repo,
concatRL, map hopefully, cropToMatch. Please stick to one direction or the
> + _ -> assert False $ error "'--last' must not occur more than once."
> + -- FIXME: is this appropriate error handling?
I think just calling 'error' is appropriate here. What were you trying to
achieve with assert, line number info? I don't think that's particularly
useful here and could be rather confusing to end-users if they got a
binary without optimisations.
> hunk ./src/Darcs/Ordered.hs 145
> +takeRL :: Int -> RL p C(x y) -> RL p C(x y)
> +takeRL i _ | i < 0 = assert False $ error "takeRL: negative argument."
> +takeRL 0 _ = NilRL
> +takeRL _ NilRL = NilRL
> +takeRL i (x:<:xs) = x :<: takeRL (i-1) xs
> +takeFL :: Int -> FL p C(x y) -> FL p C(x y)
> +takeFL i _ | i < 0 = assert False $ error "takeFL: negative argument."
> +takeFL 0 _ = NilFL
> +takeFL _ NilFL = NilFL
> +takeFL i (x:>:xs) = x :>: takeFL (i-1) xs
As previously mentioned, this won't work with witnesses as it stands. You
should be able to use FlippedSeal and Sealed for the return types of
takeRL and takeFL respectively.
The rest of your patch looked fine at first glance.
More information about the darcs-users