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

patch f9ae44ccac2475fdcb09c7246bdcca6d2c7e46a1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  5 09:45:10 CEST 2021
  * accept issue2682

patch d9f29cea4c3d0a09ec489d98c2b512f2ba671827
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul  6 18:03:06 CEST 2021
  * improve test for issue2682

patch c4191e928af9e5163d8dea62ffa13d3ff8f3efc2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul  4 23:02:48 CEST 2021
  * re-add isConflicted method to class Conflict

patch 866ef8aac8f4cc67f91c0893f2ea0ea0c8134ae6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul  4 23:29:26 CEST 2021
  * add findConflicting to Darcs.Patch.Conflict

patch 8c843f0bdc7dabc0bacc61342cf6575e4e9fc391
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  5 13:27:51 CEST 2021
  * avoid use of the instance Named (Conflict p) for rebase unsuspend

  This is so that the fix for issue2682 does not break rebase unsuspend, but
  it also makes sense independently: when unsuspending patches we definitely
  don't want explicit dependencies to count as conflict resolutions.

patch 9411e9d8ccfb7e2c52d431c2cf2343018ec3b25b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  5 09:54:17 CEST 2021
  * resolve issue2682: conflict not marked if tag pulled at the same time

  The fix is to consider a conflict as resolved only if all conflicting
  patches are (transitively) explicitly depended on by a single patch.

Attachments

• accept-issue2682.dpatch
• patch-preview.txt

Looks good, couple of minor comments:

>   * add findConflicting to Darcs.Patch.Conflict

> +-- | Find all patches in the context that conflict with a given patch.
> +-- This works by commuting the patch and its dependencies backward until it
> +-- becomes unconflicted, then minimizing the trailing patches by re-commuting
> +-- them backward as long as that keeps the patch unconflicted.
> +-- Precondition: the context must contain all conflicting patches.

> +findConflicting
> +  :: forall p wX wY wZ
> +   . (Commute p, Conflict p, ShowPatch p)
> +  => RL p wX wY
> +  -> p wY wZ
> +  -> (RL p :> p :> RL p) wX wZ

It's not obvious to me from the signature which patches are the conflicting ones,
maybe add it to the doc comment?

> +-- | Like 'standardResolution' but it doesn't use the @instance (Named p)@
> +-- because the traling list of patches may contain "fake" conflictors.
> +rebaseResolution
> +  :: (RepoPatch p)

I wonder if it's worth tightening this signature to make it explicit that it
can't use the (Named p) instance?

>> +findConflicting
>> +  :: forall p wX wY wZ
>> +   . (Commute p, Conflict p, ShowPatch p)
>> +  => RL p wX wY
>> +  -> p wY wZ
>> +  -> (RL p :> p :> RL p) wX wZ
> 
> It's not obvious to me from the signature which patches are the conflicting ones,
> maybe add it to the doc comment?

I am not opposed to adding more details to the doc comment, but I find it pretty 
obvious that it must be the right-most RL in the result. The docs as well as the type 
signature make it clear that conflicting patches from the context are commuted past 
the single p.

>> +-- | Like 'standardResolution' but it doesn't use the @instance (Named p)@
>> +-- because the traling list of patches may contain "fake" conflictors.
>> +rebaseResolution
>> +  :: (RepoPatch p)
> 
> I wonder if it's worth tightening this signature to make it explicit that it
> can't use the (Named p) instance?

Minimizing the constraint (RepoPatch p) results in (Conflict p, PrimPatch (PrimOf p)) 
i.e. no (Commute p) and thus no instance (Conflict (Named p)). Is that what you had 
in mind?

> I am not opposed to adding more details to the doc comment,
> but I find it pretty obvious that it must be the right-most
> RL in the result. The docs as well as the type signature
> make it clear that conflicting patches from the context are
> commuted past the single p.

I did think about this for a bit without being totally sure.
Reading the comment again and your notes here, I sort of see
it, but it's still not intuitive to me. Maybe I'm just rusty
at thinking about patch orderings :-)

> Minimizing the constraint (RepoPatch p) results in
> (Conflict p, PrimPatch (PrimOf p)) 
> i.e. no (Commute p) and thus no instance (Conflict (Named p)).
> Is that what you had in mind?

Yeah, something like that. Anything that would mean the
function would stop compiling if the implementation changed
to accidentally rely on it. That combined with the doc
comment should hopefully be some protection against accidentally
reintroducing a dependency.

Following up on review.

> [...] but it's still not intuitive to me. Maybe I'm just rusty
> at thinking about patch orderings :-)

Or maybe I have done so much work on conflicts and their resolution that
things are obvious to me that aren't obvious at all to others. After all,
this is code, not a math paper :-)

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

patch bb56f8b1396187d09fb36efdfce68c5ecbdaa3bc
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Mar 22 23:34:40 CET 2023
  * generalize type signature for rebaseResolution

  It no longer requires (Commute p) and thus cannot use the instance Conflict
  (Named p), making the type reflect what we claim in the docs.

patch ab27bac46cf0a593210c6d41141e307a8dbf67ce
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Mar 22 23:46:08 CET 2023
  * improve documentation of findConflicting

Attachments

• generalize-type-signature-for-rebaseresolution.dpatch
• patch-preview.txt