darcs

Patch 429 accept issue1977 (and 1 more)

Title accept issue1977 (and 1 more)
Superseder Nosy List gh
Related Issues
Status accepted Assigned To
Milestone

Created on 2010-10-18.15:46:34 by gh, last changed 2011-05-10.17:16:00 by darcswatch. Tracked on DarcsWatch.

Files
File name Status Uploaded Type Edit Remove
accept-issue1977.dpatch gh, 2010-10-18.15:46:33 text/x-darcs-patch
unnamed gh, 2010-10-18.15:46:33
See mailing list archives for discussion on individual patches.
Messages
msg12766 (view) Author: gh Date: 2010-10-18.15:46:33
This patch fixes the problem of ``darcs repair'' not wanting to repair
repositories from which the directory pristine.hashed was deleted,
see http://bugs.darcs.net/issue1977 .

To do so, I made that the function Darcs.Repository.Pristine.identifyPristine
never fails, that is, if it finds no pristine directory of any kind,
then the repository is assumed to be of the kind NoPristine.

Previously, to have a NoPristine repository, you had to have a file named
_darcs/pristine.none . (By the way it is not possible since version 2.2 to
create such a repository.)

The good thing is that repositories with that _darcs/pristine.none
file keep on being recognized as NoPristine repositories, so from the
retro-compatibility point of view, I think this change is OK.

This patch is also future-proof with regards to old-fashioned repositories
support removal.

2 patches for repository http://darcs.net:

Sun Oct 17 17:32:30 CEST 2010  Guillaume Hoffmann <guillaumh@gmail.com>
  * accept issue1977

Sun Oct 17 17:32:35 CEST 2010  Guillaume Hoffmann <guillaumh@gmail.com>
  * resolve issue1977 by considering more repositories as being NoPristine
Attachments
msg12954 (view) Author: galbolle Date: 2010-11-09.13:30:52
> The good thing is that repositories with that _darcs/pristine.none
> file keep on being recognized as NoPristine repositories, so from the
> retro-compatibility point of view, I think this change is OK.

Can you add a test that we keep this file when it exists?

Applying in the mean time, thanks.

Florent

accept issue1977
----------------
Guillaume Hoffmann <guillaumh@gmail.com>**20101017153230

addfile ./tests/failing-issue1977-darcs-repair.sh

hunk ./tests/failing-issue1977-darcs-repair.sh 1
> +#!/usr/bin/env bash
> +
> +# Issue1977 darcs repair complains when there is no pristine.hashed 
directory
> +set -ev
> +mkdir temp1
> +cd temp1
> +darcs init
> +echo "a" > a
> +darcs add a
> +darcs rec -am a
> +rm -rf _darcs/pristine.hashed/
> +darcs repair

ok, test the feature we're going to implement

resolve issue1977 by considering more repositories as being NoPristine
----------------------------------------------------------------------
Guillaume Hoffmann <guillaumh@gmail.com>**20101017153235

move ./tests/failing-issue1977-darcs-repair.sh ./tests/issue1977-darcs-
repair.sh

Yes

hunk ./src/Darcs/Repository.hs 511
>                   renameDirectory n nold
>                   renameDirectory ntmp n
>                   return ()
> -          replace (NoPristine _) = return ()
> +          replace NoPristine = return ()
>            nold = darcsdir </> "pristine-old"
>            ntmp = darcsdir </> "pristine-tmp"
>  

hunk ./src/Darcs/Repository/InternalTypes.hs 32
>  import Darcs.Patch ( RepoPatch )
>  
>  data Pristine
> -  = NoPristine !String
> +  = NoPristine
>    | PlainPristine !String
>    | HashedPristine

What would the String parameter represent before?

hunk ./src/Darcs/Repository/InternalTypes.hs 35
> -    deriving ( Show )
> +    deriving ( Show, Eq )
>  
>  data Repository (p :: * C(-> * -> *)) C(recordedstate unrecordedstate 
tentativestate) = Repo !String ![DarcsFlag] !RepoFormat !(RepoType p) 
deriving ( Show )
>  

hunk ./src/Darcs/Repository/Pristine.hs 27
>                   createPristine, removePristine, identifyPristine,
>                   applyPristine, createPristineFromWorking,
>                   getPristinePop,
> -                 pristineDirectory, pristineToFlagString,
> +                 pristineDirectory,
>                   easyCreatePristineDirectoryTree,
>                   easyCreatePartialsPristineDirectoryTree
>                 ) where

hunk ./src/Darcs/Repository/Pristine.hs 32
[imports…]

hunk ./src/Darcs/Repository/Pristine.hs 53
>  #include "impossible.h"
>  
>  nopristine :: Pristine
> -nopristine = NoPristine "aack?"
> +nopristine = NoPristine
>  
Hygiene…

hunk ./src/Darcs/Repository/Pristine.hs 58
>  
> -identifyPristine :: IO (Pristine)
> -identifyPristine = do mp <- reallyIdentifyPristine
> -                      case mp of
> -                          Nothing -> fail "Pristine tree doesn't 
exist."
> -                          Just pristine -> return pristine
> -
> -reallyIdentifyPristine :: IO (Maybe Pristine)
> -reallyIdentifyPristine =
> -    do dir <- findpristine doesDirectoryExist ""
> -       none <- findpristine doesFileExist ".none"
> +identifyPristine :: IO Pristine
> +identifyPristine =
> +    do dir <- findpristine doesDirectoryExist
>         hashinv <- doesFileExist $ darcsdir++"/hashed_inventory"
>         hashpris <- doesDirectoryExist hashedPristineDirectory

Can I kindly request some haddocks for this module?

hunk ./src/Darcs/Repository/Pristine.hs 63
> -       case (dir, none, hashinv && hashpris) of
> -           (Nothing, Nothing, False) -> return Nothing
> -           (Just n, Nothing, False) ->
> -               return (Just (PlainPristine n))
> -           (Nothing, Just n, False) ->
> -               return (Just (NoPristine n))
> -           (Nothing, Nothing, True) ->
> -               return (Just HashedPristine)
> +       case (dir, hashinv && hashpris) of
> +           (Nothing, False) -> return nopristine
> +           (Just n, False) ->  return (PlainPristine n)
> +           (Nothing, True) ->  return HashedPristine
>             _ -> fail "Multiple pristine trees."

hunk ./src/Darcs/Repository/Pristine.hs 68
> -    where findpristine fn ext =
> +    where findpristine fn =
>                do e1 <- fn n1
>                   e2 <- fn n2
>                   case (e1, e2) of

hunk ./src/Darcs/Repository/Pristine.hs 76
>                       (True, False) -> return (Just n1)
>                       (False, True) -> return (Just n2)
>                       (True, True) -> fail "Multiple pristine trees."
> -              where  n1 = darcsdir++"/pristine" ++ ext
> -                     n2 = darcsdir++"/current" ++ ext
> +              where  n1 = darcsdir++"/pristine"
> +                     n2 = darcsdir++"/current"
>  

ok, return NoPristine if we don't find it.

hunk ./src/Darcs/Repository/Pristine.hs 81
>  flagsToPristine :: [DarcsFlag] -> RepoFormat -> Pristine
>  flagsToPristine _ rf | formatHas HashedInventory rf = HashedPristine
> -flagsToPristine (PristineNone : _) _ = NoPristine (darcsdir++"/" ++ 
pristineName ++ ".none")
> +flagsToPristine (PristineNone : _) _ = NoPristine
>  flagsToPristine (PristinePlain : _) _ = PlainPristine (darcsdir++"/" 
++ pristineName)
>  flagsToPristine (_ : t) rf = flagsToPristine t rf
>  flagsToPristine [] rf = flagsToPristine [PristinePlain] rf

Removal of PrisineNone's parameter

hunk ./src/Darcs/Repository/Pristine.hs 88
>  
>  createPristine :: Pristine -> IO Pristine
>  createPristine p =
> -    do oldpristine <- reallyIdentifyPristine
> -       when (isJust oldpristine) $ fail "Pristine tree already 
exists."
> +    do oldpristine <- identifyPristine
> +       unless (oldpristine == nopristine) $ fail "Pristine tree 
already exists."
>         case p of

Ok, any reason not to use the NoPristine constructor? Now that it has
no argument, there is no reason to keep the nopristine constant around.

hunk ./src/Darcs/Repository/Pristine.hs 91
> -           NoPristine n -> writeBinFile n "Do not delete this 
file.\n"
> +           NoPristine -> return ()
>             PlainPristine n -> createDirectory n
>             HashedPristine -> do createDirectory 
hashedPristineDirectory

So creating a pristine-less no longer creates a pristine.none
file. Not a big deal, given that this is going to be dead code
soon.

hunk ./src/Darcs/Repository/Pristine.hs 94
> -                                writeDarcsHashed emptyTree 
"_darcs/pristine.hashed"
> +                                writeDarcsHashed emptyTree 
hashedPristineDirectory
>                                  return ()
>         return p
>  
ok

hunk ./src/Darcs/Repository/Pristine.hs 102
>  hashedPristineDirectory = darcsdir++"/pristine.hashed"
>  
>  removePristine :: Pristine -> IO ()
> -removePristine (NoPristine n) = removeFile n
> +removePristine NoPristine = return ()
>  removePristine (PlainPristine n) = rmRecursive n
>  removePristine HashedPristine = rmRecursive hashedPristineDirectory
>  
ok

hunk ./src/Darcs/Repository/Pristine.hs 107
>  applyPristine :: Patchy p => Pristine -> p C(x y) -> IO ()
> -applyPristine (NoPristine _) _ = return ()
> +applyPristine NoPristine _ = return ()
>  -- We don't need flags for now, since we don't care about
>  -- SetScriptsExecutable for the pristine cache.
>  applyPristine (PlainPristine n) p =
ok

hunk ./src/Darcs/Repository/Pristine.hs 116
>      bug "3 HashedPristine is not implemented yet."
>  
>  createPristineFromWorking :: Pristine -> IO ()
> -createPristineFromWorking (NoPristine _) = return ()
> +createPristineFromWorking NoPristine = return ()
>  createPristineFromWorking (PlainPristine n) = cloneTreeExcept 
[darcsdir] "." n
>  createPristineFromWorking HashedPristine =
>      bug "HashedPristine is not implemented yet."

ok (isn't the  bug "HashedPristine is not implemented yet." a lie?)

hunk ./src/Darcs/Repository/Pristine.hs 130
>  pristineDirectory (PlainPristine n) = Just n
>  pristineDirectory _ = Nothing
>  
> -pristineToFlagString :: Pristine -> String
> -pristineToFlagString (NoPristine _) = "--no-pristine-tree"
> -pristineToFlagString (PlainPristine _) = "--plain-pristine-tree"
> -pristineToFlagString HashedPristine =
> -    bug "HashedPristine is not implemented yet."
> -
>  easyCreatePristineDirectoryTree :: Pristine -> FilePath -> IO Bool

hunk ./src/Darcs/Repository/Pristine.hs 131
> -easyCreatePristineDirectoryTree (NoPristine _) _ = return False
> +easyCreatePristineDirectoryTree NoPristine _ = return False
>  easyCreatePristineDirectoryTree (PlainPristine n) p
>   = cloneTree n p >> return True
>  easyCreatePristineDirectoryTree HashedPristine _ =

hunk ./src/Darcs/Repository/Pristine.hs 139
>  
>  easyCreatePartialsPristineDirectoryTree :: FilePathLike fp => [fp] -> 
Pristine
>                                          -> FilePath -> IO Bool
> -easyCreatePartialsPristineDirectoryTree _ (NoPristine _) _ = return 
False
> +easyCreatePartialsPristineDirectoryTree _ NoPristine _ = return False
>  easyCreatePartialsPristineDirectoryTree prefs (PlainPristine n) p
>   = clonePartialsTree n p (map toFilePath prefs) >> return True
>  easyCreatePartialsPristineDirectoryTree _ HashedPristine _ =

A haddock for easy… would be welcome.
msg12962 (view) Author: darcswatch Date: 2010-11-09.16:44:42
This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-13310bdbcdfeb16549e9ff2a0fa2f1e345f28850
msg12981 (view) Author: gh Date: 2010-11-10.09:54:56
2010/11/9 Florent Becker <bugs@darcs.net>:
>
> Florent Becker <florent.becker@ens-lyon.org> added the comment:
>
>> The good thing is that repositories with that _darcs/pristine.none
>> file keep on being recognized as NoPristine repositories, so from the
>> retro-compatibility point of view, I think this change is OK.
>
> Can you add a test that we keep this file when it exists?

I have tried writing such a test but when you think about the possible
cases you get:

* either test that "darcs repair" leaves pristine.none in a hashed
repository, which is a useless test since hashed repos always have a
hashed prsitine.
* either test that "darcs repair" leaves pristine.none in an OF
repository. I tried to do that but I realized that "darcs repair"
never works in an OF repository without the _darcs/pristine/
directory, before or after this bundle. So "darcs repair" just fails
in that case.

In short, the fix for issue1977 in this bundle does not fix the case
of OF repositories, but only for hashed repositories. OTOH it does not
degrade the behaviour of darcs for the OF case.

Since by looking at the code it is clear that no pristine.none or
current.none file is removed, I suggest we can do without such a test,
what do you think?


>>  data Pristine
>> -  = NoPristine !String
>> +  = NoPristine
>>    | PlainPristine !String
>>    | HashedPristine
>
> What would the String parameter represent before?

It was either "pristine.none" or "current.none", according to the
function identifyPristine before this bundle.
It was also "aack?" when the function
Darcs.Repository.Internal.maybeIdentifyRepository needed to return a
dummy object of the type IdentifyRepo when successfully identifying a
remote repository.

> Ok, any reason not to use the NoPristine constructor? Now that it has
> no argument, there is no reason to keep the nopristine constant around.

Indeed, I'll send a patch removing this function.


> hunk ./src/Darcs/Repository/Pristine.hs 116
>>      bug "3 HashedPristine is not implemented yet."
>>
>>  createPristineFromWorking :: Pristine -> IO ()
>> -createPristineFromWorking (NoPristine _) = return ()
>> +createPristineFromWorking NoPristine = return ()
>>  createPristineFromWorking (PlainPristine n) = cloneTreeExcept
> [darcsdir] "." n
>>  createPristineFromWorking HashedPristine =
>>      bug "HashedPristine is not implemented yet."
>
> ok (isn't the  bug "HashedPristine is not implemented yet." a lie?)
>

Yes the bug strings are wrong: these cases can never be reacher in the
current state of the code.
Writing a patch for that. What a plate of spaghetti.


>>  easyCreatePartialsPristineDirectoryTree :: FilePathLike fp => [fp] ->
> Pristine
>>                                          -> FilePath -> IO Bool
>> -easyCreatePartialsPristineDirectoryTree _ (NoPristine _) _ = return
> False
>> +easyCreatePartialsPristineDirectoryTree _ NoPristine _ = return False
>>  easyCreatePartialsPristineDirectoryTree prefs (PlainPristine n) p
>>   = clonePartialsTree n p (map toFilePath prefs) >> return True
>>  easyCreatePartialsPristineDirectoryTree _ HashedPristine _ =
>
> A haddock for easy… would be welcome.

OK I'll try.

Guillaume
msg14038 (view) Author: darcswatch Date: 2011-05-10.17:16:00
This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-13310bdbcdfeb16549e9ff2a0fa2f1e345f28850
History
Date User Action Args
2010-10-18 15:46:34ghcreate
2010-10-18 15:47:25darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-13310bdbcdfeb16549e9ff2a0fa2f1e345f28850
2010-11-09 13:30:53galbollesetstatus: needs-screening -> accepted-pending-tests
messages: + msg12954
2010-11-09 16:44:42darcswatchsetstatus: accepted-pending-tests -> accepted
messages: + msg12962
2010-11-10 09:54:56ghsetmessages: + msg12981
2011-05-10 17:16:00darcswatchsetmessages: + msg14038