Rebased from an old branch of mine. More changes to the test suite will
follow.

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

patch 1838f6c90a6706f583e8d8d7187abb391b50ae37
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat May  6 17:47:16 CEST 2023
  * harness: cleanup RepoPatchV2 examples

  This gets rid of the whole Unwitnessed stuff (Darcs.Test.Patch.WSub,
  Darcs.Test.Patch.Properties.GenericUnwitnessed), renames the module,
  and mostly returns list of Sealed2 to avoid coercions.

patch 298bdc03557170be6b8b3c7932a78f87dc4e8ad3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jun 15 10:08:11 CEST 2022
  * harness: prepare generalization of RepoPatch to Mergeable

  Recorded as a separate patch for easier review.

patch 6988f752db731a838c7f64c97d942397b3c903e8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jun 15 09:20:00 CEST 2022
  * harness: add proper QC tests for Named patches

  This generalizes RepoPatch properties to any Mergeable patch type. Since we
  want to generate Named patches with meaningful explicit dependencies, we
  have to keep track of the names of patches that have been applied to a repo
  (model). This requires the introduction of yet another (somewhat ad-hoc)
  class RepoApply to add a method 'patchNames' to all testable patch types.
  The default implementation that returns [] is used for all patch types
  except for Named patches.

patch e3609fec29e212f8872480a11c0af4c6c5c5bd09
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue May 28 10:31:33 CEST 2024
  * D.T.Patch.Properties.RepoPatch -> D.T.Patch.Properties.Mergeable

  These properties now work for any Mergeable patch type.

patch 9dfdb0683703b1f275254fb4895171926395f07d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun May 26 08:12:08 CEST 2024
  * harness: disable propResolutionsOrderIndependent for Named patches

  This property occasionally fails with Named patches. Furthermore, we get
  exceptions when trying to shrink the test complaining about invalid patch
  names. This needs further investigation.

Attachments

• harness_-cleanup-repopatchv2-examples.dpatch
• patch-preview.txt

Reviewing incrementally

>  * harness: cleanup RepoPatchV2 examples

>     -  , testCases "fails" (PropU.commuteFails WSub.commute) ([] :: [(Prim2 WSub.:> Prim2) wX wY])

Is this removal intentional?

Rest of this patch looks good - thanks for finally getting rid of this junk!

>   * harness: prepare generalization of RepoPatch to Mergeable

OK

> -  , testCases "fails" (PropU.commuteFails WSub.commute) ([] :: [(Prim2 
WSub.:> Prim2) wX wY])

The list of test cases was empty, so yes, intentionally dropped.

> The list of test cases was empty, so yes, intentionally dropped.

Doh, missed that completely!

> Since we want to generate Named patches with meaningful explicit 
> dependencies, we have to keep track of the names of patches that
> have been applied to a repo (model). 

I haven't thought this through fully, but I think I'd have done this
by having a wrapper type for models that adds names, and having

ModelOf (Named p) = WithNames (ModelOf p)

or something like that.

Is that plausible?

> ModelOf (Named p) = WithNames (ModelOf p)

That is quite plausible and indeed looks a lot cleaner to me. I wonder 
why it didn't occur to me to do it like that.

I am inclined to remove this patch from screened and redo it based on 
your suggestion. The reason is that I want to see how much of the churn 
can be avoided with your approach. Might take me a few days to come up 
with a new patch though.

I am running into all kinds of typing problems, so this might be a bit 
more difficult that I initially thought. Might take me a few days to sort 
these problems out.

For the moment a follow-up patch that rolls back the changes in
D.T.P.V1Model and D.T.P.FileUUIDModel and uses 

type instance ModelOf (Named p) = WithNames (ModelOf p)

This refactor prompted me to fix a bug in the `instance ArbitraryState
(Named p)` where I failed to add the new patch name to the model; perhaps
this will fix the mysterious failures of propResolutionsOrderIndependent for
Named patches.

There remain two type errors where ghc complains that it couldn't match
`ModelOf prim` with `WithNames (ModelOf prim)`. These are in parts of the
test suite which should be more familiar to you; I can't figure out what
goes on here and why these functions/instances require the two types above
to be equal. If you can come up with a solution that is not too complicated,
I will integrate it and send a cleaned up patch.

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

patch ab1bbd97fdd933647f857ddd48504d3b37d8319a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun  4 21:59:12 CEST 2024
  * WIP RepoModel for Named patches

Attachments

• patch-preview.txt
• wip-repomodel-for-named-patches.dpatch

The basic problem is that we have constraints like

ModelOf p ~ ModelOf (PrimOf p)

and since

PrimOf (Named p) = PrimOf p

this isn't working out too well.

But I'm struggling to get the type-checker to point me
at the actual problematic code that relies on this
assumption.

OK - I think at least some of the reason is the logic for shrinking
patches called from the instance ArbitraryS2 (WithStartState2 p).

One strategy for shrinking patches involves shrinking the model and
then propagating around a primitive patch that expresses how the
model was shrunk.

To fix it we might really want something that's almost PrimOf,
but does preserve Named. Feels like overkill compared to your
original approach.

I see. Yes, it's a dilemma.

We got away with using the same type of model as for prims, even for named 
prims and RepoPatchV3, by storing these patches in unconflicted form 
(MergeableSequence/OnlyPrim), which means that patch identities can be 
disregarded, insofar as their action on the model is concerned. For actual 
Named patches this no longer works because explicit dependencies add a 
completely new dimension to the action of a patch.

Could we perhaps find a solution along the line of generalizing

  ModelOf p ~ ModelOf (PrimOf p)

to something like

  PrimModelOf (ModelOf p) ~ PrimModelOf (ModelOf (PrimOf p))

where PrimModelOf (WithNames m) = m?

Ideally we'd want the model type that shrinkModel sees to include
WithNames.

That way in future we could add ways to simplify a test case by
removing names from the starting model and then drop them from the 
dependencies of patches too.

I think that would actually mean that the things we want to push around 
for tracking the simplification end up looking quite like RebaseFixup.

That change wouldn't be necessary as part of introducing WithName
though.

> Ideally we'd want the model type that shrinkModel sees to include WithNames.

Indeed, that occurred to me as well and for the same reasons. Do you see a 
chance to get that working?

>> Ideally we'd want the model type that shrinkModel sees to include 
>> WithNames.

> Indeed, that occurred to me as well and for the same reasons.
> Do you see a chance to get that working?

Eventually, yes. But unless you really want to work on it now
I would suggest not derailing your current improvements for that.

If I was working on it I'd probably start by thinking a bit harder
about the relationships between all the different types. We have the
real patch type we want to test, then the `PrimOnly` variant used for
the raw test cases (mainly also there to make shrinking easier),
and now I suspect we might need a third type like RebaseFixup to
propagate shrinks. And we want the model type to include names,
and ideally we'd build this all up in layers to make it more
compositional.

I just realised while thinking about the issues with WithNames
that your idea about MPTCs for Apply is also relevant to ShrinkModel.

I've just sent patch2407 to play with that - it builds, but I haven't 
checked anything about it nor have I verified whether it actually solves 
the issues with using "WithNames" here.

It does get us away from ModelOf (PrimOf p) ~ ModelOf p though, so it 
seems like it has a decent chance of it.

I'll have a play with using it for WithNames properly as soon as I have 
some more time, unless you get there first.

2 patches for repository darcs-unstable@darcs.net:/opt/darcs/screened:

patch ab1bbd97fdd933647f857ddd48504d3b37d8319a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun  4 20:59:12 BST 2024
  * WIP RepoModel for Named patches

patch 94212e1492333d196cb79a841ff721c0d96863df
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Thu Jun  6 02:05:32 BST 2024
  * key instance

Attachments

• patch-preview.txt
• wip-repomodel-for-named-patches.dpatch

Actually did a bit more poking and it does typecheck now - it's just
a two line instance to combine patch2407 with your WIP patch which
I think captures exactly what is wanted here.

This consolidates my WIP patch with your "key instance" follow-up.

The second patch is something I needed to debug issue2727 but makes sense
independently IMO.

2 patches for repository https://darcs.net/screened:

patch fd2fa87680ae5d6c78d50a495c8d671818606eb6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun  4 21:59:12 CEST 2024
  * add WithNames repo model wrapper for Named patches

  This is much cleaner than hacking the applied patch names into the existing
  repo models. Also led to a clean instance ArbitraryState (Named p) (which
  was buggy before) and may in the future allow to shrink Named patches by
  removing some of their explicit dependencies.

patch 10bfc319f392fdd2c29ae0d538463794e168c00c
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 11 13:28:17 CEST 2024
  * more useful instance Arbitrary PatchInfo for Named patches

  Shrinking patch names is problematic due to explicit dependencies and not
  needed if we generate only minimal, human readable (and always valid) names.

Attachments

• add-withnames-repo-model-wrapper-for-named-patches.dpatch
• patch-preview.txt

>   * harness: add proper QC tests for Named patches
>   * add WithNames repo model wrapper for Named patches

Looked at these two together and they look fine (didn't read every 
detail).

I was disappointed that RepoApply is still needed, but after thinking
about it a bit I realised it's natural because Apply is never going
to care about names.

>   * harness: disable propResolutionsOrderIndependent for Named patches
>   * D.T.Patch.Properties.RepoPatch -> D.T.Patch.Properties.Mergeable
>   * more useful instance Arbitrary PatchInfo for Named patches

all fine

Are the last two patches ready to push? (Just checking as they're not
in screened yet)

They're ready, go ahead.

I can't push right now because I lost my private key (I should have a 
copy somewhere but can't seem to find it). And I never set a password for 
my account on darcs.net (a mistake as it turns out). I have attached my a 
public key. Could you append that to my authorized_keys on darcs.net?

Attachments

• id_ed25519.pub