Sending this patch (together with an unrelated dependency) ahead of a large
bundle because the latter depends on this one. While I am not 100% happy with
the solution, I think it is an improvement compared to what we had before.

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

patch 02340d3264aa2ac1826f86d8f8eef8afe170c43f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jun 22 16:04:08 CEST 2022
  * add module docs to Darcs.Patch.Named

patch 52607880180217735503a054736e7d81556a8154
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Nov 16 17:40:09 CET 2021
  * refactor ApplyMonad methods

  The main goal was to get rid of the overly complicated way in which the (now
  removed) methods liftApply, nestedApply, and getApplyState were used in
  Darcs.Patch.Viewing.showContextSeries. These methods are replaced by a
  single method readFilePS that directly reads a file's content from the
  state. To achieve a uniform interface for all possible ApplyStates, we
  abstract over the concrete type that is used to look up a file object from
  an ApplyState, using the type function ObjectIdOf; for Tree this is
  AnchoredPath, while for ObjectMap it is UUID. We also need to generalize
  formatFileName to a method formatObjectId to be used when displaying a patch
  with context lines.

  Please note that this changes the behavior of showContextPatch which is now
  supposed to apply the patch in the ApplyMonad given by the context. This is
  unproblematic for now, as all calls to showContextPatch are wrapped in a
  call to virtualTreeIO.

Attachments

• add-module-docs-to-darcs_patch_named.dpatch
• patch-preview.txt

Reviewing incrementally

>   * add module docs to Darcs.Patch.Named

Fine

>   * refactor ApplyMonad methods

Thanks, this does indeed look like a good cleanup. If I understand
correctly, the main reason you need to generalise various bits of
code over the new ObjectIdOf types is that you actually filled in
missing functionality in FileUUID patch types and therefore had
to abstract the rest of the code to work with that?

> Please note that this changes the behavior of showContextPatch
> which is now supposed to apply the patch in the ApplyMonad given
> by the context.

This seems like slightly odd behaviour given its name, and an easy
source of mistakes for an implementer, particularly as composite
implementations (like RepoPatches) have to decide between delegating
the "apply" operation and doing it thesmselves. Would it be better
to replace the class method by something else that doesn't need to
do the apply, and then have a helper function for callers to use?

I also noticed that ApplyMonadBase can now be removed, I'll send a
followup for that.

Regarding FileUUID: it still doesn't show any context when you call 
showContextPatch, I just implemented the readFilePS method. The reason is 
that we have no sensible IsHunk instance for FileUUID.

This is a big problem, since a lot of stuff inside the Darcs.Patch layer 
relies on that instance. The internal representation of Hunks in FileUUID 
makes more sense to me than that in V1, as it is more efficient to apply 
and is agnostic to end-of-line conventions. But it is impossible to convert 
between the two unless you have access to the state.

One way to move forward would be to change the Hunk format in FileUUID back 
to the one in V1. Another is to bite the bullet, eliminate FileHunk and 
IsHunk, and properly generalize everything that now depends on them so that 
we can implement it for FileUUID hunks.

Whether we choose one way or the other, we still need something like the 
type function ObjectIdOf as a key look up a file or directory from the 
state.

> Regarding FileUUID: it still doesn't show any context when you call 
> showContextPatch, I just implemented the readFilePS method.

Fair enough. The main point of my question was to understand why
all the overloading on ObjectOfId became necessary now when it
wasn't before, and I *think* that's because of this readFilePS
implementation?

I like the idea of separating the apply from showContextPatch and instead 
use a generic helper function for the combination of the two. I wonder 
why I didn't think of that myself.

>> Regarding FileUUID: it still doesn't show any context when you call
>> showContextPatch, I just implemented the readFilePS method.
> 
> Fair enough. The main point of my question was to understand why
> all the overloading on ObjectOfId became necessary now when it
> wasn't before, and I *think* that's because of this readFilePS
> implementation?

Basically, yes. I think I generalized AnchoredPath to ObjectIdOf in a few 
more places where it wasn't strictly necessary (e.g. 
Darcs.Patch.Prim.Canonize), probably to convince myself that this extra 
complication was worth the trouble...

Here's a followup with one possible renaming
following the discussion in msg23560 and
msg23561 (which were accidentally sent to the
wrong patch).

I'm not going to screen these immediately as
the names are a bit verbose.

1 patch for repository darcs-unstable@darcs.net:/opt/darcs/screened:

patch 75968ededceceeccff7f7bb3b19154d9a03bfd3c
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Thu Jul 13 22:42:21 BST 2023
  * rename showContextPatch and showPrimCtx

  This should make their behaviour clearer following

    patch 52607880180217735503a054736e7d81556a8154
      * refactor ApplyMonad methods

Attachments

• patch-preview.txt
• rename-showcontextpatch-and-showprimctx.dpatch

>  * rename showContextPatch and showPrimCtx

I am okay with the long names. It would be a problem if these 
methods were used all over the place, but there is actually just a 
single use outside of Darcs.Patch.

BTW, I have a patch that re-adds a showPatchWithContext (w/o apply) 
to Darcs.Patch.Show, by slightly generalizing what we do in 
contextualPrintPatchWithPager, so that the method that also applies 
the patch is no longer needed outside of Darcs.Patch and not 
exported via Darcs.Patch.

Should I send to this ticket or a new one?

> Should I send to this ticket or a new one?

Let's go with a new ticket so I can accept this one without doing
more reviewing.