These are the last 2 patches from Ben's darcs-encoding branch, the other
ones are at http://bugs.darcs.net/patch1592 .

They apply cleanly on HEAD and all tests pass (wiht darcs1 and darcs2
repositories).

Note that the issue raised by Ben in his mail [1] are created by the
first patch below.

[1] https://lists.osuosl.org/pipermail/darcs-devel/2017-May/018016.html

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

patch 0b44fbb1718bbb2b61724fc9eeed3ae2f4ebd583
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 31 17:17:28 -03 2017
  * clean up display of file names by separating display and storage of
patches

  Darcs has always used the same code to serialize patches for storage
on disk
  and for displaying them to the user. This makes sense in terms of code
  economy but until now led to strange issues when displaying non-ASCII
  filenames.

  This patch makes a few small changes at the bottom of the hierarchy of
  classes that make up the Patch API, but these small changes necessitate
  rather deep and far-reaching changes in many other places.

  The first change is to add a second parameter to showPatch and
  showContextPatch to indicate the intended use (ForDisplay |
ForStorage). The
  correct value for this parameter must be added everywhere these functions
  are used, which is tedious but mostly trivial.

  The second is to remove the instances of Darcs.Patch.Prim.V1.Prim for
  ShowPatchBasic and ShowPatch. The reason is that the Prim patch type is
  shared (as a type parameter) between RepoPatchV1 and RepoPatchV2, so it
  *cannot* know which file name encoding to use for storage. (This is why
  there are PrimShow and PrimRead classes that get the file name format
passed
  as a parameter.) Nevertheless, we *need* primitive patches to implement
  these classes! Half of the rest of the darcs code and large parts of the
  test harness rely on prim patches having the same API as full repo
patches.

  The dilemma is solved by adding yet another layer of types: V1 and V2 now
  each define their own newtype wrapper for the shared Prim type. This
wrapper
  has knowledge about how to render file names (and also patch lists) in the
  correct format and thus can implement ShowPatchBasic, ShowPatch, and
the now
  separate ShowContextPatch classes. This is the type that is now
returned by
  the PrimOf type function.

  Incidentally, this patch fixes a failure for darcs1 format of
  issue2382-mv-dir-to-file-confuses-darcs.sh that was causes by the
output of
  darcs record adding (legacy) braces around patch lists for display. We now
  use the ListFormatDefault (no braces) for all display so this problem just
  vanished. Similarly, the peculiar darcs-specific "white space
encoding" for
  filenames is now strictly limited to the on-disk format of patches
  (including patch bundles) and inventories and no longer affects the UI.

patch 1fe1c8617a2b6c8bc753f4baa3033168163ad3ae
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug 31 17:20:21 -03 2017
  * use formatFileName UserFormat in Darcs.Patch.Summary

Attachments

• clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch

Fixing a warning (anyway the test harness code has warnings because of
the commented code that Ben mentionned in his mail).

Attachments

• clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch

I guess you didn't manage to fix the qc_prim test? I spent a while 
trying to do that a few months ago and failed, and I had the feeling 
it indicated a more serious issue with the new type structure.

Sorry for never writing those experiments up, I'll try to do so in 
the next couple of days. I was leaning towards just adding the extra 
parameter to the existing classes, and ignoring it in some cases, 
ugly though that is.

Hi Ganesh,

I have not even tried, in fact. This bundle is just the result of
porting the original patches to screened (and compiling with ghc 8.2 and
cabal 2).

Indeed I have not screened the bundle, since the changes may not be the
ones that we want.

Ganesh: Yes, the qc_prim tests are the problem.

Can you perhaps elaborate what you meant with "just adding the extra
parameter to the existing classes, and ignoring it in some cases"? Are
you referring to the test code or to an alternative solution to the
problem that printing V1 primitive patches depends on the context i.e.
higher level patch type?

The extra newtype layer I added around V1 prim patches indeed makes the
typing even more complicated than it already is. On the other hand,
adapting the actual darcs code wasn't that difficult; the typing here
actually helped.

The test suite is another matter, though. IMO it is based to much on
overloading and type families, which is why it is so hard to get the
typing right. I also find the test code very hard to understand.

This is new version that incorporates the ideas I wrote about on the
mailing list
(https://lists.osuosl.org/pipermail/darcs-devel/2017-September/018219.html).

In particular: PrimPatchRaw and all related functions have been removed.
RepoPatchV1 and V2 take the final (wrapped) prim as type parameter and
no longer need to know anything about the share Prim.V1 type.

This simplified everything considerably! I could easily convert all test
code in less than one day's work. All tests passed the first time I got
the test suite to compile.

The patch is now smaller and (I think) easier to review.

Attachments

• clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch

Just to check - from a quick look, the patch you've attached is dated 
August 31st, still talks about a new layer in the description, and 
introduces a Prim.V2 patch type. Is that the one you intended to 
attach?

This bundle adds another three patches that further simplify the
wrappers and also their use. They no longer take the common base type as
parameter, instead it is baked directly into V1.Prim and V2.Prim.

sorry, forgot to attach the undle, here goes

Attachments

• clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch

Same bundle but with an additional bug fix for the test suite.

Grrr, again forgot to attach the bundle. Here it is.

Attachments

• clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch

Ganesh: It is an amended version of my earlier patch, with the same date
and comment. I merely removed the PrimPatchRaw complication. Take a look
at the latest version here, I think it is no longer too heavy-weight for
what it achieves. Using newtype wrappers to guide instance selection is
a standard technique in modern Haskell. The implementation of the
wrappers themselves is almost trivial (thanks to
GeneralizedNewtypeDeriving). The only really ugly thing is that we must
repeat all the instances in harness/Darcs/Test/Patch/Arbitrary/PrimV1.hs
for both types of Prim.V1 patches.

Sorry for being slow (again). I've had a look through and I agree it 
seems like a reasonable approach - there's much less extra machinery 
needed than before and it's not too hard to follow.

One other thought - with your final design, do we need wrappers for 
both V1 and V2? Could we treat one as a legacy case requiring a wrapper 
and the other as a simpler/more standard one?

> Could we treat one as a legacy case requiring a wrapper 
> and the other as a simpler/more standard one?

I think it would be possible to do that. Indeed, the idea is tempting.
However, this is no small refactoring in itself. It raises a number of
design questions: What to do with PrimRead and PrimShow classes when we
do that? What to do with the FileFormat and PatchListFormat types? I
don't have the head for these questions at the moment, since I have so
many other patches in my queue (some of them not yet in code but only in
my head).

There is nothing stopping anyone from doing another refactor along these
lines after this patch is screened, which is what I am going to do now
(though I may rebase it once more before I do that).

Hopefully the last version. I amended everything into a single patch and
also added some small fixes. Also, added class IsPrimV1 to
Darcs.Repository.Job and constrain PrimV1Job with it in order to support
the way darcsden wants access to prim patch details.

Attachments

• clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch

Finally screening this one, yay!

I went ahead and wrote the patch (the queue of patches in screened is
getting bigger ,we need to process it more). from Ganesh's comments I
think it is safe to accept the bundle. Congratulations \o/

Attachments

• delete-commented-code.dpatch

Oh well, Ignore my patch :D
Ben has pushed his own version to screened before me. Now I'm pushing
them to reviewed.