darcs

Patch 1715 no longer lie about repo witnesses (and 5 more)

Title no longer lie about repo witnesses (and 5 more)
Superseder Nosy List bf
Related Issues
Status review-in-progress Assigned To
Milestone

Created on 2018-07-21.20:30:07 by bf, last changed 2018-08-13.06:09:00 by ganesh.

Files
File name Status Uploaded Type Edit Remove
fix-test-for-issue2310-and-rename-because-it-does-not-actually-fail.dpatch bf, 2018-07-21.20:30:06 application/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg20218 (view) Author: bf Date: 2018-07-21.20:30:06
I have rebased some of these patches so they no longer depend on the
Transaction witness refactor, which I no longer think is a good idea
(see my comments to Issue1556). This means that
hub.darcs.net/bd/darcs-bf-latest is now obsolete; I will delete it and
re-create.

6 patches for repository http://darcs.net/screened:

patch b3109c010af6deff5e8df937e368e0ba77bfd3b0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 19 12:35:41 CEST 2018
  * fix test for issue2310 and rename because it does not actually fail

patch 4697b98a1fcecbe832f6a5c123c45353edfabe67
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 19 13:01:08 CEST 2018
  * add regression test for (already resolved) issue2536

patch a445899d1467bf217ac655f3d75e9971dca933e0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 19 19:07:44 CEST 2018
  * no longer lie about repo witnesses
  
  In particular, this means that most of the procedures in D.R.State now
  require that the recorded and the tentative state coincide, so that
reading
  the recorded state is justified even if we are in a "transaction" i.e.
  revertRepositoryChanges has been called (via withRepoLock). Also,
  finalizeRepositoryChanges now returns a properly casted Repository.
  
  We now track the Repository token using a single local variable that gets
  shadowed by each successive "update". This makes it impossible to
  accidentally use an old version. To avoid warnings, these variables are
  prefixed with an underscore.

patch c46a2aa4589102fe01d069a33dd30761f29ad5fd
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 16:09:39 CEST 2018
  * accept issue2594: darcs show index crashes replace with unrecorded
force hunk

patch 9aa931db7a4b8f3f8b5c392ed9c1452ff199086e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 19 19:10:04 CEST 2018
  * fix: must not siftForPending in revertPending
  
  The reason is that we want the checks that attempt to apply pending to
  pristine to fail as early as possible and also consistently regardless of
  whether we are in a transaction or not. If we siftForPending with a buggy
  pending such as constructed in tests/pending_has_conflicts.sh
applyToTree no
  longer fails.

patch 12b5ad555fef276cf9c8f21bd4f9f8039a8c7308
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 19 19:10:08 CEST 2018
  * resolve issue2594: add UseIndex parameter to addToPending
Attachments
msg20245 (view) Author: ganesh Date: 2018-08-13.06:08:59
>  * fix test for issue2310 and rename because it does not actually 
fail

Fine

>  * add regression test for (already resolved) issue2536

Fine

>  * no longer lie about repo witnesses
  
>  We now track the Repository token using a single local variable 
that gets
>  shadowed by each successive "update". This makes it impossible to
>  accidentally use an old version. To avoid warnings, these 
variables are
>  prefixed with an underscore.

Personally I find this a bit hard to read as I associate _ prefixes 
with
"unused". One alternative would be to just turn off the warning in a 
pragma,
but that has a global effect on the file.

> hunk ./src/Darcs/Repository/State.hs 130
> -                      => FL (PrimOf p) wT wP
> +                      => FL (PrimOf p) wX wY
> hunk ./src/Darcs/Repository/State.hs 142
> +-- note we assume pending starts at the recorded state
> hunk ./src/Darcs/Repository/State.hs 144
> -                      => FL (PrimOf p) wT wP
> +                      => FL (PrimOf p) wX wY

I think the first witness can now be wR rather than an arbitrary
state. It's no big deal as the repo is only being passed for the
types anyway.

In Darcs.Repository.State, you've added two commented out functions, 
readTentative and readTentativePending, why?

[writeNewPending]
> hunk ./src/Darcs/Repository/Pending.hs 151
> -                               -> FL (PrimOf p) wT wY -> IO ()
> +                               -> FL (PrimOf p) wX wY -> IO ()

Why does this support an arbitrary start state for pending rather 
than
the recorded or tentative states? I tried changing it and got lost 
in
all the dependencies but it feels like it ought to be one or the 
other.

> hunk ./src/Darcs/Repository/Hashed.hs 335
>  -- |addToSpecificInventory adds a patch to a specific inventory 
file, and
>  -- returns the FilePath whichs corresponds to the written-out 
patch.
>  addToSpecificInventory :: RepoPatch p => String -> Cache -> 
Compression
> -                       -> PatchInfoAnd rt p wX wY -> IO FilePath
> +                       -> PatchInfoAnd rt p wX wY -> IO ()

The doc comment is now wrong.

hunk ./src/Darcs/Repository/Hashed.hs 844

>  -- The type here should rather be
>  --  ... -> Repo rt p wR wU wT -> IO (Repo rt p wT wU wT)
>  -- In other words: we set the recorded state to the tentative 
state.
>  finalizeRepositoryChanges :: (IsRepoType rt, RepoPatch p, 
ApplyState p ~ Tree)
>                            => Repository rt p wR wU wT
>                            -> UpdatePending
>                            -> Compression
> -                          -> IO ()
> +                          -> IO (Repository rt p wT wU wT)

As is this one (or at least redundant)

> hunk ./src/Darcs/Repository/State.hs 257
>  readPendingAndMovesAndUnrecorded
>    :: (RepoPatch p, ApplyState p ~ Tree)
>    => Repository rt p wR wU wT
>    -> UseIndex
>    -> ScanKnown
>    -> LookForMoves
>    -> Maybe [SubPath]
>    -> IO ( Tree IO             -- pristine with (pending + moves)
>          , Tree IO             -- working
> -        , (FL (PrimOf p) :> FL (PrimOf p)) wT wU -- pending :> 
moves
> +        , (FL (PrimOf p) :> FL (PrimOf p)) wR wU -- pending :> 
moves

Should this also force wT=wR like readPendingAndWorking?

Also same question for readPending.

>  * accept issue2594: darcs show index crashes replace with 
unrecorded force hunk

OK

>  * fix: must not siftForPending in revertPending

OK (at least it sounds plausible, I haven't thought it through in 
detail)

>  * resolve issue2594: add UseIndex parameter to addToPending

OK
History
Date User Action Args
2018-07-21 20:30:07bfcreate
2018-08-13 06:09:00ganeshsetstatus: needs-review -> review-in-progress
messages: + msg20245