darcs

Patch 1593 clean up display of file names by separating display and storage of patches (rebased from Ben's encoding branch)

Title clean up display of file names by separating display and storage of patches (rebased from Ben's encoding branch)
Superseder Nosy List gh
Related Issues
Status needs-screening Assigned To
Milestone

Created on 2017-08-31.21:16:57 by gh, last changed 2017-09-02.09:02:39 by bf.

Files
File name Status Uploaded Type Edit Remove
clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch gh, 2017-08-31.21:16:55 application/octet-stream
clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch gh, 2017-08-31.21:39:02 application/octet-stream
See mailing list archives for discussion on individual patches.
Messages
msg19635 (view) Author: gh Date: 2017-08-31.21:16:55
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
msg19636 (view) Author: gh Date: 2017-08-31.21:39:02
Fixing a warning (anyway the test harness code has warnings because of
the commented code that Ben mentionned in his mail).
Attachments
msg19637 (view) Author: ganesh Date: 2017-09-01.06:09:37
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.
msg19638 (view) Author: gh Date: 2017-09-01.12:52:08
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.
msg19641 (view) Author: bf Date: 2017-09-02.09:02:38
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.
History
Date User Action Args
2017-08-31 21:16:57ghcreate
2017-08-31 21:39:02ghsetfiles: + clean-up-display-of-file-names-by-separating-display-and-storage-of-patches.dpatch
messages: + msg19636
2017-09-01 06:09:38ganeshsetmessages: + msg19637
2017-09-01 12:52:09ghsetmessages: + msg19638
2017-09-02 09:02:39bfsetmessages: + msg19641