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

patch 46bb7acd45b236920abf807efeef7544de76d7d0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  9 17:39:18 CET 2021
  * resolve issue2674: moving unadded files

patch f28ec42284caa0477537bb5741762abf0ab31b02
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  9 17:40:02 CET 2021
  * fail in Darcs.Util.Tree.Monad.rename if source does not exist

patch 7668ca9348d86ef0c7e5772fbc5c91116bffadbf
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  9 18:29:10 CET 2021
  * TreeMonad: improve display of paths in error messages

patch 23bcf38240654ed5bda0f984950c8c490620bf52
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  9 20:05:02 CET 2021
  * bugfix in readPendingAndMovesAndUnrecorded
  
  This bug was revealed by patch f28ec42284caa0477537bb5741762abf0ab31b02
  "fail in Darcs.Util.Tree.Monad.rename if source does not exist". We must not
  apply the detected moves to the working tree, since detecting these moves
  means that the working tree already has those moves "applied". Also, in case
  we don't use the index, we want to restrict the plain working tree with the
  modified pending tree (i.e. with the detected moves applied), not the
  unmodified one.

patch 5a9ad2a061d26df2db8541590ea4257677a4d1d8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  9 19:54:37 CET 2021
  * clean up comments and variable names in readPendingAndMovesAndUnrecorded

Attachments

• patch-preview.txt
• resolve-issue2674_-moving-unadded-files.dpatch
• unnamed

My understanding from discussion on IRC is that you are planning followups
for this, so I'm marking it as followup-in-progress for now. Please change
it back if it is actually ready to review.

Here are my follow-up patches, thanks for the reminder.

As regards what to include in 2.16.4, I think that besides the front-end fix
and the test scripts we should /at least/ include the check/repair fix, so
that people can fix their repos if they are affected.

Repairing means to eliminate the bad moves, which is possible for the same
reason that the bug is, after all, not quite as bad as I thought at first:
since bad move changes are ignored when applying a patch containing them,
they are, effectively, a no-op. In particular, you cannot record anything
that depends on the target of the bad move, which is what the new test
script verifies. It is written so that it works before /and/ after the fix
for issue2674.

With the internal checks added, some commutations that involve bad move
patches will crash darcs. Running 'darcs repair' will fix that.

There is also the question of how and when to repair our own upstream repos.
Compiling a current darcs on darcs.net seems nigh impossible.

Note that the existence of the PInvalid constructor of data PatchMod shows
that the bug was known a long time ago. Indeed the comment mentions the bad
move patch in our darcs repos:

  | PInvalid a
    -- ^ This is an invalid patch
    --   e.g. there is a patch 'Move Autoconf.lhs Autoconf.lhs.in'
    --   where there is no Autoconf.lhs in the darcs repo

This kludge is now thankfully obsolete.

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

patch ef6ebddbfa74f22df12bfa5109cd5401f3868276
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 14 10:54:26 CET 2021
  * add a check/repair rule for bad move patches
  
  This drops a move patch with either non-existing source or existing target.

patch 5e642a9553bd07eb22172bd884ce4c592d95cd96
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 14 11:46:12 CET 2021
  * remove PInvalid constructor from data PatchMod

patch 96537ad0338fa352f27e008e2c6c2f2484ed882c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Mar 14 15:06:22 CET 2021
  * test that we cannot record patches that depend on broken moves

patch 36d18cea5159459c77d7e10081cb05f8bca0f5d7
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Mar 17 08:45:45 CET 2021
  * tests/broken_move.sh: if we fail to apply, darcs check should see the bad move

Attachments

• add-a-check_repair-rule-for-bad-move-patches.dpatch
• patch-preview.txt
• unnamed

Apologies, I missed that you had sent this update. I'll take a look asap.

One initial comment - I believe the 'broken_move' test fails on Windows. I 
see it fail locally and also in a CI run which only had a change that 
couldn't possibly have broken it:
https://github.com/bfrk/darcs-ci/runs/2484167935?check_suite_focus=true

> One initial comment - I believe the 'broken_move' test fails on Windows.

I have noted this a while ago and I have a fix. Will send a follow-up.

Here are a few follow-up patches. Please take a look before I screen them,
especially the last one.

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

patch 963127e19ba988d962ca560b86f808f00bcb22f5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Apr 19 22:01:01 CEST 2021
  * tests/broken_move.sh: add a hack to avoid test failure on Windows

patch a8c86b40478039ad18ede34306612c67ad3ce5d1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Apr 25 12:09:21 CEST 2021
  * add another check to tests/broken_move.sh
  
  This tests that when we unapply a bad move during a clone --to-patch, the
  command either fails with an appropriate error message or else we can 'darcs
  repair' the repo.

patch a866cb186c403748ef4d223ff60db26536f4e579
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Apr 25 12:25:55 CEST 2021
  * tests/broken_move.sh: actually test that repair works, not only check

patch 9df4a2c6e688ab1b551e4488795d3fba8c9c7da3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Apr 25 10:21:52 CEST 2021
  * remove catching of exceptions for bad moves in DefaultIO
  
  Note that DefaultIO is used only in two places: in the 'darcs test' command
  and to unapply patches when 'darcs clone' is called with a matching option.

Attachments

• patch-preview.txt
• tests_broken_move_sh_-add-a-hack-to-avoid-test-failure-on-windows.dpatch
• unnamed

On 04/05/2021 16:57, Ben Franksen wrote:

> Here are a few follow-up patches. Please take a look before I screen them,
> especially the last one.
> 
> patch 9df4a2c6e688ab1b551e4488795d3fba8c9c7da3
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Apr 25 10:21:52 CEST 2021
>   * remove catching of exceptions for bad moves in DefaultIO
>   
>   Note that DefaultIO is used only in two places: in the 'darcs test' command
>   and to unapply patches when 'darcs clone' is called with a matching option.

So the exception will bubble out and users will have to repair their
repository before they can run this command? I think that's reasonable
if they have a good way for them to discover that they need to. So maybe
we still need to catch DoesNotExistError and print out a message before
rethrowing?

BTW the `mplus` that's still there will still mean that any exception
from the renameDirectory will be swallowed. We'll only see exceptions
coming from renameFile:

https://hackage.haskell.org/package/base-4.15.0.0/docs/src/GHC-IO.html#mplusIO

> So the exception will bubble out and users will have to repair their
> repository before they can run this command? I think that's reasonable
> if they have a good way for them to discover that they need to. So maybe
> we still need to catch DoesNotExistError and print out a message before
> rethrowing?

Right. Ideally we would be able to tell which patch failed to apply, but
we don't know that here: this is only known where we define apply. But
at /that/ point we only have an ApplyMonad constraint which does not
allow to catch and throw exceptions (this is a design error). So with
the current design we can only tell the user that *some* patch failed to
apply and suggest darcs repair. I think the right place to add this
message is in runDefault.

> BTW the `mplus` that's still there will still mean that any exception
> from the renameDirectory will be swallowed. We'll only see exceptions
> coming from renameFile:

True. We should use System.Directory.renamePath.

Will send follow-ups.

Following up with better diagnostics.

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

patch 9df4a2c6e688ab1b551e4488795d3fba8c9c7da3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Apr 25 10:21:52 CEST 2021
  * remove catching of exceptions for bad moves in DefaultIO
  
  Note that DefaultIO is used only in two places: in the 'darcs test' command
  and to unapply patches when 'darcs clone' is called with a matching option.

patch b1f8e3d15719cc4c982eceb69fb08e0484ae13dd
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue May  4 22:02:25 CEST 2021
  * use renamePath instead of renameDirectory `mplus` renameFile

patch bb307ae44bb929f9f90eb3808a8b9445fa914b54
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue May  4 22:03:43 CEST 2021
  * improve error message when runDefault fails due to an IO error

Attachments

• patch-preview.txt
• remove-catching-of-exceptions-for-bad-moves-in-defaultio.dpatch
• unnamed

Go ahead and screen these, I'll do a proper review soon.

Review below.

One other scenario I thought of: what if a fixed darcs pulls from a
broken repo? I guess it'll fail trying to apply the patch, and the user
will then have to go and repair the *remote* repo. Is that right? Do we
need to clearly signpost which repo it is in that case?

>  * resolve issue2674: moving unadded files

>  * fail in Darcs.Util.Tree.Monad.rename if source does not exist

I guess this is a lower-level fix that also addresses the same problem?
(Definitely a good thing to have two guards)

>  * TreeMonad: improve display of paths in error messages
>  * bugfix in readPendingAndMovesAndUnrecorded
>  * clean up comments and variable names in
>    readPendingAndMovesAndUnrecorded

OK

>  * add a check/repair rule for bad move patches

OK. I had to think a bit about what it means for repository
compatibility to change the content of a patch without changing the
patch name, but it's safe as the repaired patch will always have the
same effect, as you point out in your comments in msg22713. Even if it's
commuted it could never have turned into something that affects a real file.

>  * remove PInvalid constructor from data PatchMod

Excellent.

>  * test that we cannot record patches that depend on broken moves

Do we really care about this test working before the fix for issue2674?
The approach seems a bit convoluted.

>   * tests/broken_move.sh: if we fail to apply, darcs check should see the bad move
>   * tests/broken_move.sh: add a hack to avoid test failure on Windows
>   * add another check to tests/broken_move.sh
>   * tests/broken_move.sh: actually test that repair works, not only check

>   * remove catching of exceptions for bad moves in DefaultIO
>   * use renamePath instead of renameDirectory `mplus` renameFile
>   * improve error message when runDefault fails due to an IO error


All fine.

> As regards what to include in 2.16.4, I think that besides the
> front-end fix and the test scripts we should /at least/ include
> the check/repair fix, so that people can fix their repos if
> they are affected.

Agreed. I'd be fine with including the whole bundle if that's
your preference.

> One other scenario I thought of: what if a fixed darcs pulls from a
> broken repo? I guess it'll fail trying to apply the patch, and the user
> will then have to go and repair the *remote* repo. Is that right? Do we
> need to clearly signpost which repo it is in that case?

It would be great if we could improve the diagnostics to indicate where
the broken patch comes from. The problem is that this is difficult to do.

>>  * resolve issue2674: moving unadded files
> 
>>  * fail in Darcs.Util.Tree.Monad.rename if source does not exist
> 
> I guess this is a lower-level fix that also addresses the same problem?
> (Definitely a good thing to have two guards)

Yes. There are basically two independent ways patch application is
implemented. There is D.R.ApplyPatches which operates directly in IO
i.e. it affects the plain tree at the current working directory, such as
e.g. the working tree. The other one is in TreeMonad, which operates on
a either a plain or a hashed tree; for plain trees the only
implementation we currently use is virtualTreeIO which never modifies
the plain tree (but we can retrieve the modified tree as part of the
result of running virtualTreeIO), whereas with hashedTreeIO we actually
modify the underlying hashed tree by adding new hashed files under
_darcs/pristine.hashed.

This patch adds the missing guards in TreeMonad. Whereas

>>   * remove catching of exceptions for bad moves in DefaultIO

does the same for D.R.ApplyPatches.

[Remark: One reason we cannot simply refactor D.R.ApplyPatches to also
use the TreeMonad (with different initial values for the 'update'
parameter) is that D.R.ApplyPatches handles setpref changes, whereas
TreeMonad ignores them. Indeed, a faithful representation of ApplyState
Prim.V1 would have to include the prefs settings. This is another can of
worms I am reluctant to open but which I'd like to get rid of when we
make the darcs-3 transition.]

>>  * add a check/repair rule for bad move patches
> 
> OK. I had to think a bit about what it means for repository
> compatibility to change the content of a patch without changing the
> patch name, but it's safe as the repaired patch will always have the
> same effect, as you point out in your comments in msg22713. Even if it's
> commuted it could never have turned into something that affects a real file.

Yes. I should add that *all* repair rules modify patches without
changing their identity.

>>  * test that we cannot record patches that depend on broken moves
> 
> Do we really care about this test working before the fix for issue2674?
> The approach seems a bit convoluted.

It is, but I wanted to make sure this is covered because of the concerns
you raised on #darcs when we first discussed this. This test gives
justification (in addition to the theoretical considerations) that we
can really drop the broken move changes from existing patches without
breaking anything, even if they are burried deep inside a repo's history.

If we agree that the changes in this bundle are correct so far, then the
next question is how much of that to include in a new patch release 3.16.4.

On 09/05/2021 10:38, Ben Franksen wrote:

>>>  * test that we cannot record patches that depend on broken moves
>>
>> Do we really care about this test working before the fix for issue2674?
>> The approach seems a bit convoluted.
> 
> It is, but I wanted to make sure this is covered because of the concerns
> you raised on #darcs when we first discussed this. This test gives
> justification (in addition to the theoretical considerations) that we
> can really drop the broken move changes from existing patches without
> breaking anything, even if they are burried deep inside a repo's history.

I'm still a bit confused about why the script is needed. Why not just
use the tarball all the time? Anyway, this bundle is ok to push
regardless of this point.


> If we agree that the changes in this bundle are correct so far, then the
> next question is how much of that to include in a new patch release 3.16.4.

I agree that the changes are correct. I don't feel too strongly about
hwo much to include, it could just be all of it.

>>>>  * test that we cannot record patches that depend on broken moves
>>>
>>> Do we really care about this test working before the fix for issue2674?
>>> The approach seems a bit convoluted.
>>
>> It is, but I wanted to make sure this is covered because of the concerns
>> you raised on #darcs when we first discussed this. This test gives
>> justification (in addition to the theoretical considerations) that we
>> can really drop the broken move changes from existing patches without
>> breaking anything, even if they are burried deep inside a repo's history.
> 
> I'm still a bit confused about why the script is needed. Why not just
> use the tarball all the time? 

Ah, okay, now I understand your question.

Well, at first this was to help me develop the test repo with the broken
move changes. It is really helpful to be able to make changes to the
test repo and then getting the tar ball re-created automatically. For
that to work, the test had to work before and after the fix to issue2674.

Now that things have settled and I am no longer making changes to the
test repo, I could simplify it to always use the tar ball. But then I
found that keeping the creation script in there is nice documentation.
Wouldn't it be quite difficult to understand the test script if you had
no idea which patches the test repo contains?

>> If we agree that the changes in this bundle are correct so far, then the
>> next question is how much of that to include in a new patch release 3.16.4.
> 
> I agree that the changes are correct. I don't feel too strongly about
> hwo much to include, it could just be all of it.

I tend to agree. Repairing broken moves is better than silently ignoring
them. However, repair only works on a single repo. What about e.g. repos
hosted on hub.darcs.net? Do we have to add a repair button to the
darcsden UI? Or should we ask Simon (CC'd) to run a repair over all
hosted repos?

>> I'm still a bit confused about why the script is needed. Why not just
>> use the tarball all the time? 
> 
> Ah, okay, now I understand your question.
> 
> Well, at first this was to help me develop the test repo with the broken
> move changes. It is really helpful to be able to make changes to the
> test repo and then getting the tar ball re-created automatically. For
> that to work, the test had to work before and after the fix to issue2674.
> 
> Now that things have settled and I am no longer making changes to the
> test repo, I could simplify it to always use the tar ball. But then I
> found that keeping the creation script in there is nice documentation.
> Wouldn't it be quite difficult to understand the test script if you had
> no idea which patches the test repo contains?

That's a good point. You could maybe add a comment to that effect to
stop someone removing the script in future on the grounds that it's useless.

Following up on review.

1 patch for repository http://darcs.net/screened:

patch 47874a8179ea3abadb1383faf5adf3dfd15c2720
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed May 12 11:48:46 CEST 2021
  * tests/broken_move.sh: explain remaining purpose of test repo creation script

Attachments

• patch-preview.txt
• tests_broken_move_sh_-explain-remaining-purpose-of-test-repo-creation-script.dpatch
• unnamed

> That's a good point. You could maybe add a comment to that effect to
> stop someone removing the script in future on the grounds that it's useless.

Good idea, done.