Some of these patches change behavior, but in a way that is only (I think)
beneficial.

The behavior wrt broken pending is now, I think, more logical and safer. It
keeps as much of pending as is allowed, whereas previously we simply threw
it all away. (This is no longer quite as relevant these days since most of
the bugs that allowed pending to become corrupt have been fixed.)

I realized that darcs repair destroys the inventory structure when I ran it
on the darcs repo itself in order to get rid of patches with broken move
prims: some operations became noticeably slower afterwards. I regard this as
an important bug fix because other projects with a long history may also
contain such broken move prims, and darcs now fails if it encounters them
and suggests 'darcs repair'.

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

patch dd953a67e75b4c9cd0474a726459636ccda0c416
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Sep 24 09:39:34 CEST 2021
  * cleanup code layout in Darcs.Patch.Repair

patch d8927243c010e96aaa0a9d526a9969c68a6ca27d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Sep 24 09:54:01 CEST 2021
  * check/repair: also handle broken pending patch

  This required a redesign of the interface between D.R.Repair and
  D.UI.Commands.Repair i.e. the data type RepositoryConsistency. Indeed, all
  three types of problems we detect (broken patches, broken pristine, broken
  pending) can occur independently of each other, so a record type seems more
  appropriate.

  With the ability to properly fix the pending patch we no longer have to use
  the crude method of "fixing" a broken pending (by removing it) during
  command execution. Instead, commands that try to read a broken pending now
  fail with a hint to the user to run `darcs repair`.

patch 5aeaaeb4c029bd9185af16e722fb1d95dcba165b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Sep 24 10:40:55 CEST 2021
  * fix wrong use of unlines in instance Repair (FL p)

  This avoids extraneous empty lines in the check/repair output.

patch 631e79f5e5eefffaca524fe34ef8ec8fc931d6d3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Sep 24 10:08:15 CEST 2021
  * clarify internal structure of replayRepository', inline checkUniqueness

patch 58b88055cc6d81608f2ce9f682c286e75c983ddc
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Oct  5 12:21:02 CEST 2021
  * return the pristine differences from replayRepository', too

  This means we can eliminate the extra procedure brokenPristine from the
  command implementation. This removes the catchall when we read pristine
  inside replayRepository', which is a semantic change, but I haven't observed
  any problems so far, and we have a test that checks we can remove pristine
  files that still works.

patch 016761914d06d342a38db4d2588a35dc1ad1ee37
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul 18 16:14:59 CEST 2021
  * Darcs.Patch.PatchInfoAnd: cleanup imports

patch 026c01ec6881bc9421be294371b0e02c5fe5cb39
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Jun 18 23:36:03 CEST 2022
  * darcs repair: preserve inventory structure

  Previously we stored the repaired PatchSet as a single large inventory. This
  would completely destroy the breaking up of a repository into inventories.
  We now repair each Tagged section separately and rebuild it afterwards. The
  WPatchInfo and its supporting functions from PatchInfoAnd are no longer used
  and therefore deleted.

Attachments

• cleanup-code-layout-in-darcs_patch_repair.dpatch
• patch-preview.txt

Follow-up bugfix patch.

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

patch c1abf7c843b10bfd0d6c010a2f9937134a8c8090
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Nov 19 17:30:33 CET 2022
  * bugfix for darcs repair

  This fixes a bug introduced in

  patch 026c01ec6881bc9421be294371b0e02c5fe5cb39
  Author: Ben Franksen <ben.franksen@online.de>
  Date:   Sat Jun 18 23:36:03 CEST 2022
    * darcs repair: preserve inventory structure

  The problem was that darcs repair effectively failed to do any repairs.
  While the patch in question was repaired, and also the inventory directly
  referencing that patch, we failed to propagate the changed inventory hash to
  later inventories. The simple solution is to always throw away inventory
  hashes when doing repair.

Attachments

• bugfix-for-darcs-repair.dpatch
• patch-preview.txt

>   * cleanup code layout in Darcs.Patch.Repair

Fine.

>   * check/repair: also handle broken pending patch

Nice improvement, thanks!

I was wondering if an alternative representation of
`RepositoryConsistency` as a list of things that were broken would
be slightly easier to work with. The main benefit would be that
the pattern
`isJust fixedPatches || isJust fixedPristine || isJust fixedPending`
could be turned into
`null fixes` and there'd be less risk of failing to update it for
future changes. (Though that could also be accomplished with a utility
function near the datatype definition.)

I also wonder if a future refactoring could bring fixing the index
into the same logic.

>   * fix wrong use of unlines in instance Repair (FL p)
>   * clarify internal structure of replayRepository', inline 
checkUniqueness
>  * return the pristine differences from replayRepository', too
>  * Darcs.Patch.PatchInfoAnd: cleanup imports


OK

>  * darcs repair: preserve inventory structure
>  * bugfix for darcs repair

These look fine. I wouldn't have caught the bug. Is it worth a test?

Your ideas for refactoring RepositoryConsistency are appreciated 
(though I am not sure it's as easy as you make it sound, given that 
its quite different things that can be broken here). Same with 
checking the index. It might be worth a ticket so I don't forget 
next time I hack on repair.

Regarding a test to check that repair actually repairs things: that 
would be nice indeed. However, the hashed repository format makes 
constructing broken patches quite difficult, at least in a way that 
doesn't make darcs fail up-front when it tries to read the files. 
You basically have to do the hashing manually, recursively up to the 
top-level hashed_inventory. So, unfortunately, this would be yet 
another ticket rather than something I can quickly add as a follow-
up.