[darcs-devel] darcs patch (Current changes)

[David Roundy]

I've created an unstable branch, available at

http://abridgegame.org/repos/darcs-unstable

On Thu, Sep 16, 2004 at 03:01:47AM -0400, Juliusz Chroboczek wrote:
> 
> Thu Sep 16 08:58:29 CEST 2004  Juliusz Chroboczek <jch at pps.jussieu.fr>
>   * Make _darcs/current polymorphic.
>   This allows multiple formats for _darcs/current.  Currently, two
>   formats are implemented:
>   
>     - PlainCurrent, the directory _darcs/current that we all know and love;
>     - NoCurrent, just a placeholder file _darcs/current.none.
>   
>   A third format, HashedCurrent, is planned but not implemented yet
>   (there are stubs in Current.lhs).
>   
>   You can convert to the NoCurrent format by doing
>   
>     $ rm -r _darcs/current
>     $ touch _darcs/current.none
>   
>   and convert back to PlainCurrent by doing
>   
>     $ rm _darcs/current.none
>     $ mkdir _darcs/current
>     $ darcs repair
>   
>   There are two problems with this code that I was unable to solve.
>   First, getCurrentPop in Population.lhs is a gross violation of
>   modularity; it is actually the only reason why Current is not abstract
>   (it has to export PlainCurrent).  Second, surely_slurp_Current in
>   Repository.lhs leaves a temporary directory after darcs terminates;
>   we're bitten by the laziness of slurp.  These problems should be
>   solved before release.
>   
>   Converting Current to be a type class instead of an algebraic
>   datatype is left as an exercice for the reader (hint: you need
>   existential types).

> hunk ./Apply.lhs 46
> +import Current ( identifyCurrent, slurp_write_dirty_Current )

Perhaps identifyCurrent should be implicit in the various _Current
commands? I guess the advantage of the way you wrote it is that you don't
need to reidentify it if you do multiple operations... that may be a good
reason.

Naming-wise, I'd prefer to drop the "slurp_" from slurp_write_dirty_Current
etc.  I think it's pretty clear without the slurp_, and a bit more
readable:  "write the dirty stuff to current".  A good opportunity here for
darcs replace...

> hunk ./Check.lhs 20
> +import Maybe ( fromJust, isJust )

I prefer to use the fromJust macro included in "impossible.h" to the
standard library fromJust, since the macro lets us know on failure which
fromJust failed.

> +module Current ( Current(PlainCurrent),
> +                 createCurrent, removeCurrent, identifyCurrent,
> +                 checkCurrent,
> +                 slurpCurrent, mmap_slurp_Current,

I think perhaps we can leave out mmap_slurp_Current, and just make
slurpCurrent do a mmap? I think that on posix systems, Current is always
safe to mmap (since darcs never modifies files, only creates new files and
renames them).  On windows, I don't think mmap *ever* works, and if it
does, we can deal with the breakage when it happens.

The only thing we can't mmap_slurp is the working directory (since it might
be modified by the user), or the "external resolution" directories.
Keeping in mind that mmap_slurp is the same as slurp if mmap was disabled
at configure time.

> hunk ./Population.lhs 41
> +import Current ( Current(PlainCurrent), identifyCurrent )
> hunk ./Population.lhs 256
> -\begin{code}
> +\begin{code} 
> +
> +-- This shouldn't be here, but I see no other way of avoiding a
> +-- circular dependency between modules.  I guess this has to be
> +-- rethought -- jch
> +
> +getCurrentPop :: PatchInfo -> Current -> IO (Maybe Population)
> +getCurrentPop pinfo PlainCurrent = 
> +    Just `liftM` getPopFrom "_darcs/current" pinfo
> +getCurrentPop _ _ = return Nothing
> +
> hunk ./Population.lhs 271
> -      getPopFrom (repobasedir ++ "/_darcs/current") pinfo
> +      mp <- withCurrentDirectory repobasedir $ 
> +                identifyCurrent >>= getCurrentPop pinfo
> +      case mp of
> +          (Just pop) -> return pop
> +          (Nothing) -> getRepoPopVersion repobasedir pinfo

Perhaps moving getPopFrom into PopulationData, and then getCurrentPop into
Current? It's a bit ugly putting a pop function into PopulationData, but
the whole reason that module exists is to avoid circular dependencies, so
it's perhaps not so bad...

> hunk ./Repository.lhs 243
> +surely_slurp_Current :: Current -> IO Slurpy
> +surely_slurp_Current cur = do
> +    mc <- mmap_slurp_Current cur
> +    case mc of
> +        (Just slurpy) -> return slurpy
> +        Nothing -> do
> +           patches <- read_repo "."
> +           -- we should be using checkpoints!
> +           -- slurp doesn't like the files to disappear; we hack around
> +           -- it for now by using withPermDir, which means that the
> +           -- temporary directory is never deleted.  What we need is a 
> +           -- strict version of slurp, or a delayed version of withTempDir
> +           -- (that only deletes the directory when darcs terminates).
> +           withPermDir "current.temp" $ \cd -> do
> +               apply_patches noPut noPut $ reverse $ concat patches
> +               mmap_slurp cd
> +             where noPut _ = return ()

A delayed version of withTempDir sounds like a good idea.  Or perhaps we
could stick a finalizer on the Slurpy that would delete the directory? That
would be pretty fancy.  Perhaps something like

keepingTemporaryDir :: (FilePath -> IO a) -> IO a

which would stick a finalizer on the "a" output, and not delete the
directory until that output is no longer accessible? This wouldn't be a
complete solution, since the internals of that data may still be
accessible... probably best to just delete the directory on termination.

That's what comes to mind on the first pass through.
-- 
David Roundy
http://www.abridgegame.org