[darcs-users] [issue1208] wish: darcs trackdown --bisect

[Ganesh Sittampalam]

Hi,

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 
other :-)

> +                   _    -> 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.

Cheers,

Ganesh