darcs

Patch 236 In replacePristine, cope also with Trees... (and 2 more)

Title In replacePristine, cope also with Trees... (and 2 more)
Superseder Nosy List kowey, mornfall
Related Issues
Status accepted Assigned To
Milestone

Created on 2010-05-06.15:27:10 by mornfall, last changed 2011-05-10.19:35:40 by darcswatch. Tracked on DarcsWatch.

Files
File name Status Uploaded Type Edit Remove
in-replacepristine_-cope-also-with-trees-that-have-no-hashes-in-them_.dpatch mornfall, 2010-05-06.15:27:10 text/x-darcs-patch
unnamed mornfall, 2010-05-06.15:27:10
See mailing list archives for discussion on individual patches.
Messages
msg10989 (view) Author: mornfall Date: 2010-05-06.15:27:10
Hi,

this resolves the immediate problem reported as issue1837. A regression test
would be welcome but I don't have time to work on that. The first two patches
are required, and can be reasonably cherry-picked for branch-2.4 as well.

There are caveats though...

<mornfall> kowey: (I have fixed the --partial bug, *but* the whole way it works
         is pretty dangerous...)
<mornfall> kowey: I.e. if *something* writes into the working copy while the
         get is running, it will corrupt pristine, hashed or not (since *this*
         variant of get will work in working and then copy the result over to
         pristine, instead of the other way around)
<mornfall> Most get variants work the other way around (pristine first), which
         is safe.
<mornfall> (Bloody mess.)

Yours,
   Petr.

3 patches for repository darcs-unstable@darcs.net:darcs:

Thu May  6 17:15:24 CEST 2010  Petr Rockai <me@mornfall.net>
  * In replacePristine, cope also with Trees that have no hashes in them.

Thu May  6 17:19:43 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1837: Add readWorking and use it in pristineFromWorking.

Thu May  6 17:20:35 CEST 2010  Petr Rockai <me@mornfall.net>
  * Also use readWorking in setScriptsExecutable (minor refactor).
Attachments
msg10996 (view) Author: kowey Date: 2010-05-07.11:58:47
On Thu, May 06, 2010 at 15:27:10 +0000, Petr Ročkai wrote:
> this resolves the immediate problem reported as issue1837. A regression test
> would be welcome but I don't have time to work on that. The first two patches
> are required, and can be reasonably cherry-picked for branch-2.4 as well.

I confirm this applies on branch-2.4 (needs a refactor patch from David M which
seems harmless enough).

I'm not going to push this just yet... some things to figure out with the tests.

> <mornfall> kowey: (I have fixed the --partial bug, *but* the whole way it works
>          is pretty dangerous...)
> <mornfall> kowey: I.e. if *something* writes into the working copy while the
>          get is running, it will corrupt pristine, hashed or not (since *this*
>          variant of get will work in working and then copy the result over to
>          pristine, instead of the other way around)
> <mornfall> Most get variants work the other way around (pristine first), which
>          is safe.
> <mornfall> (Bloody mess.)

This is a comment on the pre-existing setup, not Petr's changes

We talked about it a bit.  When fetching from old-fashioned
repositories, we apply patches to the working dir (checkpoint or
otherwise) and then copy from working to pristine.  [In the darcs 1
client, things were even worse; we'd just copying the pristine, I think,
which hid a lot of errors that only resurfaced when darcs 2 client
became more thorough].  This ought to change for the future.

> Thu May  6 17:15:24 CEST 2010  Petr Rockai <me@mornfall.net>
>   * In replacePristine, cope also with Trees that have no hashes in them.
> 
> Thu May  6 17:19:43 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Resolve issue1837: Add readWorking and use it in pristineFromWorking.
> 
> Thu May  6 17:20:35 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Also use readWorking in setScriptsExecutable (minor refactor).


In replacePristine, cope also with Trees that have no hashes in them.
---------------------------------------------------------------------
> hunk ./src/Darcs/Repository.hs 337
>      where replace HashedPristine =
>                do let t = darcsdir </> "hashed_inventory"
>                   i <- gzReadFilePS t
> -                 root <- writeDarcsHashed tree $ darcsdir </> "pristine.hashed"
> +                 tree' <- darcsAddMissingHashes tree
> +                 root <- writeDarcsHashed tree' $ darcsdir </> "pristine.hashed"

I'm guessing this computes the hash for all files in the tree we don't
yet know the hash for.

Worth haddocking?

  darcsAddMissingHashes :: (Monad m, Functor m) => Tree m -> m (Tree m)
  darcsAddMissingHashes = updateTree update
      where update (SubTree t) = return . SubTree $ t { treeHash = darcsTreeHash t }
            update (File blob@(Blob con NoHash)) =
                do hash <- sha256 <$> readBlob blob
                   return $ File (Blob con hash)
            update (Stub _ NoHash) = fail "NoHash Stub encountered in darcsAddMissingHashes"
            update x = return x

I gather this is necessary because we want to account for the tree coming from
an unhashed source (ie. working).

It doesn't sound like it could hurt in any case.  Could there be some sort of
performance issue involved?

Resolve issue1837: Add readWorking and use it in pristineFromWorking.
---------------------------------------------------------------------
> hunk ./src/Darcs/Repository.hs 353
>  pristineFromWorking :: RepoPatch p => Repository p C(r u t) -> IO ()
>  pristineFromWorking repo@(Repo dir _ rf _)
>      | formatHas HashedInventory rf =
> -        withCurrentDirectory dir $ readUnrecorded repo >>= replacePristine repo
> +        withCurrentDirectory dir $ readWorking >>= replacePristine repo

This would normally alarm me, but I guess it's in a function called
pristineFromWorking (the idea of which is a bit yuck-inducing)
  
> +-- | Obtains a Tree corresponding to the working copy of the
> +-- repository. NB. Almost always, using readUnrecorded is the right
> +-- choice. This function is only useful in not-completely-constructed
> +-- repositories.
> +readWorking :: IO (Tree IO)
> +readWorking = expand =<< (nodarcs `fmap` readPlainTree ".")
> +  where nodarcs = Tree.filter (\(AnchoredPath (Name x:_)) _ -> x /= BSC.pack "_darcs")

Yay haddock. Seems fine, maybe I'm missing something.
We could use the new darcsdir.

Also use readWorking in setScriptsExecutable (minor refactor).
--------------------------------------------------------------
> -    let nodarcs = Tree.filter (\(AnchoredPath (Name x:_)) _ -> x /= BC.pack "_darcs")
> -    tree <- expand =<< (nodarcs `fmap` readPlainTree ".")
> +    tree <- readWorking

Yeah

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
msg11012 (view) Author: kowey Date: 2010-05-07.17:55:30
These were accepted, but I had broken the patch tracker.
msg14164 (view) Author: darcswatch Date: 2011-05-10.19:35:40
This patch bundle (with 3 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-8c49b0ba9fce7b090f591d7ebf0be8983562231c
History
Date User Action Args
2010-05-06 15:27:10mornfallcreate
2010-05-06 15:28:16darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-8c49b0ba9fce7b090f591d7ebf0be8983562231c
2010-05-07 11:58:48koweysetstatus: needs-review -> accepted-pending-tests
nosy: + kowey
messages: + msg10996
2010-05-07 17:55:31koweysetstatus: accepted-pending-tests -> accepted
nosy: - darcs-users
messages: + msg11012
2011-05-10 19:35:40darcswatchsetmessages: + msg14164