Note: When I try to build darcs with hashed-storage 0.3.3.2 and these patches, I get an "Not in scope: unfold" for Darcs.Gorsvet...
On Wed, Jun 17, 2009 at 20:48:46 +0200, Petr Rockai wrote: > >From the darcs-hs branch. I will release hashed-storage 0.3.3 sometime > >probably > tomorrow, at which point, we need to bump the dependency to that, due to > incompatible API changes. Please do not apply till then (but review is > welcome). Hmm, so I didn't have any success recruiting new reviewers for the SoC stuff (hopefully Christian, when he's done with the gzip stuff), so here we go! Note: this only took about 45 minutes, including time to patch Darcs's haddocks. Patches may look very big at a glance, but most of the time, it's minor things like the first few things I commented on, or just patch context, which I mostly ignore because I use a graphical diff. So if you think you *might* be a reviewer, it really doesn't take that long (as long as we spread it around a bit) and it could be a nice way to relax from time to time :-) --------------------------------------------------------- TreeIO is smart enough now to unfold as needed. Bump the hashed-storage dependency to >= 0.3.2. --------------------------------------------------------- no comments Explicit import list for Storage.Hashed.Monad in Gorsvet. --------------------------------------------------------- Note that you have a trailing comma on the writeFile above. Have you fixed this already in your repo? Also curse haskell_policy in Czech. ----------------------------------- > import Storage.Hashed.AnchoredPath > import Storage.Hashed.Monad > ( virtualTreeIO, hashedTreeIO, plainTreeIO > - , unlink, rename, createDirectory, writeFile, readFile, cwd, tree > - , TreeState ) > + , unlink, rename, createDirectory, writeFile, > + , readFile -- ratify readFile: haskell_policy je natvrdl?? Cute, but it may be worthwhile to explain that this is because this is not System.IO.readFile that we're importing (and presumably, that your Storage.Hashed.Monad.readFile avoids this problem?) We need to unfold the pristine Tree before rebuilding the index. ---------------------------------------------------------------- > Petr Rockai <[email protected]>**20090603203631 > Ignore-this: a7501a0d75c5596d91bc7ee96f5895f9 > ] hunk ./src/Darcs/Gorsvet.hs 251 > - pristine <- readRecordedAndPending repo > + pristine <- readRecordedAndPending repo >>= unfold Or else the consequence would be that the index only contains top-level entries? Fix index invalidation in the move command. ------------------------------------------- > hunk ./src/Darcs/Commands/Move.lhs 101 > cur <- slurp_pending repository > addpatch <- check_new_and_old_filenames opts cur work (old_fp,new_fp) > + invalidateIndex repository > withSignalsBlocked $ do > case addpatch of > Nothing -> add_to_pending repository (Darcs.Patch.move old_fp new_fp > :>: NilFL) > hunk ./src/Darcs/Commands/Move.lhs 124 > cur <- slurp_pending repository > work <- slurp "." > addpatches <- mapM (check_new_and_old_filenames opts cur work) $ zip > moved movetargets > + invalidateIndex repository > withSignalsBlocked $ do > add_to_pending repository $ unsafeFL $ catMaybes addpatches ++ > movepatches > zipWithM_ (move_file_or_dir work) moved movetargets If I understand correctly, the fix is to avoid invaliding the index unless we actually move something, that is to only do this after we pass the checking for new and old filenames. Since the checking happens in two places, we have to do invalidateIndex in both. The hashed-storage index functions take a filename now. ------------------------------------------------------- > - (False, False) -> (relevant `fmap` readIndex) >>= unfold > - (False, True) -> do guide <- unfold current > - all <- readPlainTree "." > - return $ relevant $ (restrict guide) all > - (True, _) -> do all <- readPlainTree "." > - return $ relevant $ nonboring all > + (False, False) -> do > + all <- readIndex "_darcs/index" > + unfold (relevant all) > + (False, True) -> do > + guide <- unfold current > + all <- readPlainTree "." > + return $ relevant $ (restrict guide) all > + (True, _) -> do > + all <- readPlainTree "." > + return $ relevant $ nonboring all These are mostly whitespace and other tweaks. The main change is supplying an argument to readIndex ("_darcs/index"), which now takes a filename. By the way, if hashed-storage starts to be more stable and to be used by tools other than darcs, it would be a good idea to follow the Package versioning policy in http://www.haskell.org/haskellwiki/Package_versioning_policy ...doesn't it want you to bump to 0.4 in this case, since the new argument is a non-backward compatible change? Use local pendingChanges instead of read_pending, in readRecordedAndPending. ---------------------------------------------------------------------------- > - Sealed pend <- read_pending repo > - (_, t) <- virtualTreeIO (apply [] pend) pristine > + Sealed pending <- pendingChanges repo [] > + (_, t) <- virtualTreeIO (apply [] pending) pristine I'll just trust prior review on pendingChanges here... Optimise the file existence checking in whatsnew <files>. --------------------------------------------------------- This replaces our code for checking if the user passed in an filename to darcs whatsnew for a file that either isn't there or (more usefully) isn't being tracked by darcs. Here's the old code for reference... > -warn_if_bogus :: (Slurpy,Slurpy) -> [SubPath] -> IO() > -warn_if_bogus _ [] = return () > -warn_if_bogus (rec, pend) (f:fs) = > - do exist1 <- doesFileExist file > - exist2 <- doesDirectoryExist file > - let exist = exist1 || exist2 > - if exist then when (not (slurp_has fp rec || slurp_has fp pend)) $ > - putStrLn $ "WARNING: File '" > - ++file++"' not in repository!" > - else putStrLn $ "WARNING: File '"++file++"' does not exist!" > - warn_if_bogus (rec, pend) fs > - where fp = toFilePath f > - file = encode_white fp And the new code. > announce_files :: (RepoPatch p) => Repository p C(r u t) -> [SubPath] -> IO > () > announce_files repo files = > when (areFileArgs files) $ do > - slurps <- slurp_recorded_and_unrecorded repo > - warn_if_bogus slurps files > + nonboring <- restrictBoring > + working <- nonboring `fmap` readPlainTree "." > + pristine <- readRecordedAndPending repo > + let paths = map (fn2fp . sp2fn) files > + check = virtualTreeIO (mapM exists $ map floatPath paths) > + (in_working, _) <- check working > + (in_pending, _) <- check pristine > + mapM_ maybe_warn $ zip3 paths in_working in_pending Looks good to me at a glance. I was initially confused by the zip3 but now I understand we're just cruising the list of booleans (no size mis-matches here). How is this an optimisation? Is just by virtue of your hashed-storage stuff being better than slurpies? Also, we've stopped calling encode_white, but that's probably a good thing because (a) encode_white translates whitespace to something presumably darcs-specific (e.g. \32 for ' ') whereas I'll bet we originally meant to just use for back-slashing and (b) we already wrap the filename in quotes anyway. I'll disregard filenames with apostrophes in them here :-). [I'll be sending my haddock patches for encode_white in a bit... good place to apply the haddock-as-you-go principle for reading darcs code]. > putStrLn $ "What's new in "++unwords (map show files)++":\n" > hunk ./src/Darcs/Commands/WhatsNew.lhs 108 > + where maybe_warn (file, False, False) = > + putStrLn $ "WARNING: File '"++file++"' does not exist!" > + maybe_warn (file, True, False) = > + putStrLn $ "WARNING: File '" ++ file ++ "' not in repository!" > + maybe_warn _ = return () Should we be giving a warning if the user asks what's new on a boring file? Minor style comment: zipWith3 could be helpful here. Also, an approach like the below may be more explicit and easier to follow, albeit more verbose. maybe_warn (file, exists, tracked) = if exists then if tracked then ... Basic "show index" implementation. ---------------------------------- > + "The `darcs show index' command lists all version-controlled files and > " ++ > + "directories along with their hashes as stored in _darcs/index. " ++ > + "For files, the fields correspond to file size, sha256 of the current > " ++ > + "file content and the filename.", No timestamp? Not useful? > +show_index_cmd :: [DarcsFlag] -> [String] -> IO () > +show_index_cmd opts _ = withRepository opts $- \repo -> do UI nit: it may be useful to demand non-empty args here (or is this somehow already enforced by other parts of darcs?). > + checkIndex repo > + x <- unfold =<< readIndex "_darcs/index" > + let line | NullFlag `elem` opts = \t -> putStr t >> putChar '\0' > + | otherwise = putStrLn > + output (p, i) = do > + let hash = case itemHash i of > + Just h -> BS.unpack $ darcsFormatHash h > + Nothing -> "(no hash available)" > + path = anchorPath "" p > + isdir = case i of > + SubTree _ -> "/" > + _ -> "" > + line $ hash ++ " " ++ path ++ isdir > + mapM_ output $ list x Looks fine to me. I tend to prefer more separation between my IO stuff and my non-IO stuff, but it doesn't really matter. I don't really understand what purpose NullFlag (-0, separate file names by NUL characters) serves, but darcs show files implements it too. -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
