I am not screening this one yet, please take a look and tell me if you think
this is a bad idea. With this patch I can successfully push and pull between
the two repos that lemming attached to the ticket (after reverting
unrecorded changes).

This patch breaks the test for issue1879 which is to be expected. Detecting
inconsistencies between repos (this includes malicious patch manipulation)
is a valid goal but I think we should aim for something more reliable. That
is, commute all (supposedly) common remote patches into the same context as
the version in our repo and then compare contents; which would not interfere
with the proposed mitigation for issue2738.

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

patch 3bba990fe96e421f81bb2cf45ece9994d075ce35
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jan 29 13:13:20 CET 2025
  * mitigate issue2738: make merging more robust in the face of V1/V2 commute bugs

  We add a new method robustMerge that is able to handle identical patches by
  returning "null patches" (represented as Nothing2 :: Maybe2), and base the
  generic instance Merge for FLs on that, instead of the plain merge. This
  allows us to keep common patches that cannot be commuted to the tail in the
  "uncommon" branches (in functions like findCommonFL) instead of calling
  error.

Attachments

• mitigate-issue2738_-make-merging-more-robust-in-the-face-of-v1_v2-commute-bugs.dpatch
• patch-preview.txt

I'm still thinking this over (along with your separate email about
commute consistency) but some initial thoughts:

There's no robustMerge instance for `FL` which means that I think
now merge on `FL (FL (Named p))` won't behave the same as merge
on `FL (Named p)`.

I wonder if the implementation could instead be via a merge
instance for `Maybe2 p` or even some special wrapper
`AllowDuplicates p` which behaves the same but whose intended
behavior is clearer. Then just the code that handles "repo merges"
would use the wrapper so the effect would be more tightly scoped.

Also if V1/V2 are the only things to have these bugs maybe we only
need to use this there?

Sorry, I was wrong about merge on FL (FL p) not being the same
as merge on FL p - I now see that once you get to FL you
don't need robustMerge any more because you can just collapse the
list.

Yes, exactly! For sequences we identify Nothing2:>:NilFL with NilFL, which 
is how catMaybes2 gets rid of the Maybe2s.

Following up with an optimized version.

1 patch for repository /home/franksen/src/darcs/screened-with-mitigation-of-issue2738:

patch 697ac0eee2d3606c18fc2081bbf97ceee3945c0e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jan 31 09:14:57 CET 2025
  * minimize the amount of Maybe2 wrapping and unwrapping in merge for FLs

  This replaces the implementation in terms of the generic mergerFLFL with a
  direct one, at the cost of some (minor) code duplication. The implementation
  uses monadic style with the Identity functor in order to avoid nested case
  expressions.

Attachments

• minimize-the-amount-of-maybe2-wrapping-and-unwrapping-in-merge-for-fls.dpatch
• patch-preview.txt

BTW even though I was wrong about the FL FL thing, I still would
argue for a wrapper type over the new class method approach. It
seems less scary in terms of potential impact.

Could you illustrate (or explain a bit more) what you mean with the 
wrapper type approach? Which patch types do you want to wrap and what 
difference does that make?

Instead of having robustMerge, introduce

newtype AllowDuplicates p wX wY = AllowDuplicates (Maybe2 p wX wY)

with a merge that does what robustMerge does now

Then when we are about to do an operation that requires it, wrap
all our Named/PatchInfoAnds in AllowDuplicates, do the merge,
and unwrap (dropping the Nothing2s).

I see. So, basically, we move the decision whether to do the 
wrapping/unwrapping or not to the call site.

This has a number of drawbacks:

This a much more invasive change. It means we have to find every place where 
we use merge and make a decision. What if we miss one? Or forget about it 
and use the wrong function when making a change in the future?

Also, what about generic functions that are built on merge but also require, 
say, commute or, let's say, displayPatch? Should they now all come in two 
varieties? Or do we also define instances of all these classes for 
AllowDuplicates?

Considering these costs, what exactly are the benefits?