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

Reinier Lamers bugs at darcs.net
Sun Jun 20 18:25:27 UTC 2010


Reinier Lamers <tux_rocker at reinier.de> added the comment:

Hi Eric,

Before pushing this, I'd like to know:

 * if the change of protocol of seekRepo is harmless
 * if we should also change amInRepository

> 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
> +. 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
> + grep "Can't understand repository format" log
> + cd ..
> +cd ..

Here, we check that if the _darcs/format file of R2 is corrupted, we don't
accidentally fall back to the repository in its enclosing directory R.

> [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 )

In this patch we're going to distinguish nonexistent from unreadable repos
by checking the start of the error message. Hence import "isPrefixOf".

> 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
>

So here we distinguish a new case, the one where our error is one about the
repository format. This is the fix for the issue.

But doesn't calling code expect to get either Right () or the given onFail
from this function? At least that was the behavior of the old function. In
findRepository, I see a call "seekRepo (Right ())". Doesn't that expect
never to get a Left? But it looks like findRepository is usually used as a
commandPrereq, in which case a Left will be handled.

The patch below removes he match on the error string and introduces a more
principled approach ung a type with three constructors (instead of
piggybacking on the two-constructor Either type).

> [Generalise mechanism for distinguishing between bad and non repos.
> Eric Kow <kowey at darcs.net>**20100618185428
> 
>  Ignore-this: f637d68510645249ce221ad91df076ae
> 
> ] hunk ./src/Darcs/Repository.hs 56
> -     maybeIdentifyRepository, identifyRepositoryFor,
> +     maybeIdentifyRepository, identifyRepositoryFor, IdentifyRepo(..),

So here we have that three-constructor IdentifyRepo type.
	
> 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

So this is in the writePatchSet function, which writes a PatchSet to a
repository, discarding what's already in the repo. Could I ask you to remove
the quotes around "bad"? ;-)

> hunk ./src/Darcs/Repository.hs 354
> 
>          withCurrentDirectory dir $ readWorking >>= replacePristine repo
>  
>  pristineFromWorking (Repo dir _ _ (DarcsRepository p _)) =
>  
>    withCurrentDirectory dir $ createPristineFromWorking p
> 
> +

Spurious newline added? Looking at it with hexedit, the file does not
currently have a newline at the end. I'm willing to let this pass.

> 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,
>

Add the 3-constructor IdentifyRepo to the export list of
Darcs.Repository.Internal

> 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
>

We don't use isPrefixOf anymore now that the error message matching is gone.
 
> 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 "."
> 

This adds the 3-constructor IdentifyRepo type and changes
maybeIdentifyRepository to use it.

> 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
> 

Now we're inside the maybeIdentifyRepository function, in a pattern match
alternative that special-cases it for trying to identify the current
directory ("."). This hunk handles the case that identifyRepoFormat returns
Left, in which case there is no _darcs/format or _darcs/inventory file in
the directory. It's safe to guess it's not a repo then.

> 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
> 

This handles the case that readProblem returns an error. readProblem
examines the repository format, so at least we got a format when we get
here. So we return BadRepository, not NonRepository.

> 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")

And finally we handle the case that the _darcs directory does not exist: in
that case it's not a repo. But what sense does it make to check if _darcs
exist if we already found an inventory or a format file, which is what
identifyRepoFormat seems to check for?

> 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 =
> 

Here we do the same thing for the other pattern match alternative, which
does identification of repositories at all URLs that are not ".".
Interestingly, the check if _darcs exist is not present here.

> 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 =
> 

Change some code to use the new interface of maybeIdentifyRepository.

> 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
> 

Remove an isRight function that is apparently no longer used.

> hunk ./src/Darcs/Repository/Internal.hs 239
> -currentDirIsRepository = isRight `liftM` maybeIdentifyRepository [] "."
> +currentDirIsRepository = isGoodRepository `fmap` maybeIdentifyRepository
> [] "." + where
> +  isGoodRepository (GoodRepository _) = True
> +  isGoodRepository _ = False
> 
>  amInRepository :: [DarcsFlag] -> IO (Either String ())
>  amInRepository (WorkRepoDir d:_) =
> 

Switch currentDirIsRepository to the new interface for
maybeIdentifyRepository.

> hunk ./src/Darcs/Repository/Internal.hs 266
> 
>     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

And finally, change seekRepo to use this new interface.

Shouldn't we also change the amInRepository function above seekRepo? It will
still say "You need to be a repository directory to run this command." even
when it should already know that it *is* in a repo but just doesn't
understand the format. At least if it gets an argument of the "WorkRepoDir"
constructor. But I don't even know what that is, so I'll shut up.

--
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