Reinier Lamers <tux_roc...@reinier.de> added the comment:
Applied, thanks!
I was already halfway reviewing this patch, and then I tried to suspend my
laptop for the first time on Ubuntu Lucid and things froze and it was lost.
Hence the delay.
> New patches:
>
> [Accept issue1277: darcs repository format errors not reported in add.
> Eric Kow <ko...@darcs.net>**20100618190106
>
> Ignore-this: d0961642c7526643b98a5ed066434288
>
> ] addfile ./tests/failing-issue1277-repo-format.sh
> hunk ./tests/failing-issue1277-repo-format.sh 1
> +#!/usr/bin/env bash
> +## Test for issue1277 - repository format errors should be reported
> +## correctly (ie. not as some totally unrelated error)
> +##
> +## Copyright (C) 2010 Eric Kow
> +##
> +## Permission is hereby granted, free of charge, to any person
> +## obtaining a copy of this software and associated documentation
> +## files (the "Software"), to deal in the Software without
> +## restriction, including without limitation the rights to use, copy,
> +## modify, merge, publish, distribute, sublicense, and/or sell copies
> +## of the Software, and to permit persons to whom the Software is
> +## furnished to do so, subject to the following conditions:
> +##
> +## The above copyright notice and this permission notice shall be
> +## included in all copies or substantial portions of the Software.
> +##
> +## THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> +## EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> +## MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> +## NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> +## BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> +## ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> +## CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> +## SOFTWARE.
> +
> +. lib # Load some portability helpers.
> +rm -rf R # Another script may have left a mess.
> +darcs init --repo R # Create our test repos.
> +cd R
> + darcs init --repo R2 # Protect the darcs darcs repo with R
> + cd R2
> + echo impossible >> _darcs/format
> + echo 'Example content.' > f
> + not darcs add f > log 2>&1
> + grep "Can't understand repository format" log
> + cd ..
> +cd ..
This just adds the redirection, okay.
> [Resolve issue1277: percolate repository format errors correctly.
> Eric Kow <ko...@darcs.net>**20100618185423
>
> Ignore-this: b541efa39c3b55b67479b209f55ffd1d
> The problem is that we do not distinguish between bad repos and
> non-repositories so we keep seeking upwards.
>
> ] move ./tests/failing-issue1277-repo-format.sh
> ./tests/issue1277-repo-format.sh hunk ./src/Darcs/Repository/Internal.hs
> 137
>
> import Darcs.Witnesses.Sealed ( Sealed(Sealed), seal,
> FlippedSeal(FlippedSeal), flipSeal ) import
> Darcs.Repository.InternalTypes( Repository(..), RepoType(..) ) import
> Darcs.Global ( darcsdir )
>
> +
> +import Data.List ( isPrefixOf )
>
> import System.Mem( performGC )
>
> import qualified Storage.Hashed.Tree as Tree
>
> hunk ./src/Darcs/Repository/Internal.hs 259
>
> -> IO (Either String ())
>
> seekRepo onFail = getCurrentDirectory >>= helper where
>
> helper startpwd = do
>
> - air <- currentDirIsRepository
> - if air
> - then return (Right ())
> - else do cd <- toFilePath `fmap` getCurrentDirectory
> + status <- maybeIdentifyRepository [] "."
> + case status of
> + Right _ -> return (Right ())
> + Left e | "Can't understand repository format" `isPrefixOf` e ->
> return (Left e) + Left _ ->
> + do cd <- toFilePath `fmap` getCurrentDirectory
>
> setCurrentDirectory ".."
> cd' <- toFilePath `fmap` getCurrentDirectory
> if cd' /= cd
This hasn't changed since last review, OK.
> [Extend issue1277 test for more prerequisites.
> Eric Kow <ko...@darcs.net>**20100621182016
>
> Ignore-this: 49548f2470cd22133cecca1c5458b1d9
>
> ] hunk ./tests/issue1277-repo-format.sh 37
>
> echo 'Example content.' > f
> not darcs add f > log 2>&1
> grep "Can't understand repository format" log
>
> + not darcs whatsnew > log 2>&1
> + grep "Can't understand repository format" log
> + not darcs init > log 2>&1
> + grep "You may not run this command in a repository" log
This is the last line printed before it fails, without applying the next
test. Apparently the 'whatsnew' case already works without the following
patch.
> [Generalise mechanism for distinguishing between bad and non repos.
> Eric Kow <ko...@darcs.net>**20100621182834
>
> Ignore-this: 85bb5cfee6f8955eedea4cd438603e2d
> We remove the potentially misleading currentDirIsRepository along
> the way.
>
> ] hunk ./src/Darcs/Repository.hs 56
>
> import Darcs.Repository.Internal
>
> (Repository(..), RepoType(..), ($-),
>
> - maybeIdentifyRepository, identifyRepositoryFor,
> + maybeIdentifyRepository, identifyRepositoryFor, IdentifyRepo(..),
>
> findRepository, amInRepository, amNotInRepository,
> makePatchLazy,
> withRecorded,
>
> hunk ./src/Darcs/Repository.hs 259
>
> maybeRepo <- maybeIdentifyRepository opts "."
> let repo@(Repo _ _ rf2 (DarcsRepository _ c)) =
>
> case maybeRepo of
>
> - Right r -> r
> - Left e -> bug ("Current directory not repository in
> writePatchSet: " ++ e)
> + GoodRepository r -> r
> + BadRepository e -> bug ("Current directory is a bad repository
> in writePatchSet: " ++ e)
> + NonRepository e -> bug ("Current directory not a repository in
> writePatchSet: " ++ e)
>
> debugMessage "Writing inventory"
> if formatHas HashedInventory rf2
>
> then do HashedRepo.writeTentativeInventory c (compression opts)
> patchset
>
Thanks for removing the quotes =)
> hunk ./src/Darcs/Repository/Internal.hs 26
>
> module Darcs.Repository.Internal ( Repository(..), RepoType(..),
> RIO(unsafeUnRIO), ($-),
>
> maybeIdentifyRepository, identifyDarcs1Repository,
> identifyRepositoryFor,
>
> + IdentifyRepo(..),
>
> findRepository, amInRepository, amNotInRepository,
> revertRepositoryChanges,
> announceMergeConflicts, setTentativePending,
>
> hunk ./src/Darcs/Repository/Internal.hs 139
>
> import Darcs.Repository.InternalTypes( Repository(..), RepoType(..) )
> import Darcs.Global ( darcsdir )
>
> -import Data.List ( isPrefixOf )
>
> import System.Mem( performGC )
>
> import qualified Storage.Hashed.Tree as Tree
>
> hunk ./src/Darcs/Repository/Internal.hs 193
>
> getRepository :: RIO p C(r u t t) (Repository p C(r u t))
> getRepository = RIO return
>
> -maybeIdentifyRepository :: [DarcsFlag] -> String -> IO (Either String
> (Repository p C(r u t)))
> +-- | The status of a given directory: is it a darcs repository?
> +data IdentifyRepo p C(r u t) = BadRepository String -- ^ looks like a
> repository with some error
> + | NonRepository String -- ^ safest guess
> + | GoodRepository (Repository p C(r u t))
> +
> +maybeIdentifyRepository :: [DarcsFlag] -> String -> IO (IdentifyRepo p C(r u
> t))
>
> maybeIdentifyRepository opts "." =
>
> do darcs <- doesDirectoryExist darcsdir
>
> rf_or_e <- identifyRepoFormat "."
>
> hunk ./src/Darcs/Repository/Internal.hs 204
>
> here <- toPath `fmap` ioAbsoluteOrRemote "."
> case rf_or_e of
>
> - Left err -> return $ Left err
> + Left err -> return $ NonRepository err
>
> Right rf ->
>
> case readProblem rf of
>
> hunk ./src/Darcs/Repository/Internal.hs 207
> - Just err -> return $ Left err
> + Just err -> return $ BadRepository err
>
> Nothing -> if darcs then do pris <- identifyPristine
>
> cs <- getCaches opts here
>
> hunk ./src/Darcs/Repository/Internal.hs 210
> - return $ Right $ Repo here opts rf
> (DarcsRepository pris cs)
> - else return (Left "Not a repository")
> + return $ GoodRepository $ Repo here
> opts rf (DarcsRepository pris cs)
> + else return (NonRepository "Not a
> repository")
>
> maybeIdentifyRepository opts url' =
>
> do url <- toPath `fmap` ioAbsoluteOrRemote url'
>
> rf_or_e <- identifyRepoFormat url
>
> hunk ./src/Darcs/Repository/Internal.hs 216
>
> case rf_or_e of
>
> - Left e -> return $ Left e
> + Left e -> return $ NonRepository e
>
> Right rf -> case readProblem rf of
>
> hunk ./src/Darcs/Repository/Internal.hs 218
> - Just err -> return $ Left err
> + Just err -> return $ BadRepository err
>
> Nothing -> do cs <- getCaches opts url
>
> hunk ./src/Darcs/Repository/Internal.hs 220
> - return $ Right $ Repo url opts rf
> (DarcsRepository nopristine cs)
> + return $ GoodRepository $ Repo url opts rf
> (DarcsRepository nopristine cs)
>
> identifyDarcs1Repository :: [DarcsFlag] -> String -> IO (Repository Patch
> C(r u t)) identifyDarcs1Repository opts url =
>
This is all old news from the previous review. The check of the "_darcs"
directory remains for "." and stays absent for the general case. But we're
not here to fix that now, if necessary.
> hunk ./src/Darcs/Repository/Internal.hs 226
>
> do er <- maybeIdentifyRepository opts url
>
> case er of
>
> - Left s -> fail s
> - Right r -> return r
> + BadRepository s -> fail s
> + NonRepository s -> fail s
> + GoodRepository r -> return r
>
> identifyRepositoryFor :: forall p C(r u t). RepoPatch p => Repository p
> C(r u t) -> String -> IO (Repository p C(r u t)) identifyRepositoryFor
> (Repo _ opts rf _) url =
>
> hunk ./src/Darcs/Repository/Internal.hs 238
>
> Just e -> fail $ "Incompatibility with repository " ++ url ++
> ":\n" ++ e Nothing -> return $ Repo absurl opts rf_ t'
>
> -isRight :: Either a b -> Bool
> -isRight (Right _) = True
> -isRight _ = False
> -
> -currentDirIsRepository :: IO Bool
> -currentDirIsRepository = isRight `liftM` maybeIdentifyRepository [] "."
> -
>
> amInRepository :: [DarcsFlag] -> IO (Either String ())
> amInRepository (WorkRepoDir d:_) =
>
> do setCurrentDirectory d `catchall` (fail $ "can't set directory to
> "++d)
>
Old news up to the point where we delete "currentDirIsRepository". I suppose
it has to go because it returns a Boolean, while we want some tri-state
(good repo, bad repo, non repo) return type now.
> hunk ./src/Darcs/Repository/Internal.hs 241
> - air <- currentDirIsRepository
> - if air
> - then return (Right ())
> - else return (Left "You need to be in a repository directory to run
> this command.")
> + status <- maybeIdentifyRepository [] "."
> + case status of
> + GoodRepository _ -> return (Right ())
> + BadRepository e -> return (Left $ "While " ++ d ++ " looks like a
> repository directory, we have a problem with it:\n" ++ e)
> + NonRepository _ -> return (Left "You need to be in a repository
> directory to run this command.")
>
> amInRepository (_:fs) = amInRepository fs
> amInRepository [] =
>
> seekRepo (Left "You need to be in a repository directory to run this
> command.")
>
This is the new amInRepository that checks whether we're in a usable
repository, and supplies an error message if not.
"But" might be a better conjunction to use than "while". Like 'd ++ "looks
like a repository directory, but we have a problem with it:\n" ++ e'. But
note that I misspelt "than" as "then" initially, so don't trust me too hard
:).
> hunk ./src/Darcs/Repository/Internal.hs 261
>
> helper startpwd = do
>
> status <- maybeIdentifyRepository [] "."
> case status of
>
> - Right _ -> return (Right ())
> - Left e | "Can't understand repository format" `isPrefixOf` e -> return
> (Left e)
> - Left _ ->
> + GoodRepository _ -> return (Right ())
> + BadRepository e -> return (Left e)
> + NonRepository _ ->
>
> do cd <- toFilePath `fmap` getCurrentDirectory
>
> setCurrentDirectory ".."
> cd' <- toFilePath `fmap` getCurrentDirectory
>
This changes seekRepo, this is old news, okay.
> hunk ./src/Darcs/Repository/Internal.hs 283
>
> amNotInRepository []
>
> amNotInRepository (_:f) = amNotInRepository f
> amNotInRepository [] =
>
> - do air <- currentDirIsRepository
> - if air then return (Left $ "You may not run this command in a
> repository.")
> - else return $ Right ()
> + do status <- maybeIdentifyRepository [] "."
> + case status of
> + GoodRepository _ -> return (Left $ "You may not run this command in
> a repository.")
> + BadRepository e -> return (Left $ "You may not run this command in
> a repository.\nBy the way, we have a problem with it:\n" ++ e)
> + NonRepository _ -> return (Right ())
>
> findRepository :: [DarcsFlag] -> IO (Either String ())
> findRepository (WorkRepoUrl d:_) | isFile d =
>
And here we adapt amNotInRepository to the removal of currentDirIsRepository.
amNotInRepository is what's used in init. So this makes the test
succeed again.
Regards,
Reinier
----------
title: Accept issue1277: darcs repository forma... (and 2 more) -> Accept
issue1227: darcs repository forma... (and 2 more)
__________________________________
Darcs bug tracker <b...@darcs.net>
<http://bugs.darcs.net/patch283>
__________________________________
_______________________________________________
darcs-users mailing list
darcs-users@darcs.net
http://lists.osuosl.org/mailman/listinfo/darcs-users
