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

• fix-test-for-issue2310-and-rename-because-it-does-not-actually-fail.dpatch

>  * 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

I have sent two responses to the tracker (bugs@darcs.net) but they do
not show here. Has this part of the infrastructure broken down, too?

>>  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.

I am not 100% happy with this solution but I think it is better than
what we had before i.e. appending more and more primes to the variable.

Yes, normally leading _ means unused, but there very few places in Darcs
where we use it for this purpose. This means that a variable with a
leading _ stands out pretty much, which I regard as an advantage, since
this use of a variable is really special. My initial impression was "uh,
this is ugly" but one can get used to it.

Also note that the pattern is used only for variables of type Repository
and I do not think we need to use it for anything else.

I'll answer the other points you raised in a separate message.

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

Initially I thought these could be used to allow tentativelyMergePatches
to operate on the tentative state instead of the recorded one. This
would avoid having to commit inside doSuspend. But then I decided to
postpone this change. I kept the implementations in there in case we
decide to do this at a later stage.

All the other points you raise are well spotted ommisions and addressed
in the attached follow-up bundle. Thanks for the detailed review!

Attachments

• fix-doc-for-addtospecificinventory-and-addtotentativeinventory.dpatch

I'm re-reviewing this collection as a group:

>  * no longer lie about repo witnesses
>  * fix doc for addToSpecificInventory and addToTentativeInventory
>  * writeNewPending, makeNewPending: restrict witnesses
>  * restrictSubpathsAfter, maybeRestrictSubpaths: restrict
>    witnesses
>  * fix docs for finalizeRepositoryChanges
>  * tightened repo witnesses to demand wR~wT for more functions
>    in in D.R.State


In "no longer lie about repo witnesses",

> hunk ./src/Darcs/Repository/Merge.hs 250
> -                tentativelyAddPatches_ DontUpdatePristine r
> -                    compression verbosity YesUpdatePending them'
> +                tentativelyAddPatches_ DontUpdatePristine _repo
> +                    compression verbosity NoUpdatePending them'

What's this change (YesUpdatePending -> NoUpdatePending) about? 
Sorry, I think I missed it on the first review.

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

> Initially I thought these could be used to allow
> tentativelyMergePatches to operate on the tentative state
> instead of the recorded one. This would avoid having to commit
> inside doSuspend. But then I decided to
> postpone this change. I kept the implementations in there in case
> we decide to do this at a later stage.

In general I think leaving commented-out code around is just
storing up future confusion, as it probably will stop compiling
after a while, and future developers (including future-me :-))
will be inclined to just delete it anyway.

If you do want to keep it around not being used, I'd suggest
uncommenting it and adding a comment explaining why it's not
currently used so that a future developer can easily judge
whether the time has come to drop it.

Anyway, not a big deal if you still want to keep them as they are.

The rest looks good, the combined witness changes are very clear.

>> hunk ./src/Darcs/Repository/Merge.hs 250
>> -                tentativelyAddPatches_ DontUpdatePristine r
>> -                    compression verbosity YesUpdatePending them'
>> +                tentativelyAddPatches_ DontUpdatePristine _repo
>> +                    compression verbosity NoUpdatePending them'
>
> What's this change (YesUpdatePending -> NoUpdatePending) about?
> Sorry, I think I missed it on the first review.

A very good question! I had to think about this for a while. The answer
is: it doesn't matter because a few lines down we overwrite pending with
a new version anyway:

        setTentativePending _repo (effect pw' +>+ resolution)

So the NoUpdatePending saves unnecessary work. It is also cleaner and
more obvious this way, since in the Reorder case we pass
NoUpdatePending, too. I should have recorded that change separately.

>>> In Darcs.Repository.State, you've added two commented out
>>> functions, readTentative and readTentativePending, why?
>
>> Initially I thought these could be used to allow
>> tentativelyMergePatches to operate on the tentative state
>> instead of the recorded one. This would avoid having to commit
>> inside doSuspend. But then I decided to
>> postpone this change. I kept the implementations in there in case
>> we decide to do this at a later stage.
>
> In general I think leaving commented-out code around is just
> storing up future confusion, as it probably will stop compiling
> after a while, and future developers (including future-me :-))
> will be inclined to just delete it anyway.

You are right. Go ahead.

> If you do want to keep it around not being used, I'd suggest
> uncommenting it and adding a comment explaining why it's not
> currently used so that a future developer can easily judge
> whether the time has come to drop it.
>
> Anyway, not a big deal if you still want to keep them as they are.

I don't, please delete them. I have given up on the idea of reading the
tentative state for now. I think the correct way to do that is to
abandon all tentative files, hash everything that isn't already hashed
(head inventory, pending, rebase, etc), and store the hashes of any
intermediate ("tentative") states inside the Repository token. Or, even
better, inside a new "Branch" object, and add that to Repository... ;-)
The hashes are written back to disk to a small file that identifies the
branch only when we finalize the repo changes.

This will be a /major/ refactor, because it means we /have/ to return a
new Repository whenever we change anything and we /have/ to use that new
Repository in order for the changes to have any effect. It will give us
branches for free, though (that is, apart from the necessary changes and
additions to the UI).

1 patch for repository /home/ganesh/darcs/screened-temp:

patch a494e6ac72c268cf6729394c7c15f37107d57d14
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Wed Oct 17 06:49:33 BST 2018
  * delete commented out code

Attachments

• delete-commented-out-code.dpatch
• patch-preview.txt
• unnamed