Fix several bugs caused by a bad Ord instance for Remote.

A long time ago I made Remote be an instance of the Ord typeclass, with an
implementation that compared the costs of Remotes. That seemed like a good
idea at the time, as it saved typing.. But at the time I was still making
custom Read and Show instances too. I've since learned that this is *not* a
good idea, and neither is making custom Ord instances, without deep thought
about the possible sets of values in a type. Haskell typeclasses are not a
toy.

This Ord instance came around and bit me when I put Remotes into a Set,
because now remotes with the same cost appeared to be in the Set even if
they were not. Also affected putting Remotes into a Map.

Rarely does a bug go this deep. I've fixed it comprehensively, first
removing the Ord instance entirely, and fixing the places that wanted to
order remotes by cost to do it explicitly. Then adding back an Ord instance
that is much more sane. Also by checking the rest of the Ord instances in
the code base (which were all ok).

While doing that, I found lots of places that kept remotes in Maps and
Sets. All of it was probably subtly broken in one way or another before
this fix, but it would be hard to say exactly how the bugs would
manifest.
This commit is contained in:
Joey Hess 2013-03-16 17:43:42 -04:00
parent b3d3ece2ab
commit 17c9ff576d
3 changed files with 5 additions and 4 deletions

View file

@ -45,6 +45,7 @@ import qualified Data.Map as M
import Text.JSON import Text.JSON
import Text.JSON.Generic import Text.JSON.Generic
import Data.Tuple import Data.Tuple
import Data.Ord
import Common.Annex import Common.Annex
import Types.Remote import Types.Remote
@ -208,7 +209,7 @@ keyPossibilities' key trusted = do
allremotes <- enabledRemoteList allremotes <- enabledRemoteList
let validremotes = remotesWithUUID allremotes validuuids let validremotes = remotesWithUUID allremotes validuuids
return (sort validremotes, validtrusteduuids) return (sortBy (comparing cost) validremotes, validtrusteduuids)
{- Displays known locations of a key. -} {- Displays known locations of a key. -}
showLocations :: Key -> [UUID] -> String -> Annex () showLocations :: Key -> [UUID] -> String -> Annex ()
@ -249,7 +250,7 @@ logStatus remote key = logChange key (uuid remote)
{- Orders remotes by cost, with ones with the lowest cost grouped together. -} {- Orders remotes by cost, with ones with the lowest cost grouped together. -}
byCost :: [Remote] -> [[Remote]] byCost :: [Remote] -> [[Remote]]
byCost = map snd . sort . M.toList . costmap byCost = map snd . sortBy (comparing fst) . M.toList . costmap
where where
costmap = M.fromListWith (++) . map costpair costmap = M.fromListWith (++) . map costpair
costpair r = (cost r, [r]) costpair r = (cost r, [r])

View file

@ -87,6 +87,5 @@ instance Show (RemoteA a) where
instance Eq (RemoteA a) where instance Eq (RemoteA a) where
x == y = uuid x == uuid y x == y = uuid x == uuid y
-- order remotes by cost
instance Ord (RemoteA a) where instance Ord (RemoteA a) where
compare = comparing cost compare = comparing uuid

1
debian/changelog vendored
View file

@ -15,6 +15,7 @@ git-annex (4.20130315) UNRELEASED; urgency=low
with the problems Google Talk has with not always sending presence with the problems Google Talk has with not always sending presence
messages to clients. messages to clients.
* map: Combine duplicate repositories, for a nicer looking map. * map: Combine duplicate repositories, for a nicer looking map.
* Fix several bugs caused by a bad Ord instance for Remote.
-- Joey Hess <joeyh@debian.org> Fri, 15 Mar 2013 00:10:07 -0400 -- Joey Hess <joeyh@debian.org> Fri, 15 Mar 2013 00:10:07 -0400