[darcs-users] [patch283] Accept issue1227: darcs repository forma... (and 2 more)

Reinier Lamers bugs at darcs.net
Sun Jun 27 13:46:33 UTC 2010


Reinier Lamers <tux_rocker at 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 <kowey at 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 <kowey at 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 <kowey at 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 <kowey at 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 <bugs at darcs.net>
<http://bugs.darcs.net/patch283>
__________________________________


More information about the darcs-users mailing list