[darcs-users] darcs patch: test that tentative leftover is cleared (and 2 more)

[Jason Dagit]

Kamil,

You've really tackled an unruly bit of code.  It's forced me to read over
this part and really try to understand it.  I hope my understanding is
correct and hopefully you're also learning a lot from this too.  This
version is better than the last, but I suspect I've found some problems with
it (see below).

Trent, hopefully you'll have time to go over the test suite changes soon.
If not, it looks like there will be another iteration.

Ganesh, do you know this part of darcs at all?  Having another set of eyes
to go over this code and my analysis would be greatly appreciated.

This is the part of the patch I'm focusing on:

[resolve issue1406: amend-record unrecords on a test failure
> Kamil Dworakowski <kamil at dworakowski.name>**20090902221053
> Ignore-this: f137023c7379af697dcda9f41ff6a61f
>
> A failing test on amend no longer unrecords the original patch. The
> failure manifested itself only on old-fashioned repositories. Change
> DarcsRepo.remove_from_tentative_inventory not to modify the pristine
> nor the inventory.
>
> ] hunk ./src/Darcs/Repository/DarcsRepo.lhs 62
>                                     copy_patches
>                                   ) where
>
> -import System.Directory ( doesDirectoryExist, createDirectoryIfMissing )
> +import System.Directory ( doesDirectoryExist, doesFileExist,
> createDirectoryIfMissing )
> import Workaround ( renameFile )
> import Darcs.Utils ( clarify_errors )
> import Progress ( debugMessage, beginTedious, endTedious, finishedOneIO )
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 140
> format_inventory (pinfo:ps) = showPatchInfo pinfo $$ format_inventory ps
>
> write_inventory :: RepoPatch p => FilePath -> PatchSet p C(x) -> IO ()
> --- Note that write_inventory optimizes the inventory it writes out by
> +write_inventory = write_inventory_ "inventory"
> +
> +write_tentative_inventory :: RepoPatch p => FilePath -> PatchSet p C(x) ->
> IO ()
> +write_tentative_inventory = write_inventory_ "tentative_inventory"
> +
> +write_inventory_ :: RepoPatch p => FilePath -> FilePath -> PatchSet p C(x)
> -> IO ()
> +-- Note that write_inventory_ optimizes the inventory it writes out by
> -- checking on tag dependencies.
> -- FIXME: There is also a problem that write_inventory always writes
> -- out the entire inventory, including the parts that you haven't
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 151
> -- changed...
> -write_inventory dir ps = withSignalsBlocked $ do
> +write_inventory_ inventory dir ps = withSignalsBlocked $ do
>     createDirectoryIfMissing False (dir++"/"++darcsdir++"/inventories")
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 153
> -    simply_write_inventory "inventory" dir $ slightly_optimize_patchset ps
> +    simply_write_inventory inventory dir $ slightly_optimize_patchset ps
> +
>
> simply_write_inventory :: RepoPatch p => String -> FilePath -> PatchSet p
> C(x) -> IO ()
> simply_write_inventory name dir NilRL =
>

Up to this point in the patch nothing has really changed.  The code has been
lightly refactored so the changes below are possible.

hunk ./src/Darcs/Repository/DarcsRepo.lhs 202
>
> remove_from_tentative_inventory :: RepoPatch p => Bool -> [DarcsFlag] -> FL
> (Named p) C(x y) -> IO ()
> remove_from_tentative_inventory update_pristine opts to_remove =
> -    do finalize_tentative_changes
> -       Sealed allpatches <- read_repo opts "."
> +    do Sealed allpatches <- read_tentative_repo opts "."
>        skipped :< unmodified <- return $ commute_to_end (unsafeCoerceP
> to_remove) allpatches
>        sequence_ $ mapFL (write_patch opts) skipped
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 205
> -       write_inventory "." $ deep_optimize_patchset
> -                           $ mapRL_RL n2pia (reverseFL skipped) :<:
> unmodified
> -       remove_from_checkpoint_inventory to_remove
> -       when update_pristine $
> -            do pris <- identifyPristine
> -               repairable $ applyPristine pris
> -                              $ progressFL "Applying inverse to pristine"
> $ invert to_remove
> -       revert_tentative_changes
> +       write_tentative_inventory "." $ deep_optimize_patchset
> +                                     $ mapRL_RL n2pia (reverseFL skipped)
> :<: unmodified
> +       when update_pristine $ add_to_tentative_pristine
> +                            $ progressFL "Applying inverse to pristine" $
> invert to_remove
> +       remove_from_tentative_checkpoint_inventory to_remove
>

Some of these lines have been reordered and some now work with tentative
variants of the inventory.  The stuff that has been reordered is applying to
pristine and removing patches from the checkpoint.  I suspect it's better to
update all inventories and then change pristine to match.  Imagine if your
computer rebooted between those two operations.  Which order would leave
your repository in a repairable state?  I'm not certain, but my intuition
says that you can repair your pristine more easily than your inventory
(using darcs repair).  Also, what happens if we fail during
add_to_tentative_pristine?  In that case we won't make it to
remove_from_tentative_checkpoint_inventory, but we will have already updated
the tentative_inventory.  I think this means that the tentative_inventory
will be out of sync with the checkpoint's tentative_inventory.  Therefore, I
think the last thing this function should do is add_to_tentative_pristine.

Looking at add_to_tentative_pristine, I see that it updates a file
(tentative_pristine) instead of the actual pristine.  I guess that makes it
safer than what I was talking about above.  It could be safer yet by writing
to a new file and then move that to be tentative_pristine.  If I recall
correctly that's how you can do atomic file changes on unix.  Not sure how
well that works on windows.  We already do this in
finalize_tentative_changes for inventories.

We also would no longer wipe out the contents of tentative_pristine (that's
one thing revert_tentative_changes does).  Which is, I think, the cruxt of
the fix.  Instead of reverting tentative_pristine to a blank slate, now we
write everything that was tentative except the sequence to_remove.
 Hmm...Actually, we'd be writing out the inverse of the patches we want to
remove.  Since the patches are in the tentative_inventory should we be
removing them or adding their inverse?  Before we were updating the pristine
so that meant we had to apply the inverses.  Now we're just writing the
unrecorded patches to a file.  So in that case, I'm no longer sure which is
the right way to do it.

Furthermore, I just realized with these changes we may actually call
write_patch on patches that have yet to be recorded.  How could this happen?
 Suppose there are patches in tentative_inventory that are not in to_remove.
 The first thing we do is to read everything in the repo +
tentative_inventory.  This would give us a sequence containing all the
tentative patches (including the patches in to_remove) and the rest of the
patches in the repo.  Then we commute to_remove to the end so they can be
safely discarded.  But, some of the tentative patches which are not in the
inventory and not to be removed could potentially also be in the sequence
named 'skipped'.  Then we write out that whole sequence, potentially
including patches which should be only be in a tentative_inventory but now
they would be stored with the other patches yet do not appear in the
inventory.  This essentially creates orphaned patches unless we just happen
to record them soon.

I wouldn't have guessed this to be so tricky.


+
> +
> +checkpointsDir :: FilePath
> +checkpointsDir = darcsdir++"/checkpoints"
>
> finalize_tentative_changes :: IO ()
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 216
> -finalize_tentative_changes = renameFile (darcsdir++"/tentative_inventory")
> (darcsdir++"/inventory")
> +finalize_tentative_changes = do
> +    renameFile (darcsdir++"/tentative_inventory") (darcsdir++"/inventory")
> +    hasChackpoints <- doesFileExist $ checkpointsDir ++
> "/tentative_inventory"
> +    when hasChackpoints
> +        $ renameFile (checkpointsDir++"/tentative_inventory")
> (checkpointsDir++"/inventory")
>

Okay, so this has been updated to also finalize the checkpoint inventory.
 It has a typo that will be fixed in the next iteration.


finalize_pristine_changes :: IO ()
> finalize_pristine_changes =
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 227
>     do Sealed ps <- read_patches $ darcsdir++"/tentative_pristine"
>        pris <- identifyPristine
>        repairable $ applyPristine pris ps
> -    where
> +    where
>       read_patches :: String -> IO (Sealed (FL Prim C(x)))
>       read_patches f = do ps <- B.readFile f
>                           return $ case readPatch ps of
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 243
> revert_tentative_changes =
>     do cloneFile (darcsdir++"/inventory")
> (darcsdir++"/tentative_inventory")
>        writeBinFile (darcsdir++"/tentative_pristine") ""
> +       hasChackpoints <- doesFileExist $ checkpointsDir ++ "/inventory"
> +       when hasChackpoints
> +            $ cloneFile (checkpointsDir++"/inventory")
> (checkpointsDir++"/tentative_inventory")
>
> copy_patches :: [DarcsFlag] -> FilePath -> FilePath -> [PatchInfo] -> IO ()
> copy_patches opts dir out patches = do
>

Everything below this point is a refactoring.

hunk ./src/Darcs/Repository/DarcsRepo.lhs 318
>                      Just (pinfo,r) -> pinfo : read_patch_ids r
>                      Nothing -> []
>
> -read_checkpoints :: String -> IO [(PatchInfo, Maybe Slurpy)]
> -read_checkpoints d = do
> +read_tentative_checkpoints :: String -> IO [(PatchInfo, Maybe Slurpy)]
> +read_tentative_checkpoints d = do
>   realdir <- toPath `fmap` ioAbsoluteOrRemote d
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 321
> -  pistr <- fetchFilePS (realdir++"/"++darcsdir++"/checkpoints/inventory")
> Uncachable
> +  pistr <- fetchFilePS
> (realdir++"/"++checkpointsDir++"/tentative_inventory") Uncachable
>            `catchall` return B.empty
>   pis <- return $ reverse $ read_patch_ids pistr
>   slurpies <- sequence $ map (fetch_checkpoint realdir) pis
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 329
>       where fetch_checkpoint r pinfo =
>                 unsafeInterleaveIO $ do
>                 pstr <- gzFetchFilePS
> -                    (r++"/"++darcsdir++"/checkpoints/"++make_filename
> pinfo) Cachable
> +                    (r++"/"++checkpointsDir++"/"++make_filename pinfo)
> Cachable
>                 case fst `liftM` readPatch_ pstr of
>                   Nothing -> return Nothing
>                   Just (Sealed p) -> return $ apply_to_slurpy p
> empty_slurpy
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 336
>             readPatch_ :: B.ByteString -> Maybe (Sealed (Named Patch C(x)),
> B.ByteString)
>             readPatch_ = readPatch
>
> -remove_from_checkpoint_inventory :: RepoPatch p => FL (Named p) C(x y) ->
> IO ()
> -remove_from_checkpoint_inventory ps = do
> +remove_from_tentative_checkpoint_inventory :: RepoPatch p => FL (Named p)
> C(x y) -> IO ()
> +remove_from_tentative_checkpoint_inventory ps = do
>     -- only tags can be checkpoints
>     let pinfos = filter is_tag $ mapFL patch2patchinfo ps
>     unless (null pinfos) $ do
> hunk ./src/Darcs/Repository/DarcsRepo.lhs 341
> -        createDirectoryIfMissing False (darcsdir++"/checkpoints")
> -        cpi <- (map fst) `liftM` read_checkpoints "."
> -        writeDocBinFile (darcsdir++"/checkpoints/inventory") $
> +        createDirectoryIfMissing False checkpointsDir
> +        cpi <- (map fst) `liftM` read_tentative_checkpoints "."
> +        writeDocBinFile (checkpointsDir++"/tentative_inventory") $
>             format_inventory $ reverse $ filter (`notElem` pinfos) cpi
> \end{code}
>

Thanks!
Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20090905/db38e92e/attachment-0001.htm>