Hi,

This set of patches makes darcs itself build and run with witnesses
enabled. It involved quite a bit of nasty hackery in the mergers
code (see the comment added by the last patch).

I've tried to make no functional changes, just to fixup the
witness types and use sealing/unsafe operations where necessary.

Unfortunately some bits of the unit test suite also need significant
work to fix witnesses, and I haven't done this yet. Nevertheless
I suggest that this bundle closes issue1288 and we have a new
bug to deal with the test suite.

I don't know if this should go into 2.5 or not. issue1288 is listed
as milestone 2.5, but I guess it won't get reviewed before the code
freeze.

If it does go in, we'd also have to decide whether to enable witnesses
by default.

IMO the most pressing reason to enable witnesses is to make them
accessible to users of the library. This will be a substantial
burden to them, but it'll also prevent/discourage them making a
lot of the nasty mistakes that are possible when handling patches,
such as filtering a list without commutation.

On the flip side, enforcing witnesses makes life harder for most
patch authors who aren't familiar with witnesses, and our
documentation is also very poor. So one argument might be that
we should wait until we have good documentation for witnesses
before turning them on by default.

It also may have a performance impact (as there's more wrapping in
Sealed etc), so we'd need to do at least a bit of benchmarking to
check this.

Note that we could choose to enable them for the library but not
the executable initially - assuming we don't move to having the
executable use the library in the near future.

Cheers,

Ganesh

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

Sun Mar 21 20:33:17 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop GADT_WITNESSES conditionalisation in modernizePatch

Sun Mar 21 20:34:24 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop GADT_WITNESSES conditionalisation for clever_commute

Sun Mar 21 22:17:57 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * push GADT_WITNESSES conditionalisation down into actual_merge definition

Sun Mar 21 22:19:47 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * remove conditionalisation of imports on GADT_WITNESSES for simplicity

Sun Mar 21 23:30:37 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * don't re-export unravel/merger in Darcs.Patch

Sun Mar 21 23:48:17 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * replace 'assert' function with equivalent 'guard' from Control.Monad

Mon Mar 22 00:58:15 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * remove unused instance

Mon Mar 22 01:06:46 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add Darcs.Test.Patch.QuickCheck to witnesses build

Mon Mar 22 01:07:49 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * switch Darcs.Test.Patch.QuickCheck to use Origin type

Mon Mar 22 01:10:08 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop some unused unsafe functions

Tue Mar 23 07:09:46 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop a few unsafeFL uses

Thu Jan  7 18:27:33 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * packStringLetters Show hack needs to use Char8

Thu Jan  7 18:28:41 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * rework Show<n> classes to be more flexible

Tue Mar 30 18:56:39 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop redundant part of testcases

Wed Mar 31 06:05:07 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move Show2 Patch instance to Darcs.Patch.Show

Wed Mar 31 06:09:31 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * use packStringLetters Show hack for RepoModel

Wed Mar 31 06:10:06 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * a couple more MyEq instances

Wed Mar 31 06:12:24 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * export unsafeUnseal etc with GADT_WITNESSES

Wed Mar 31 06:27:06 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * remove some GADT_WITNESSES conditionalisation

Thu Apr  1 06:11:37 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop GADT_WITNESSES conditionalisation of commute_no_merger

Thu Apr  1 06:58:11 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop spurious semicolon

Thu Apr  1 07:16:46 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add type signature for actual_merge'

Thu Apr  1 18:15:33 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop headRL

Thu Apr  1 19:15:00 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move instance to Darcs.Witnesses

Tue Apr 20 07:24:31 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * push witnesses error from public_unravel into unravel
  

Tue Apr 20 07:28:21 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add dummy implementation of merger

Tue Apr 20 07:28:54 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop GADT_WITNESSES conditionalisation in Darcs.Patch.Read

Tue Apr 20 07:29:00 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop GADT_WITNESSES conditionalisation of [other_]commute_recursive_merger

Tue Apr 20 07:29:07 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop GADT_WITNESSES conditionalisation of merger_commute

Tue Apr 20 07:46:01 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop GADT_WITNESSES conditionalisation of unwind

Tue Apr 20 07:46:55 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop GADT_WITNESSES conditionalisation of new_ur

Tue Apr 20 07:47:04 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * drop headFL

Tue Apr 20 07:49:33 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add witnesses to get_supers

Tue Apr 20 18:06:24 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * remove witnesses conditionalisation from unravel

Wed Apr 21 06:51:57 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * rename unsafeUnflippedseal for consistency

Tue Jun 22 07:11:20 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add unsafeCoercePStart/End

Tue Jun 22 07:12:43 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * move unsafeFL definition to its one remaining use site

Tue Jun 29 07:47:27 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add a comment

Mon Jul  5 07:25:14 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * reduce scope of GADT_WITNESSES guard

Mon Jul  5 18:48:07 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * make mapPrimFL work with witnesses

Tue Jul  6 06:26:10 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add witnesses to actualMerge

Tue Jul  6 06:40:16 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * add witnesses to glump09

Tue Jul  6 06:58:29 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * fix witnesses in resolveConflicts

Tue Jul  6 18:40:06 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * remove final bits of witness conditionalisation

Tue Jul  6 20:25:42 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * Resolve issue1288: the main darcs code now compiles and runs with witnesses

Attachments

• drop-gadt_witnesses-conditionalisation-in-modernizepatch.dpatch
• unnamed

Hi,

Ganesh Sittampalam <bugs@darcs.net> writes:
> I don't know if this should go into 2.5 or not. issue1288 is listed
> as milestone 2.5, but I guess it won't get reviewed before the code
> freeze.
I have managed to review 18 patches so far (out of 45). Those that are
reviewed (and tested), I have pushed. I don't really have any comments
for now. I'll try to continue tomorrow, although I expect that the
tricky bits are yet to come.

> On the flip side, enforcing witnesses makes life harder for most
> patch authors who aren't familiar with witnesses, and our
> documentation is also very poor. So one argument might be that
> we should wait until we have good documentation for witnesses
> before turning them on by default.
[snip]

> Note that we could choose to enable them for the library but not
> the executable initially - assuming we don't move to having the
> executable use the library in the near future.
This sounds like a reasonable compromise to me. (I don't know much about
the assumption either, though.)

Yours,
   Petr.

1 patch for repository http://darcs.net:

Tue Jul  6 22:01:06 BST 2010  Ganesh Sittampalam <ganesh@earth.li>
  * GHC 6.10 build fix: don't use GADT records
  
  This could be rolled back once we drop GHC 6.10, but on the other hand the
  code looks ok like this too so it's probably not worth it.

Attachments

• ghc-6_10-build-fix_-don_t-use-gadt-records.dpatch
• unnamed

I've pushed the build fix in the last patch directly (as it's pretty 
trivial and should fix the buildbots).

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-775aba2608d00cec84444569c796690a9c138251

This patch bundle (with 74 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-feabdf2d69abcd993750bdfee984c4974c2df19b

This patch bundle (with 74 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-feabdf2d69abcd993750bdfee984c4974c2df19b

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-775aba2608d00cec84444569c796690a9c138251