This is just for discussion for now.

I wanted to write a small prim type to play with examples of
V3 patches where I could control conflicts directly and could
think about things just as names as we do in theory discussions.
But RepoPatchV3 requires constructing PatchInfos and that's not
exactly lightweight.

This is yet another layer of abstraction, but on the other hand
the changes were surprisingly simple and I think it's good for
our code to make sure that RepoPatchV3 doesn't get too cosy
with NamedPrim in the long-term.

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

patch cc61cc90b02428072c78c07725c8844f8ad04c77
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Aug 16 00:24:39 BST 2019
  * decouple RepoPatchV3 impl from NamedPrim
  
  NamedPrim is in turn tied to PatchInfo, which means it's
  a bit heavyweight to use RepoPatchV3 on top of a simple
  PrimPatch implementation.
  
  NamedPrim is now abstracted behind a type class,
  PrimContainer, and almost all of the RepoPatchV3
  implementation uses a new type RepoPatchV3Core.
  
  RepoPatchV3 itself becomes a type synonym for
  RepoPatchV3Core NamedPrim.

Attachments

• decouple-repopatchv3-impl-from-namedprim.dpatch
• patch-preview.txt
• unnamed

> I wanted to write a small prim type to play with examples of
> V3 patches where I could control conflicts directly and could
> think about things just as names as we do in theory discussions.
> But RepoPatchV3 requires constructing PatchInfos and that's not
> exactly lightweight.

Hm, yes. That sounds reasonable.

> This is yet another layer of abstraction, but on the other hand
> the changes were surprisingly simple and I think it's good for
> our code to make sure that RepoPatchV3 doesn't get too cosy
> with NamedPrim in the long-term.

The idea isn't bad in principle, but I really don't like the renaming of
RepoPatchV3 to RepoPatchV3Core. This is ugly. Instead, I think you
should define the "real" RepoPatchV3 in module Darcs.Patch.V3 and
resolve the name collision by importing Darcs.Patch.V3.Core qualified.

I wonder if PrimContainer should rather be a class of container types,
that is types of kind (* -> * -> *) -> * -> * -> *. This would remove
the need for the PrimContents type family. In practice, even for your
simple prim patch type, you would need to attach a name to them somehow.
This may slightly complicate your simple prim type (because you have to
attach the name with a wrapper) but I think it would be clearer and
improve type inference.

I would also try to find a way to get rid of the superclass constraints
if possible. IME these are almost always a mistake and make things
needlessly inflexible. If we define PrimContainer as a container class
as indicated above, we could factor most of the lifting of prim to
NamedPrim instances generically to Darcs.Patch.Prim.Container. This
would in turn also simplify your simple prim type.

> The idea isn't bad in principle, but I really don't like the renaming of
> RepoPatchV3 to RepoPatchV3Core. This is ugly. Instead, I think you
> should define the "real" RepoPatchV3 in module Darcs.Patch.V3 and
> resolve the name collision by importing Darcs.Patch.V3.Core qualified.

Yes, fair enough. (Comments on any of the other names also welcome).

> I wonder if PrimContainer should rather be a class of container types,
> that is types of kind (* -> * -> *) -> * -> * -> *. This would remove
> the need for the PrimContents type family. In practice, even for your
> simple prim patch type, you would need to attach a name to them somehow.
> This may slightly complicate your simple prim type (because you have to
> attach the name with a wrapper) but I think it would be clearer and
> improve type inference.

I had a bit of a play with it and I can't really make it work. You end
up with a lot of constraints on (c prim) which are a bit awkward, e.g.
SignedId (PatchId (c prim)). In general I think trying to abstract over
container types cleanly is actually quite hard.

It would also add a fair bit of boilerplate/redundancy to my simple prim
type though that's less of an issue if it does make the code in darcs
better.

> I would also try to find a way to get rid of the superclass constraints
> if possible. IME these are almost always a mistake and make things
> needlessly inflexible.

I think they're basically shorthand, we could just inline the  minimal
constraints to the use sites but it makes things quite verbose. I don't
mind doing it if you prefer that.

> needlessly inflexible. If we define PrimContainer as a container class
> as indicated above, we could factor most of the lifting of prim to
> NamedPrim instances generically to Darcs.Patch.Prim.Container. This
> would in turn also simplify your simple prim type.

I don't see how that would work. In general how would you write
something like

instance (PrimContainer c, ShowPatchBasic prim) => ShowPatchBasic (c prim)

?

Here's a version with the suggested renaming.

I did look at moving the code that's just about
RepoPatchV3 (as opposed to Core.RepoPatch.V3) into
Darcs.Patch.V3, but the FromPrim instance needs access
to the Core.RepoPatchV3 constructors.

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

patch d019358560973f2a7c8bdb20740a0289b87b2063
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Aug 16 12:26:43 BST 2019
  * decouple RepoPatchV3 impl from NamedPrim
  
  NamedPrim is in turn tied to PatchInfo, which means it's
  a bit heavyweight to use RepoPatchV3 on top of a simple
  PrimPatch implementation.
  
  NamedPrim is now abstracted behind a type class,
  PrimContainer. Almost all of the implementation in
  Darcs.Patch.V3.Core now uses this abstraction.
  
  The RepoPatchV3 type we expose from Darcs.Patch.V3
  becomes a type synonym for Core.RepoPatchV3 NamedPrim.

Attachments

• decouple-repopatchv3-impl-from-namedprim.dpatch
• patch-preview.txt
• unnamed

Instead of a class, we could define a generic wrapper:

data NamedPatch n p wX wY
  = NamedPatch { name :: !n, patch :: !(p wX wY) }

and use that to do all the heavy lifting. In particular, this type would
implement and encapsulate the requirements for Commute i.e. that neither
p :> p nor p :> p^ must commute and for Eq2, Ident, etc . Some instances
would need a (SignedId n) and we may need an additional method to
construct a SignedId but otherwise this should go through. The only
instances that access the internals of the PrimPatchId are ShowPatch,
ReadPatch, and Commute.

NamedPrim would be reduced to a type synonym for NamedPatch PrimPatchId.

We could even think about refactoring Named using this data type. Most
of the code in Darcs.Repository and Darcs.UI uses PatchInfoAnd anyway,
and not Named, and thus wouldn't even notice this change.

If we do that we should make sure that the components are unboxed.

I've made a start on the 'NamedPatch' idea, and I think it
would go through fairly easily. Regardless of whether
we use it in RepoPatchV3, it's a nice refactoring of
NamedPrim into the things that are specific to PrimPatchId
and the things that aren't, so I would be inclined to keep
it in some form.

I'm less sure about using it explicitly in RepoPatchV3. It
makes the types even longer whereas they actually get
simpler with the PrimContents approach, albeit with more
constraints.

It's really a choice about designing interfaces (in this case
between RepoPatchV3 and the underlying prim): we can have
one with a rigid structure, or a more flexible one expressed
via constraints. The rigid structure is, well, rigid, with
the flexible one the constraints can become overwhelming.

Some notes on the details of the type:

I found calling it NamedPatch too confusing given
Darcs.Patch.Named. We can't refactor that easily because
it also has explicit dependencies in the type. I ended
up calling it Darcs.Patch.Ident.WithIdent since it's
closely associated with Ident. Better ideas welcome.

As you mention, the Commute instance for PrimPatchId
needs to make sure a patch conflicts with itself.
The current implementation with abs obviously needs
abstracting somehow, we could manually write something
like absId i = if positiveId i then i else invertId i.
I also wonder if we should always just fail the commute,
or if some of the cases should be considered internal
errors.

My current WIP uses the RepoPatchV3Core name and also
some pattern synonyms, just to make it easy to make
the changes incrementally. In a final version I'd
rename RepoPatchV3Core and probably also drop the synonyms.

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

patch b50a077c08ea8886ac10ab0e4c6fd89e802375ef
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Fri Aug 16 13:27:35 BST 2019
  * WIP: introduce WithIdent and make NamedPrim a type synonym

Attachments

• patch-preview.txt
• unnamed
• wip_-introduce-withident-and-make-namedprim-a-type-synonym.dpatch

> I've made a start on the 'NamedPatch' idea, and I think it
> would go through fairly easily. Regardless of whether
> we use it in RepoPatchV3, it's a nice refactoring of
> NamedPrim into the things that are specific to PrimPatchId
> and the things that aren't, so I would be inclined to keep
> it in some form.

Good!

> I'm less sure about using it explicitly in RepoPatchV3. It
> makes the types even longer whereas they actually get
> simpler with the PrimContents approach, albeit with more
> constraints.

We could shorten the type variable names: ident -> i, prim -> p. (Using
'ident' is bad anyway because it collides with the 'ident' function
method from class Ident, see below for an alternative.)

> It's really a choice about designing interfaces (in this case
> between RepoPatchV3 and the underlying prim): we can have
> one with a rigid structure, or a more flexible one expressed
> via constraints. The rigid structure is, well, rigid, with
> the flexible one the constraints can become overwhelming.

I generally like to use data types unless there is a convincing reason
not to. Data types are easier to understand and easier for the compiler
to infer. I also tend to find them easier to refactor. They are more
rigid, true, but that isn't a problem, and can even be an advantage, as
long as they fit the problem domain.

In this case, I have a hard time imagining an implementation that does
/not/ use a record type with an extra field or a wrapper that adds such
a field. So unless you come up with a convincing argument for a more
general class based interface, my preference is definitely the wrapper type.

> I found calling it NamedPatch too confusing given
> Darcs.Patch.Named. We can't refactor that easily because
> it also has explicit dependencies in the type.

Perhaps not "easily" in the sense that it is done by changing or adding
a few lines of code, but it shouldn't be /difficult/. I just added a
type parameter to Named to get a feeling. Only 2 classes (ReadPatch,
ShowPatch), and 3 functions (anonymous, infopatch, patchname) need the
parameter to be instantiated to PatchInfo. Next step would then be to

data Named i p wX wY where
    NamedP :: ![i] -- ^ explicit dependencies (should use a Set here!)
           -> !(WithIdent i (FL p) wX wY)
           -> Named i p wX wY

BTW, if I could re-design Darcs from scratch without regard for
compatibility, I wouldn't use PatchInfo as identifier at all! Instead
I'd use the random "salt" directly. (The "hash" would be just this
number, b16 encoded.) Then the whole discussion about whether to use
PatchInfo or its hash in the PrimPatchId would be irrelevant.

> I ended
> up calling it Darcs.Patch.Ident.WithIdent since it's
> closely associated with Ident. Better ideas welcome.

If you think NamedPatch is bad (I agree that it may be confusing) then
WithName may be an alternative. At least type variable 'name' (see
above) doesn't collide with any class method or function, AFAIK.

> As you mention, the Commute instance for PrimPatchId
> needs to make sure a patch conflicts with itself.
> The current implementation with abs obviously needs
> abstracting somehow, we could manually write something
> like absId i = if positiveId i then i else invertId i.

If it can be expressed with the existing primitives from SignedId, we
should do so, otherwise adding one or two primitives wouldn't hurt much.

> I also wonder if we should always just fail the commute,
> or if some of the cases should be considered internal
> errors.

I have wondered, too. I think commuting identical patches is an
indication of something having gone wrong, as it shouldn't be possible
to get into this situation. Commuting p with p^ is okay and should just
fail. Merging a patch with itself is definitely illegal, since the
result would need to be two "null" patches and we don't have that. (It
is okay for sequences of course, they can be empty.) In fact I have a
patch that changes the instance Merge HasIdent to throw an error here;
the merge2FL can then be scrapped and it becomes a precondition for
merge for FLs to not contain any common patches. Works fine.

> My current WIP uses the RepoPatchV3Core name and also
> some pattern synonyms, just to make it easy to make
> the changes incrementally. In a final version I'd
> rename RepoPatchV3Core and probably also drop the synonyms.

I wondered why WithIdent has so few instances.I assume this is because
you just wanted to quickly push out a proof of concept? In the end I
think almost all instances should be defined on the generic wrapper. And
I do think using it for Named patches makes sense and reduces a lot of
boilerplate there.

If for some reason we agree on the class based interface, we should use
DerivingVia so we can at least share the implementations.

>> I'm less sure about using it explicitly in RepoPatchV3. It
>> makes the types even longer whereas they actually get
>> simpler with the PrimContents approach, albeit with more
>> constraints.
> 
> We could shorten the type variable names: ident -> i, prim -> p. (Using
> 'ident' is bad anyway because it collides with the 'ident' function
> method from class Ident, see below for an alternative.)

I was thinking of length in terms of tokens/AST depth. The length of the
names themselves is less of an issue to me and shorter names are often
less helpful. With the class, we're just pushing 'prim' around
everywhere. Without it we're pushing around 'WithName ident prim'.

> In this case, I have a hard time imagining an implementation that does
> /not/ use a record type with an extra field or a wrapper that adds such
> a field. So unless you come up with a convincing argument for a more
> general class based interface, my preference is definitely the wrapper type.

The only example I have is my existing playground code, where I use the
same type for the patch names and contents (so I can have PrimContents
prim = prim). I may also turn that into a test if that seems useful.

Since we'd be using the wrapper anyway, we can always introduce the
class interface later, so it's not a big problem to just use the wrapper
at this point. But I do find having it directly in the V3 code a bit ugly.

> Perhaps not "easily" in the sense that it is done by changing or adding
> a few lines of code, but it shouldn't be /difficult/. I just added a
> type parameter to Named to get a feeling. Only 2 classes (ReadPatch,
> ShowPatch), and 3 functions (anonymous, infopatch, patchname) need the
> parameter to be instantiated to PatchInfo. Next step would then be to
> 
> data Named i p wX wY where
>     NamedP :: ![i] -- ^ explicit dependencies (should use a Set here!)
>            -> !(WithIdent i (FL p) wX wY)
>            -> Named i p wX wY

I don't think this factoring makes sense. The thing that has the name
should also have the explicit dependencies, otherwise you end up with
two different layers looking at the name when commuting. If we were
going to refactor Named I think it should be to factor out an object
that encapsulates both the PatchInfo and the dependencies.

> BTW, if I could re-design Darcs from scratch without regard for
> compatibility, I wouldn't use PatchInfo as identifier at all! Instead
> I'd use the random "salt" directly. (The "hash" would be just this
> number, b16 encoded.) Then the whole discussion about whether to use
> PatchInfo or its hash in the PrimPatchId would be irrelevant.

Where would the actual commit metadata go? I think there are also cases
where you don't want a salt, e.g. importing commits from another VCS
where you'd like to get the same result if you do it again.

>> I ended
>> up calling it Darcs.Patch.Ident.WithIdent since it's
>> closely associated with Ident. Better ideas welcome.
> 
> If you think NamedPatch is bad (I agree that it may be confusing) then
> WithName may be an alternative. At least type variable 'name' (see
> above) doesn't collide with any class method or function, AFAIK.

WithName is fine (though type variables don't live in the same namespace
as values, do they? So this is just about readability.)

Do we want Darcs.Patch.WithName or Darcs.Patch.Ident.WithName?

>> I also wonder if we should always just fail the commute,
>> or if some of the cases should be considered internal
>> errors.
> 
> I have wondered, too. I think commuting identical patches is an
> indication of something having gone wrong, as it shouldn't be possible
> to get into this situation. Commuting p with p^ is okay and should just
> fail. Merging a patch with itself is definitely illegal, since the
> result would need to be two "null" patches and we don't have that. (It
> is okay for sequences of course, they can be empty.)

so

p;p -> error
p;p^ -> fail
p^;p -> fail? I guess it needs to because of inverse commute.
p^;p^ -> error

>> My current WIP uses the RepoPatchV3Core name and also
>> some pattern synonyms, just to make it easy to make
>> the changes incrementally. In a final version I'd
>> rename RepoPatchV3Core and probably also drop the synonyms.
> 
> I wondered why WithIdent has so few instances.I assume this is because
> you just wanted to quickly push out a proof of concept? In the end I
> think almost all instances should be defined on the generic wrapper.

Yes.

> If for some reason we agree on the class based interface, we should use
> DerivingVia so we can at least share the implementations.

Regardless of whether we use the class interface or the wrapper, we'd
still have just one wrapper, so I don't think this is an issue.

updated WIP on WithName

Still need to sort out Commute (in discussion), and
define an abstraction for Show/Read on names (shouldn't be hard)

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

patch 5829805166b63114f464f0fd5070a254f3e83006
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Aug 17 15:52:24 BST 2019
  * WIP: introduce WithName and make NamedPrim a type synonym

Attachments

• patch-preview.txt
• unnamed
• wip_-introduce-withname-and-make-namedprim-a-type-synonym.dpatch

> With the class, we're just pushing 'prim' around
> everywhere. Without it we're pushing around 'WithName ident prim'.

Before this change we had NamedPrim prim, so this is just one token
more. And what do you mean "everywhere"? NamedPrim appears only in the
data type definition (and the pattern synonyms you added); and in a
handfull of type signatures in the conflict resolution code. This is
where WithName would appear, right?

>> In this case, I have a hard time imagining an implementation that does
>> /not/ use a record type with an extra field or a wrapper that adds such
>> a field. So unless you come up with a convincing argument for a more
>> general class based interface, my preference is definitely the wrapper type.
> 
> The only example I have is my existing playground code, where I use the
> same type for the patch names and contents (so I can have PrimContents
> prim = prim). I may also turn that into a test if that seems useful.

Oh, I didn't know that. I had wondered about the doc comment for the
class where you said something like that. An interesting idea, but I
can't imagine how that works... e.g. how can you guarantee uniqueness of
names if they are equal to the contents? I guess you also cannot
implement things like splitting or coalescing, but for your purpose that
may be allright.

Can you put this code online somewhere? It doesn't have to work or even
compile, just for reading.

> Since we'd be using the wrapper anyway, we can always introduce the
> class interface later, so it's not a big problem to just use the wrapper
> at this point. But I do find having it directly in the V3 code a bit ugly.

Well. Compared to the NamedPrim wrapper it isn't /such/ a big
difference. But I agree it makes a difference when compared to the class
thing. This affects perhaps a handfull of type signatures, apart from
the data type definitions.

A problem with class PrimContainer is that it doesn't give us any useful
structure. I see no class laws attached to it and I think this is not an
oversight on your part, but lies in the nature of such classes. I have
the very strong feeling that using type classes for things like that is
just not right. I would rather search for a possibility to find a common
data type that fits both NamePrim patches and your playground prim type.
(I would really like to take a look at that thing!)

The number of superclasses is an indication that the class mixes things
that don't belong together. PrimContainer should not need any
superclasses at all. It has only one method and one associated type
family, a simple thing, really. And (repeating myself) only three
instances need its features. The superclasses are, as you say, for
convenience. I think you should perhaps add this convenience by adding a
separate constraint synonym that can be used in place of PrimPatch.
Perhaps something as simple as

type NamedPrimPatch prim = (PrimPatch prim, PrimContainer prim)

would suffice. But if you want a smaller constraint (for precision; and
so you don't have to define useless or bogus instances), you could also
list the classes explicitly.

>> Perhaps not "easily" in the sense that it is done by changing or adding
>> a few lines of code, but it shouldn't be /difficult/. I just added a
>> type parameter to Named to get a feeling. Only 2 classes (ReadPatch,
>> ShowPatch), and 3 functions (anonymous, infopatch, patchname) need the
>> parameter to be instantiated to PatchInfo. Next step would then be to
>>
>> data Named i p wX wY where
>>     NamedP :: ![i] -- ^ explicit dependencies (should use a Set here!)
>>            -> !(WithIdent i (FL p) wX wY)
>>            -> Named i p wX wY
> 
> I don't think this factoring makes sense. The thing that has the name
> should also have the explicit dependencies, otherwise you end up with
> two different layers looking at the name when commuting.

Good point, I hadn't thought about that.

> If we were
> going to refactor Named I think it should be to factor out an object
> that encapsulates both the PatchInfo and the dependencies.

I don't see anything to gain from that.

>> BTW, if I could re-design Darcs from scratch without regard for
>> compatibility, I wouldn't use PatchInfo as identifier at all! Instead
>> I'd use the random "salt" directly. (The "hash" would be just this
>> number, b16 encoded.) Then the whole discussion about whether to use
>> PatchInfo or its hash in the PrimPatchId would be irrelevant.
> 
> Where would the actual commit metadata go?

Huh? They remain where they are? I didn't mean we need no meta data. I
just said I wouldn't use them as names i.e. unique identifiers.

I know that using random numbers is not as convincing or reliable as the
hash of a canonical representation. But since we currently have no
canonical representation that can be efficiently calculated, a good
cryptographically sound random number is the next best thing.

> I think there are also cases
> where you don't want a salt, e.g. importing commits from another VCS
> where you'd like to get the same result if you do it again.

That might be convenient, but it would be a very bad idea, inviting any
amount of trouble later. Why not instead use something akin to a marks
file? When we import, we generate a file that associates our patch
identifiers with the pair of commit hashes (start state, end state). You
could publish that file so other can read it before starting to import,
then associate the same identifier to the diff between known states.

BTW, a similar approach could be useful when converting between darcs
formats, at least for any patches that we can't translate one-to-one.

>>> I ended
>>> up calling it Darcs.Patch.Ident.WithIdent since it's
>>> closely associated with Ident. Better ideas welcome.
>>
>> If you think NamedPatch is bad (I agree that it may be confusing) then
>> WithName may be an alternative. At least type variable 'name' (see
>> above) doesn't collide with any class method or function, AFAIK.
> 
> WithName is fine (though type variables don't live in the same namespace
> as values, do they? So this is just about readability.)

Yes, and grep-ability ;-) I use grep quite a lot. Call me old-fashioned
but I never got warm with any of the alternatives.

> Do we want Darcs.Patch.WithName or Darcs.Patch.Ident.WithName?

My choice would be Darcs.Patch.Prim.WithName and PrimWithName for the
type. That's because it will contain instances for classes from
Darcs.Patch.Prim.Class. And we have no plans to use it for anything
other than prims.

BTW, my personal opinion is that having both modules X and X.Y should be
restricted to the case where X is the official interface for accessing
stuff from X.Y. So Darcs.Patch.Prim and its sub modules is good. But
Darcs.Util.Printer and Darcs.Util.Printer.Color is bad.

>>> I also wonder if we should always just fail the commute,
>>> or if some of the cases should be considered internal
>>> errors.
>>
>> I have wondered, too. I think commuting identical patches is an
>> indication of something having gone wrong, as it shouldn't be possible
>> to get into this situation. Commuting p with p^ is okay and should just
>> fail. Merging a patch with itself is definitely illegal, since the
>> result would need to be two "null" patches and we don't have that. (It
>> is okay for sequences of course, they can be empty.)
> 
> so
> 
> p;p -> error
> p;p^ -> fail
> p^;p -> fail? I guess it needs to because of inverse commute.
> p^;p^ -> error

Yes, though I think we only need to distinguish 2 cases here.

>> With the class, we're just pushing 'prim' around
>> everywhere. Without it we're pushing around 'WithName ident prim'.
> 
> Before this change we had NamedPrim prim, so this is just one token
> more. And what do you mean "everywhere"? NamedPrim appears only in the
> data type definition (and the pattern synonyms you added); and in a
> handfull of type signatures in the conflict resolution code. This is
> where WithName would appear, right?

And an extra type variable in all the instances, though that's less
distracting. But I liked the simplification in the signatures when I got
rid of NamedPrim with the class, so I dislike the even longer signatures
with WithName more. Since we definitely want to replace NamedPrim
regardless, the relevant comparison is between WithName ident prim and prim.

But it's not that big a deal. We have plenty of much more cluttered
code, and it usually takes me a little while to let go of my attachment
to nice code I just wrote :-)

>>> In this case, I have a hard time imagining an implementation that does
>>> /not/ use a record type with an extra field or a wrapper that adds such
>>> a field. So unless you come up with a convincing argument for a more
>>> general class based interface, my preference is definitely the wrapper type.
>>
>> The only example I have is my existing playground code, where I use the
>> same type for the patch names and contents (so I can have PrimContents
>> prim = prim). I may also turn that into a test if that seems useful.
> 
> Oh, I didn't know that. I had wondered about the doc comment for the
> class where you said something like that. An interesting idea, but I
> can't imagine how that works... e.g. how can you guarantee uniqueness of
> names if they are equal to the contents? I guess you also cannot
> implement things like splitting or coalescing, but for your purpose that
> may be allright.

Because the contents are just abstract names and I never reuse them. The
idea is to be able to type in examples in a similar way to the language
we use when talking about the algorithms.

> Can you put this code online somewhere? It doesn't have to work or even
> compile, just for reading.

Attached. (If you do want to run it just do things like "printWithMin
rSTUV" in cabal v2-repl. It's the example from the camp paper showing
why we have to look at the conflicting set when commuting. The things in
angle brackets are contexts, on the left-hand side of the | are the live
patches and on the right-hand side are the reverted ones.)

I expect if I did need to use the wrapper I could hide the boilerplate
in the utility functions, so it'd just be a bit of extra clutter but not
too annoying.

> A problem with class PrimContainer is that it doesn't give us any useful
> structure. I see no class laws attached to it and I think this is not an
> oversight on your part, but lies in the nature of such classes. I have
> the very strong feeling that using type classes for things like that is
> just not right. I would rather search for a possibility to find a common
> data type that fits both NamePrim patches and your playground prim type.
> (I would really like to take a look at that thing!)

I don't think a lack of laws is a fundamental problem with a class. Some
classes capture structure that needs laws for it to make sense, others
capture structure in a way that doesn't need external laws. Classes are
ultimately just a way of parameterizing things.

That said, in the case of PrimContainer perhaps we should have some laws
relating the instances for p and for PrimContainer p.

> The number of superclasses is an indication that the class mixes things
> that don't belong together. PrimContainer should not need any
> superclasses at all. It has only one method and one associated type
> family, a simple thing, really. And (repeating myself) only three
> instances need its features. The superclasses are, as you say, for
> convenience. I think you should perhaps add this convenience by adding a
> separate constraint synonym that can be used in place of PrimPatch.
> Perhaps something as simple as
> 
> type NamedPrimPatch prim = (PrimPatch prim, PrimContainer prim)
> 
> would suffice. But if you want a smaller constraint (for precision; and
> so you don't have to define useless or bogus instances), you could also
> list the classes explicitly.

I don't mind factoring it into a simpler class + constraint synonym for
the other things we want in practice.

>> If we were
>> going to refactor Named I think it should be to factor out an object
>> that encapsulates both the PatchInfo and the dependencies.
> 
> I don't see anything to gain from that.

I think it would be quite nice to have a separate type that encapsulates
the semantics of explicit dependencies that we can reason about
separately to the patch contents.

> Huh? They remain where they are? I didn't mean we need no meta data. I
> just said I wouldn't use them as names i.e. unique identifiers.
> 
> I know that using random numbers is not as convincing or reliable as the
> hash of a canonical representation. But since we currently have no
> canonical representation that can be efficiently calculated, a good
> cryptographically sound random number is the next best thing.

Ah, right.


>> I think there are also cases
>> where you don't want a salt, e.g. importing commits from another VCS
>> where you'd like to get the same result if you do it again.
> 
> That might be convenient, but it would be a very bad idea, inviting any
> amount of trouble later. Why not instead use something akin to a marks
> file? When we import, we generate a file that associates our patch
> identifiers with the pair of commit hashes (start state, end state). You
> could publish that file so other can read it before starting to import,
> then associate the same identifier to the diff between known states.

I agree it's potentially a bit dangerous, but it means you can convert
multiple branches of the same codebase in isolation and keep
compatibility. Anyway, it's all fairly hypothetical.

>> WithName is fine (though type variables don't live in the same namespace
>> as values, do they? So this is just about readability.)
> 
> Yes, and grep-ability ;-) I use grep quite a lot. Call me old-fashioned
> but I never got warm with any of the alternatives.

Yeah, that's the main thing I use too. But I can't see myself grepping
for 'ident'.

>> Do we want Darcs.Patch.WithName or Darcs.Patch.Ident.WithName?
> 
> My choice would be Darcs.Patch.Prim.WithName and PrimWithName for the
> type. That's because it will contain instances for classes from
> Darcs.Patch.Prim.Class. And we have no plans to use it for anything
> other than prims.

OK.

> BTW, my personal opinion is that having both modules X and X.Y should be
> restricted to the case where X is the official interface for accessing
> stuff from X.Y. So Darcs.Patch.Prim and its sub modules is good. But
> Darcs.Util.Printer and Darcs.Util.Printer.Color is bad.

Seems reasonable.

>> p;p -> error
>> p;p^ -> fail
>> p^;p -> fail? I guess it needs to because of inverse commute.
>> p^;p^ -> error
> 
> Yes, though I think we only need to distinguish 2 cases here.

In the code you mean? i.e. the logic is simply "equal identities =>
error", "inverted identities => fail".

Attachments

• example.dpatch

This should be getting closer to the final form. I still
need to sort out the tests on my machine so I can actually
check this doesn't break anything.

I'll make the class-based approach sit on top of this so
we can decide whether or not to have it independently.

2 patches for repository darcs-unstable@darcs.net:screened:

patch bb4fb968172a76524a61f9661e77175aeac49c45
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Aug 18 20:19:53 BST 2019
  * commuting identical NamedPrims is an internal error
  
  This also refactors the implementation so it just
  relies on the Ident class instead of the internals.

patch 3183d11b363020e4df7f1c264bbe3983808a8917
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Aug 18 23:05:28 BST 2019
  * introduce PrimWithName and make NamedPrim a type synonym

Attachments

• commuting-identical-namedprims-is-an-internal-error.dpatch
• patch-preview.txt
• unnamed

>>> With the class, we're just pushing 'prim' around
>>> everywhere. Without it we're pushing around 'WithName ident prim'.
>>
>> Before this change we had NamedPrim prim, so this is just one token
>> more. And what do you mean "everywhere"? NamedPrim appears only in the
>> data type definition (and the pattern synonyms you added); and in a
>> handfull of type signatures in the conflict resolution code. This is
>> where WithName would appear, right?
> 
> And an extra type variable in all the instances, though that's less
> distracting.

You are right, I forgot those.

> But I liked the simplification in the signatures when I got
> rid of NamedPrim with the class, so I dislike the even longer signatures
> with WithName more. Since we definitely want to replace NamedPrim
> regardless, the relevant comparison is between WithName ident prim and prim.

Okay.

> But it's not that big a deal. We have plenty of much more cluttered
> code, and it usually takes me a little while to let go of my attachment
> to nice code I just wrote :-)

I am not rejecting your class proposal out of hand. Your arguments are
valid, as I think are mine. It's a matter of priorities. Assuming my
proposal for refactoring your simple prim patch type below works out,
what remains is a win in readability in RepoPatchV3 versus potential
complications due to the type family (i.e. extra type signatures needed
to avoid ambiguity). Whenever I am stuck between pro/con arguments like
that I prefer to err on the conservative side.

I think I would have to see how RepoPatchV3 would look like /without/
the class but with all the other refactorings we agreed on to properly
judge if the simplifications offered by the class are worth it.

>>>> In this case, I have a hard time imagining an implementation that does
>>>> /not/ use a record type with an extra field or a wrapper that adds such
>>>> a field. So unless you come up with a convincing argument for a more
>>>> general class based interface, my preference is definitely the wrapper type.
>>>
>>> The only example I have is my existing playground code, where I use the
>>> same type for the patch names and contents (so I can have PrimContents
>>> prim = prim). I may also turn that into a test if that seems useful.
>>
>> Oh, I didn't know that. I had wondered about the doc comment for the
>> class where you said something like that. An interesting idea, but I
>> can't imagine how that works... e.g. how can you guarantee uniqueness of
>> names if they are equal to the contents? I guess you also cannot
>> implement things like splitting or coalescing, but for your purpose that
>> may be allright.
> 
> Because the contents are just abstract names and I never reuse them. The
> idea is to be able to type in examples in a similar way to the language
> we use when talking about the algorithms.

Okay, in this case I would use something like

  data Unit wX wY = Unit

as the "base" prim patch type with only trivial instances. Youŕ named
prim would then be

  type NamedUnit = WithName <your prim patch data> Unit

Looks perhaps a bit strange but shouldn't cause you to write any
additional code, just refactor a bit. Assuming the instances for
WithName are all in place anyway, your code may even become simpler.

>> Can you put this code online somewhere? It doesn't have to work or even
>> compile, just for reading.
> 
> Attached. (If you do want to run it just do things like "printWithMin
> rSTUV" in cabal v2-repl. It's the example from the camp paper showing
> why we have to look at the conflicting set when commuting. The things in
> angle brackets are contexts, on the left-hand side of the | are the live
> patches and on the right-hand side are the reverted ones.)

Nice, I'll take a look later.

>>> If we were
>>> going to refactor Named I think it should be to factor out an object
>>> that encapsulates both the PatchInfo and the dependencies.
>>
>> I don't see anything to gain from that.
> 
> I think it would be quite nice to have a separate type that encapsulates
> the semantics of explicit dependencies that we can reason about
> separately to the patch contents.

Hmm. True. But at some point we must deal with combining the two
semantics into a coherent whole, right?. I guess this is something we
should try out and then see if it actually makes it easier to understand
or reason about.

>>> I think there are also cases
>>> where you don't want a salt, e.g. importing commits from another VCS
>>> where you'd like to get the same result if you do it again.
>>
>> That might be convenient, but it would be a very bad idea, inviting any
>> amount of trouble later. Why not instead use something akin to a marks
>> file? When we import, we generate a file that associates our patch
>> identifiers with the pair of commit hashes (start state, end state). You
>> could publish that file so other can read it before starting to import,
>> then associate the same identifier to the diff between known states.
> 
> I agree it's potentially a bit dangerous, but it means you can convert
> multiple branches of the same codebase in isolation and keep
> compatibility. Anyway, it's all fairly hypothetical.

On the contrary, the question is highly relevant, since there is still
the open problem of designing a useful 'darcs convert --darcs-3'
command. Where with "useful" I mean something that does /not/ require
users to first pull all their branches into one repo, then convert, and
then pull them apart again, like we required for darcs-1 -> darcs-2
conversion.

>>> WithName is fine (though type variables don't live in the same namespace
>>> as values, do they? So this is just about readability.)
>>
>> Yes, and grep-ability ;-) I use grep quite a lot. Call me old-fashioned
>> but I never got warm with any of the alternatives.
> 
> Yeah, that's the main thing I use too. But I can't see myself grepping
> for 'ident'.

Why not?

>>> [...commuting named prims...]
>>> p;p -> error
>>> p;p^ -> fail
>>> p^;p -> fail? I guess it needs to because of inverse commute.
>>> p^;p^ -> error
>>
>> Yes, though I think we only need to distinguish 2 cases here.
> 
> In the code you mean?

And the spec.

> i.e. the logic is simply "equal identities =>
> error", "inverted identities => fail".

Exactly.

> This should be getting closer to the final form. I still
> need to sort out the tests on my machine so I can actually
> check this doesn't break anything.

Perhaps pulling my latest test/harness patches makes it a bit easier.

> I'll make the class-based approach sit on top of this so
> we can decide whether or not to have it independently.

Yes, let us separate that out and mark as in-discussion, for the moment.

Updated version of the patches that have broad agreement.
I'm going to screen this now so I don't have to keep
rebasing.

It turns out that making commuting identical prims fail
causes unit test errors, so I've left that out for now.

I'll followup separately on
(a) making the commute of identical prims fail
(b) switching to the class-based interface

2 patches for repository darcs-unstable@darcs.net:screened:

patch b30e3fccbe39ab771e551d6d516683ddca738b7e
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Aug 27 12:36:20 BST 2019
  * Refactor the commute implementation for NamedPrims
  
  It now just relies on the Ident class instead of the internals.
  This also distinguishes a case that ought to be an internal error,
  but the unit tests seem to rely on it, so this is left as a
  TODO for now.

patch 403b46b4a02845a8cd2d3b63eef285d734195245
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Aug 27 12:44:48 BST 2019
  * introduce PrimWithName and make NamedPrim a type synonym

Attachments

• patch-preview.txt
• refactor-the-commute-implementation-for-namedprims.dpatch
• unnamed

>   This also distinguishes a case that ought to be an internal error,
>   but the unit tests seem to rely on it, so this is left as a
>   TODO for now.

I think this is because Darcs.Test.Patch.Arbitrary.NamedPrim.aPatchId
does not generate unique IDs. We cannot guarantee uniqueness because the
Gen monad from QuickCheck doesn't allow us to thread a (user defined)
state through the actions. There is
http://hackage.haskell.org/package/QuickCheck-GenT but to use that we'd
have to switch to GenT everywhere in the harness.

For RepoPatchV3 I have used the fromAnonymousPrim hack to generate
random PrimPatchIds. These have a very low probability of colliding.

> patch 403b46b4a02845a8cd2d3b63eef285d734195245
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Tue Aug 27 12:44:48 BST 2019
>   * introduce PrimWithName and make NamedPrim a type synonym

I would like to request a follow-up that removes the import of NamedPrim
from Darcs.Patch.V3.Core, so that it really becomes independent of
NamedPrim. The instance FromPrim (RepoPatchV3 prim) can go into
Darcs.Patch.V3 and I guess we can generalize the pattern synonyms from
NamedPrim to 'PrimWithName name'.

I'll leave this to you since you introduced the pattern synonyms and I
am pretty unfamiliar with them.

> I would like to request a follow-up that removes the import of NamedPrim
> from Darcs.Patch.V3.Core, so that it really becomes independent of
> NamedPrim. The instance FromPrim (RepoPatchV3 prim) can go into
> Darcs.Patch.V3

I would have done that, but it would require a constructor of
Core.RepoPatchV3 to be exported which we've been trying to avoid. Though
looking at it again, it's just construction with Prim so I guess we
could just export a function to do it.

> and I guess we can generalize the pattern synonyms from
> NamedPrim to 'PrimWithName name'.

Yes, that would be simple, I just didn't bother due to FromPrim.

> I think this is because Darcs.Test.Patch.Arbitrary.NamedPrim.aPatchId
> does not generate unique IDs.
[...]
> For RepoPatchV3 I have used the fromAnonymousPrim hack to generate
> random PrimPatchIds. These have a very low probability of colliding.

I see, thanks. Doesn't that mean we should be able to do the same thing
for aPatchId, i.e. use unsafePerformIO + the same PrimPatchId generator?
But I tried and it still fails - see attached - but maybe I got the
details wrong somehow.

Attachments

• wip_-identical-commutes-are-an-internal-error.dpatch

>> I think this is because Darcs.Test.Patch.Arbitrary.NamedPrim.aPatchId
>> does not generate unique IDs.
> [...]
>> For RepoPatchV3 I have used the fromAnonymousPrim hack to generate
>> random PrimPatchIds. These have a very low probability of colliding.
> 
> I see, thanks. Doesn't that mean we should be able to do the same thing
> for aPatchId, i.e. use unsafePerformIO + the same PrimPatchId generator?
> But I tried and it still fails - see attached - but maybe I got the
> details wrong somehow.

unsafePerformIO generally makes it pretty hard to predict what's going
to happen. I had a bad feeling about calling it twice, so I tried to use
anonymousNamedPrim directly in Darcs.Test.Patch.Arbitrary.PrimV3FileUUID
and Darcs.Test.Patch.Arbitrary.PrimV3V1 and that seems to do it.

I'll send a patch. I've also got a hunch that we can generalize the
instances in these two modules to (at least) NamedPrim, perhaps even
WithNamedPrim, so we can scrap these two modules.

BTW. I am currently using the latest screened with your latest patches
we discussed and some of mine, and I just noticed that darcs seems to
have gotten /considerably/ slower than it used to be, for almost every
command.

This is what I have now on top of screened:

ben@juliana[1]:.../darcs/sent>darcs send --dry-run
Creating patch to "http://darcs.net/screened"...
Patch bundle would  be sent to: patches@darcs.net
Would send the following changes:
patch 88b3d7ced3ddbb81565ce039a33a410dc1c46f38
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 15:03:14 CEST 2019
  * rename actually_get_log to get_log_using_editor

patch 238ffe4bb9aa170249bc359c64d81355ff5b8a82
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 24 14:01:09 CEST 2019
  * fix prompting when we get a bad patch name

  If the user gives a patch name on the command line and that turns out
to be
  bad, we want darcs to fail and not prompt the user. Think of using 'darcs
  record' in a script...
  Also, if we do prompt for a patch name and get a bad name, we should
explain
  why it is bad before prompting again.
  Finally, when we get a bad name from reading a log file or from
launching an
  editor, we also want to fail, rather than prompting the user.

patch 2343f0cab3c9f7a13702d83fc465eb49bde2c7ba
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Aug 18 16:34:26 CEST 2019
  * remove unused functions from D.P.W.Sealed

patch ff075a27c24e92768b0402d95841df9b366f9705
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Aug 27 14:11:42 CEST 2019
  * get rid of unsafe unsealing

  An unsafe unseal is really just an unsafe coerce of
  an existentially quantified witness, so make that explicit
  and reduce the number of kinds of "unsafe" operations
  we have to understand.

  The laziness property of 'seal' doesn't seem to be actually
  needed by any test and is unlikely to have any performance
  impact.


patch cf337f231cff6f3f0b194643becdf6a6086a8131
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Aug 27 18:33:49 CEST 2019
  * WIP: identical commutes are an internal error

patch b5c70c85e56f25190016c33e9e4fe3aab8fbca69
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 27 21:45:08 CEST 2019
  * remove unused dependency of darcs-test on split package

patch 41106e6f1995a8ffa7475f701faa78fb746aeca0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 27 21:45:45 CEST 2019
  * harness: don't generate arbitrary PrimPatchIds, instead use
anonymousNamedPrim

  This ensures (or rather: increases the probability to near certainty)
that we
  never generate arbitrary prim patches with colliding PrimPatchIds.

Making no changes: this is a dry run.

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

patch cf337f231cff6f3f0b194643becdf6a6086a8131
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Tue Aug 27 18:33:49 CEST 2019
  * WIP: identical commutes are an internal error

patch 41106e6f1995a8ffa7475f701faa78fb746aeca0
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Aug 27 21:45:45 CEST 2019
  * harness: don't generate arbitrary PrimPatchIds, instead use anonymousNamedPrim
  
  This ensures (or rather: increases the probability to near certainty) that we
  never generate arbitrary prim patches with colliding PrimPatchIds.

Attachments

• patch-preview.txt
• unnamed
• wip_-identical-commutes-are-an-internal-error.dpatch

> BTW. I am currently using the latest screened with your latest patches
> we discussed and some of mine, and I just noticed that darcs seems to
> have gotten /considerably/ slower than it used to be, for almost every
> command.
> 
> This is what I have now on top of screened:
> 
> ben@juliana[1]:.../darcs/sent>darcs send --dry-run
[...]
> patch ff075a27c24e92768b0402d95841df9b366f9705
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Tue Aug 27 14:11:42 CEST 2019
>   * get rid of unsafe unsealing
> 
>   An unsafe unseal is really just an unsafe coerce of
>   an existentially quantified witness, so make that explicit
>   and reduce the number of kinds of "unsafe" operations
>   we have to understand.
> 
>   The laziness property of 'seal' doesn't seem to be actually
>   needed by any test and is unlikely to have any performance
>   impact.

Back to normal after I obliterated this one and rebuilt. I should have
known, of course. Still, interesting to observe how much of a difference
this makes in practice.

On 27/08/2019 21:16, Ben Franksen wrote:

>>   The laziness property of 'seal' doesn't seem to be actually
>>   needed by any test and is unlikely to have any performance
>>   impact.
> 
> Back to normal after I obliterated this one and rebuilt. I should have
> known, of course. Still, interesting to observe how much of a difference
> this makes in practice.

OK, seems this one needs to be handled with extreme care. I shall do so.
It's still horrible though :-)

>>>   The laziness property of 'seal' doesn't seem to be actually
>>>   needed by any test and is unlikely to have any performance
>>>   impact.
>>
>> Back to normal after I obliterated this one and rebuilt. I should have
>> known, of course. Still, interesting to observe how much of a difference
>> this makes in practice.
> 
> OK, seems this one needs to be handled with extreme care. I shall do so.
> It's still horrible though :-)

It is. On the other hand, the way we use unsafeInterleaveIO for reading
hashed files is safe, and it saves us from a lot of hassle we'd
otherwise have, to make sure we don't read more from disk (or even
download) than necessary. I am almost sure that streaming libraries a la
conduit aren't up to the task, given that we want to read both
inventories and patches on demand and that everything we produce is
witness typed.

An (as yet unsent) patch in my Repository refactors reduces the pattern
to two occurrences, defines a local function make_lazy for it, and also
documents why we do this.

I am quite glad we have tests that fail if we make a mistake here.
tests/issue2293-laziness-amend.sh is particularly drastic: the repo it
unpacks misses the parent inventory of _darcs/hashed_inventory so we run
into an error as soon as we try to read more than the top-level
inventory (which has only the tag and a single patch).

Perhaps the "unseal seal <$>" could be avoided if we are willing to
incur some major upheaval. I believe the reason we need this are the
pattern matches on the Sealed constructor. These ultimately stem from
the type of readPatch, which returns a Sealed p. I think we could change
that to return the un-Sealed patch with two witnesses (so the caller
gets to choose them at will). I may play with this idea.

> patch cf337f231cff6f3f0b194643becdf6a6086a8131
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Tue Aug 27 18:33:49 CEST 2019
>   * WIP: identical commutes are an internal error
> 
> patch 41106e6f1995a8ffa7475f701faa78fb746aeca0
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Tue Aug 27 21:45:45 CEST 2019
>   * harness: don't generate arbitrary PrimPatchIds, instead use anonymousNamedPrim
>   
>   This ensures (or rather: increases the probability to near certainty) that we
>   never generate arbitrary prim patches with colliding PrimPatchIds.

Feel free to amend/rebase/highjack this patch. (I assume your WIP patch
still needs some polishing and mine depends on it.)

> Perhaps the "unseal seal <$>" could be avoided if we are willing to
> incur some major upheaval. I believe the reason we need this are the
> pattern matches on the Sealed constructor. These ultimately stem from
> the type of readPatch, which returns a Sealed p. I think we could change
> that to return the un-Sealed patch with two witnesses (so the caller
> gets to choose them at will). I may play with this idea.

This failed miserably. We need to seal the result because the NilFL
constructor requires wX ~ wY. So we cannot let the caller choose /both/
witnesses at will. At least one of them must be chosen by the callee
i.e. existentially quantified. (I remember trying this before, long ago,
and coming to the same conclusion.)

OK, so that wasn't too ugly.

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

patch 587d15fd349e13c83f49e70e7e004f03d4218463
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Wed Aug 28 00:01:56 BST 2019
  * Make D.P.V3.Core independent of NamedPrim/PrimPatchId

Attachments

• make-d_p_v3_core-independent-of-namedprim_primpatchid.dpatch
• patch-preview.txt
• unnamed

On 27/08/2019 23:58, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>> Perhaps the "unseal seal <$>" could be avoided if we are willing to
>> incur some major upheaval. I believe the reason we need this are the
>> pattern matches on the Sealed constructor. These ultimately stem from
>> the type of readPatch, which returns a Sealed p. I think we could change
>> that to return the un-Sealed patch with two witnesses (so the caller
>> gets to choose them at will). I may play with this idea.
> 
> This failed miserably. We need to seal the result because the NilFL
> constructor requires wX ~ wY. So we cannot let the caller choose /both/
> witnesses at will. At least one of them must be chosen by the callee
> i.e. existentially quantified. (I remember trying this before, long ago,
> and coming to the same conclusion.)

Thanks, I finally get it :-) I now have a feeling David or Ian may have
explained this to me ~15 years ago, but I forgot..

Operationally, a pattern-match on Sealed is necessarily strict because
it's a GADT, and that implies evaluating the scrutinee far enough to
discover *which* Sealed invocation in the code was used. Sensible code
style and things like NilFL, mean that readPatch is implemented like this:

  if blah then Sealed (Hunk ...) else Sealed (Replace ...)

rather than like this:

  Sealed (if blah then Hunk ... else Replace ...)

so we have to evaluate 'blah' when we pattern-match on Sealed.

If instead of

  case v of Sealed p -> f v

we write

  unseal f v

this is equivalent to

  f (unsafeUnseal v) = f (case v of Sealed x -> unsafeCoercePEnd x)

and now f can be evaluated lazily and the Sealed will only be
scrutinised if f scrutinises its argument, which in an ideal world would
only happen if it genuinely needed to distinguish between Hunk and
Replace etc.

The unsafeCoercePEnd is needed because otherwise the type checker would
be rightly unhappy at leaking existentials. [In the actual code the
types are more general so it's actually unsafeCoerceP1 but the principle
is the same.]

More generally, when you bring an existential type variable into the
scope by unpacking its constructor, you are forcing evaluation far
enough to discover where in the code that existential was originally packed.

Anyway, I should modify my opinion about this being horrible. What's
really nasty is not the unsafe operation, which does seem unavoidable,
but the fact that it's not clear in the API that unpacking Sealed and
calling seal are not equivalent. Given that, introducing the make_lazy
you mentioned, and making seal strict, seems perfect, as it would mean
places where we genuinely rely on it are clearly documented.

> patch 587d15fd349e13c83f49e70e7e004f03d4218463
> Author: Ganesh Sittampalam <ganesh@earth.li>
> Date:   Wed Aug 28 00:01:56 BST 2019
>   * Make D.P.V3.Core independent of NamedPrim/PrimPatchId

I am not yet convinced hiding of the Core.RepoPatchV3 constructors and
using a pattern synonym is worth the overhead here. We just uncovered
one example where we legitimately want to construct a Core.RepoPatchV3
from a named prim outside of the Core module, forcing us to export an
"unsafe" constructor function mkPrim, circumventing the abstraction
barrier. How is this any better than exporting the Prim constructor in
the first place?

For me, the "official" API has always been Darcs.Patch.Vx; anything
under that are implementation details, not to be accessed from outside.
Such a convention is IMO enough to ensure that invariants aren't
violated, and this is how we have always handled things for V1 and V2.
Has that ever been a problem in practice? Note that exposing Internal
modules (with appropriate warnings attached) is nowadays common practice
even for whole packages.

On 28/08/2019 09:19, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>> patch 587d15fd349e13c83f49e70e7e004f03d4218463
>> Author: Ganesh Sittampalam <ganesh@earth.li>
>> Date:   Wed Aug 28 00:01:56 BST 2019
>>   * Make D.P.V3.Core independent of NamedPrim/PrimPatchId
> 
> I am not yet convinced hiding of the Core.RepoPatchV3 constructors and
> using a pattern synonym is worth the overhead here. We just uncovered
> one example where we legitimately want to construct a Core.RepoPatchV3
> from a named prim outside of the Core module, forcing us to export an
> "unsafe" constructor function mkPrim, circumventing the abstraction
> barrier. How is this any better than exporting the Prim constructor in
> the first place?

mkPrim is better in pure abstraction terms because it doesn't allow
pattern-matching (and the PrimP synonym that does allow it is TestOnly).

> For me, the "official" API has always been Darcs.Patch.Vx; anything
> under that are implementation details, not to be accessed from outside.
> Such a convention is IMO enough to ensure that invariants aren't
> violated, and this is how we have always handled things for V1 and V2.
> Has that ever been a problem in practice? Note that exposing Internal
> modules (with appropriate warnings attached) is nowadays common practice
> even for whole packages.

I don't feel that strongly about keeping this abstract, but it seems
that we have an opportunity to do it and that hiding implementations
where possible means we are forced to stop and think about it if we
actually do want access to them at some point.

Ganesh

[Sorry, following up to myself]

>> For me, the "official" API has always been Darcs.Patch.Vx; anything
>> under that are implementation details, not to be accessed from outside.
>> Such a convention is IMO enough to ensure that invariants aren't
>> violated, and this is how we have always handled things for V1 and V2.
>> Has that ever been a problem in practice? Note that exposing Internal
>> modules (with appropriate warnings attached) is nowadays common practice
>> even for whole packages.
> 
> I don't feel that strongly about keeping this abstract, but it seems
> that we have an opportunity to do it and that hiding implementations
> where possible means we are forced to stop and think about it if we
> actually do want access to them at some point.

Actually an argument against trying to keep it abstract is that it
inhibits refactoring within the Darcs.Patch.Vx namespace. For example we
could plausibly move the graph-based conflict resolution code to a
separate module as it's quite independent, but then we'd certainly have
to export the constructors. So perhaps what we really need is to be
stricter about enforcing the convention about not accessing
Darcs.Patch.Vx.Blah from outside, preferably with something automatic.

> patch 41106e6f1995a8ffa7475f701faa78fb746aeca0
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Tue Aug 27 21:45:45 CEST 2019
>   * harness: don't generate arbitrary PrimPatchIds, instead use 
>     anonymousNamedPrim

> Feel free to amend/rebase/highjack this patch. (I assume your WIP patch
> still needs some polishing and mine depends on it.)

Thanks! I've resent these two to patch1898 and will continue the 
discussion there as this one is completely out of control :-)

>>> For me, the "official" API has always been Darcs.Patch.Vx; anything
>>> under that are implementation details, not to be accessed from outside.
>>> Such a convention is IMO enough to ensure that invariants aren't
>>> violated, and this is how we have always handled things for V1 and V2.
>>> Has that ever been a problem in practice? Note that exposing Internal
>>> modules (with appropriate warnings attached) is nowadays common practice
>>> even for whole packages.
>>
>> I don't feel that strongly about keeping this abstract, but it seems
>> that we have an opportunity to do it and that hiding implementations
>> where possible means we are forced to stop and think about it if we
>> actually do want access to them at some point.
> 
> Actually an argument against trying to keep it abstract is that it
> inhibits refactoring within the Darcs.Patch.Vx namespace. For example we
> could plausibly move the graph-based conflict resolution code to a
> separate module as it's quite independent, but then we'd certainly have
> to export the constructors. So perhaps what we really need is to be
> stricter about enforcing the convention about not accessing
> Darcs.Patch.Vx.Blah from outside, preferably with something automatic.

I was just going to hit you with exactly this argument ;-))

Something automatic to enforce the convention would be nice. The
TestOnly trick doesn't work because of the implicit export of instances.
We could rename V3.Core to V3.Internal, that would at least communicate
the intent pretty clearly. (Though in this case we should really rename
D.R.InternalTypes to something different, otherwise we couldn't even
grep -r Internal without getting false positives).

Warning pragmas would help if we could disable them selectively.

Or perhaps a script that we run during build?

> More generally, when you bring an existential type variable into the
> scope by unpacking its constructor, you are forcing evaluation far
> enough to discover where in the code that existential was originally packed.

I can't believe this. If that were the case we could find out a lot more
about the hidden type when we unpack it, i.e. we'd have run-time type
reflection. This isn't the case: /all/ we know is that we have /some/
type. When we unpack and pattern match on e.g. Sealed, the code that
handles the value we unpacked must be fully (parametrically) polymorphic
w.r.t the existentially quantified type, in this case the second
witness. That is, it must be able to handle /all/ possible instantiations.

On the other hand I don't have a better explanation. It must have
something to do with how unsafeInterleaveIO works.

We should ask the GHC devs.

On 28/08/2019 16:30, Ben Franksen wrote:
> 
> Ben Franksen <ben.franksen@online.de> added the comment:
> 
>> More generally, when you bring an existential type variable into the
>> scope by unpacking its constructor, you are forcing evaluation far
>> enough to discover where in the code that existential was originally packed.
> 
> I can't believe this. If that were the case we could find out a lot more
> about the hidden type when we unpack it, i.e. we'd have run-time type
> reflection. This isn't the case: /all/ we know is that we have /some/
> type. When we unpack and pattern match on e.g. Sealed, the code that
> handles the value we unpacked must be fully (parametrically) polymorphic
> w.r.t the existentially quantified type, in this case the second
> witness. That is, it must be able to handle /all/ possible instantiations.

My statement was just a generalisation/extrapolation of what I already
said about Sealed:

> Operationally, a pattern-match on Sealed is necessarily strict because
> it's a GADT, and that implies evaluating the scrutinee far enough to
> discover *which* Sealed invocation in the code was used.

It doesn't imply that there is actually run-time reflection. Each Sealed
(or other existential constructor) has exactly the same runtime
representation. But we have to evaluate far enough to prove that we
actually had a Sealed and not bottom.

It's no different to this code:

let x = if blah then 1 else 1
case x of
  1 -> ...

But whenever I think I understand strictness/laziness I discover I was
wrong.

>>> More generally, when you bring an existential type variable into
>>> the scope by unpacking its constructor, you are forcing
>>> evaluation far enough to discover where in the code that
>>> existential was originally packed.
>> 
>> I can't believe this. If that were the case we could find out a lot
>> more about the hidden type when we unpack it, i.e. we'd have
>> run-time type reflection. This isn't the case: /all/ we know is
>> that we have /some/ type. When we unpack and pattern match on e.g.
>> Sealed, the code that handles the value we unpacked must be fully
>> (parametrically) polymorphic w.r.t the existentially quantified
>> type, in this case the second witness. That is, it must be able to
>> handle /all/ possible instantiations.
> 
> My statement was just a generalisation/extrapolation of what I
> already said about Sealed:
> 
>> Operationally, a pattern-match on Sealed is necessarily strict
>> because it's a GADT, and that implies evaluating the scrutinee far
>> enough to discover *which* Sealed invocation in the code was used.

I know, and I still don't believe this is the case. It cannot matter
*which* Sealed invocation in the code was used.

> It doesn't imply that there is actually run-time reflection. Each
> Sealed (or other existential constructor) has exactly the same
> runtime representation. But we have to evaluate far enough to prove
> that we actually had a Sealed and not bottom.

I'll buy /that/ statement any time!

In fact I think the explanation for why (unseal seal) works is simple: From

  seal = Sealed

and

  unseal f x = f (unsafeUnseal x)

we can conclude that

  unseal seal x = seal (unsafeUnseal x)
                = Sealed (unsafeUnseal x)

This means (unseal seal) will /always/ return a Sealed, no matter what
the input is. Its result is already in WHNF and tehre is no reason to
evaluate any further. The implementation or sematics of unsafeUnseal is
irrelevant.

On 28/08/2019 17:37, Ben Franksen wrote:

>>> Operationally, a pattern-match on Sealed is necessarily strict
>>> because it's a GADT, and that implies evaluating the scrutinee far
>>> enough to discover *which* Sealed invocation in the code was used.
> 
> I know, and I still don't believe this is the case. It cannot matter
> *which* Sealed invocation in the code was used.

>> It doesn't imply that there is actually run-time reflection. Each
>> Sealed (or other existential constructor) has exactly the same
>> runtime representation. But we have to evaluate far enough to prove
>> that we actually had a Sealed and not bottom.
> 
> I'll buy /that/ statement any time!

OK, I think the way I put it was wrong (particularly given recursion).
Let me rephrase.

We have to prove we had a Sealed.

The only way to do that, is to evaluate far enough that we have a Sealed.

So that means that, for example, we have to do enough evaluation to
distinguish between different statically placed Sealeds in the codebase.
So for example

if blah then Sealed ... else Sealed ...

means we have to evaluate blah.

And in general we can't just float those statically placed Sealeds
upwards to be a single Sealed without messing up the use of witnesses,
particularly because of NilFL.

That was really all I was trying to say, but I put it in a confusing way.

>   unseal seal x = seal (unsafeUnseal x)
>                 = Sealed (unsafeUnseal x)
> 
> This means (unseal seal) will /always/ return a Sealed, no matter what
> the input is. Its result is already in WHNF and tehre is no reason to
> evaluate any further. The implementation or sematics of unsafeUnseal is
> irrelevant.

Yes, I agree with that. But it's useful to observe that it can only be
implemented with something like unsafeCoercePEnd.

> So for example
> 
> if blah then Sealed ... else Sealed ...
> 
> means we have to evaluate blah.
> 
> And in general we can't just float those statically placed Sealeds
> upwards to be a single Sealed without messing up the use of witnesses,
> particularly because of NilFL.

This is true. And I guess since ghc's internal representation is typed
it would be true there, too. Though I wouldn't rely on it...

> That was really all I was trying to say, but I put it in a confusing way.

We are essentially in agreement, then. Good. :-)

>>   unseal seal x = seal (unsafeUnseal x)
>>                 = Sealed (unsafeUnseal x)
>>
>> This means (unseal seal) will /always/ return a Sealed, no matter what
>> the input is. Its result is already in WHNF and tehre is no reason to
>> evaluate any further. The implementation or sematics of unsafeUnseal is
>> irrelevant.
> 
> Yes, I agree with that. But it's useful to observe that it can only be
> implemented with something like unsafeCoercePEnd.

Sure. It still bugs me that we have to go through (unsafely) unpacking
and then repacking the Sealed here. I have tried lots of alternatives
but nothing works that isn't essentially the same as what we have now.
IMO the strictness restriction for pattern matching existentials (and
GADTs in general) is really annoying and I wish they would lift that. I
opened an issue here: https://gitlab.haskell.org/ghc/ghc/issues/17130

> Sure. It still bugs me that we have to go through (unsafely) unpacking
> and then repacking the Sealed here. I have tried lots of alternatives
> but nothing works that isn't essentially the same as what we have now.
> IMO the strictness restriction for pattern matching existentials (and
> GADTs in general) is really annoying and I wish they would lift that. I
> opened an issue here: https://gitlab.haskell.org/ghc/ghc/issues/17130

If the GADT embeds a proof then matching on it has to be strict, or you
can get an unsound program, e.g. you could write

 case undefined :: Int :~: Bool of
  ~Refl -> 5 :: Bool

But that's not true of a pure existential, so some kind of restricted
lazy match would be possible in theory.

>> Can you put this code online somewhere? It doesn't have to work or even
>> compile, just for reading.
> 
> Attached. (If you do want to run it just do things like "printWithMin
> rSTUV" in cabal v2-repl. It's the example from the camp paper showing
> why we have to look at the conflicting set when commuting. The things in
> angle brackets are contexts, on the left-hand side of the | are the live
> patches and on the right-hand side are the reverted ones.)

I finally had the leisure to look at this code. I think this is
brilliant! I can see now why doing this as a name wrapper around a unit
patch type isn't easy or perhaps even impossible.

Your technique is very useful to formalize concrete examples. It is a
bit unfortunate that the 'conflicts' and 'dependencies' must be fixed
statically and cannot be configured at runtime. This means we cannot use
it for quickcheck or leancheck tests, at least not as is. Perhaps this
could be done with a helper class, similar to Commute, where we pass in
the "commutation behavior" as an extra parameter. Also, your patch type
is a fixed, finite type. I'd say we could as well use String for
PrimCore to get an infinite supply of prims.

This could be used for far more than just examining a few concrete
examples we find interesting. For instance, I find it interesting to
investigate which definitions of 'conflicts' / 'dependencies' satisfy
our patch laws. E.g. does permutivity hold for the example type you
defined? How large is the (finite!) set of possible 'dependencies' such
that all your patch laws hold? Can we combine conflicts and dependencies
into a single data structure, something like a matrix?

Furthermore: your ApplyState is trivial, which is okay for the purpose
you had in mind. But I wonder: is it possible to construct, for every
finite PrimCore-like type and each set of conflicts and dependencies
such that the patch laws hold, a suitable "model" of a state, such that
sequences of the corresponding Prim type map to (all) partial bijections
on that state?

> I expect if I did need to use the wrapper I could hide the boilerplate
> in the utility functions, so it'd just be a bit of extra clutter but not
> too annoying.

As far as I understand your code I would have thought this isn't be
possible. The generic wrapper would already have an instance Commute
defined that delegates the actual commutation to the base type, which
doesn't have the identifiers and therefore cannot commute as prescribed
by 'dependencies'. I was therefore ready to admit we may actually need
something like your PrimContainer class.

Cheers
Ben

> Your technique is very useful to formalize concrete examples. It is a
> bit unfortunate that the 'conflicts' and 'dependencies' must be fixed
> statically and cannot be configured at runtime. This means we cannot use
> it for quickcheck or leancheck tests, at least not as is. Perhaps this
> could be done with a helper class, similar to Commute, where we pass in
> the "commutation behavior" as an extra parameter. Also, your patch type
> is a fixed, finite type. I'd say we could as well use String for
> PrimCore to get an infinite supply of prims.

For runtime configuration I was planning on making it table-driven and
indeed maybe using Strings. But I hadn't thought about the class
instance issue. Probably could either drag around the tables with the
patches (just a pointer operationally), or do something complicated with
implicit configuration/reflection.

> This could be used for far more than just examining a few concrete
> examples we find interesting. For instance, I find it interesting to
> investigate which definitions of 'conflicts' / 'dependencies' satisfy
> our patch laws. E.g. does permutivity hold for the example type you
> defined? How large is the (finite!) set of possible 'dependencies' such
> that all your patch laws hold? Can we combine conflicts and dependencies
> into a single data structure, something like a matrix?

I've been doing more with it off and on to try to get a better handle on
some underlying model for conflictors, in particular looking at things
like minimal contexts and how those relate to the patches actually
mentioned in the conflictor. I'd like to get some higher-level
understanding of the rules for commuting conflictors, not just the
operational description we have at the moment. I haven't really figured
out anything illuminating yet though.

And yes, it'd be great to combine conflicts and dependencies into a
single overarching data structure. I've thought about it off and on and
never really come up with anything.

> Furthermore: your ApplyState is trivial, which is okay for the purpose
> you had in mind. But I wonder: is it possible to construct, for every
> finite PrimCore-like type and each set of conflicts and dependencies
> such that the patch laws hold, a suitable "model" of a state, such that
> sequences of the corresponding Prim type map to (all) partial bijections
> on that state?

A state could just be a set recording the PrimCores already applied. But
I'm not sure what use it would be at the moment.

> I can see now why doing this as a name wrapper around a unit
> patch type isn't easy or perhaps even impossible.
[...]
>> I expect if I did need to use the wrapper I could hide the boilerplate
>> in the utility functions, so it'd just be a bit of extra clutter but not
>> too annoying.
> 
> As far as I understand your code I would have thought this isn't be
> possible. The generic wrapper would already have an instance Commute
> defined that delegates the actual commutation to the base type, which
> doesn't have the identifiers and therefore cannot commute as prescribed
> by 'dependencies'. I was therefore ready to admit we may actually need
> something like your PrimContainer class.

Good point that a name wrapper around unit wouldn't work. But I think I
could just reuse PrimCore for the name as well as for the primitive
patch contents, and then use a helper function to guarantee that the
same is used for both. (Which is logically equivalent to what I already
have, as the Ident instance returns the PrimCore.) I'll have a bit more
of a look next time I'm playing with it.

>> This could be used for far more than just examining a few concrete
>> examples we find interesting. For instance, I find it interesting to
>> investigate which definitions of 'conflicts' / 'dependencies' satisfy
>> our patch laws. E.g. does permutivity hold for the example type you
>> defined? How large is the (finite!) set of possible 'dependencies' such
>> that all your patch laws hold? Can we combine conflicts and dependencies
>> into a single data structure, something like a matrix?
> 
> I've been doing more with it off and on to try to get a better handle on
> some underlying model for conflictors, in particular looking at things
> like minimal contexts and how those relate to the patches actually
> mentioned in the conflictor. I'd like to get some higher-level
> understanding of the rules for commuting conflictors, not just the
> operational description we have at the moment. I haven't really figured
> out anything illuminating yet though.

And ultimately it all comes down to establishing general permutivity
(for all n>=2 and including consistency of failure). I am convinced it
should be possible to formally derive the commute rules from that
requirement.

I have also come to see merging as the more fundamental operation
compared to commuting. The direct definition of cleanMerge is nicely
symmetric and easy to understand. I really should rebase the patches and
send them.

>> Furthermore: your ApplyState is trivial, which is okay for the purpose
>> you had in mind. But I wonder: is it possible to construct, for every
>> finite PrimCore-like type and each set of conflicts and dependencies
>> such that the patch laws hold, a suitable "model" of a state, such that
>> sequences of the corresponding Prim type map to (all) partial bijections
>> on that state?
> 
> A state could just be a set recording the PrimCores already applied.

Yes, I guess this is how one would abstractly construct one concrete
ApplyState. However, the set (or type) of all ApplyStates has a bit more
stucture, as not all such sets are valid states. I was curious about a
more declarative way to characterize the states i.e. the allowed sets.

> But I'm not sure what use it would be at the moment.

Just pure fun mathematics :-) It would shed light on the algebraic
structure of patch algebras. For instance, as these are prim patches, no
two elements may be in the set that are conflicting. And for each
element, it must also contain its dependencies. Note how the first
condition limts what we may add to an existing (valid) set, and how the
latter limits what we can remove from it.

>> I can see now why doing this as a name wrapper around a unit
>> patch type isn't easy or perhaps even impossible.
> [...]
>>> I expect if I did need to use the wrapper I could hide the boilerplate
>>> in the utility functions, so it'd just be a bit of extra clutter but not
>>> too annoying.
>>
>> As far as I understand your code I would have thought this isn't be
>> possible. The generic wrapper would already have an instance Commute
>> defined that delegates the actual commutation to the base type, which
>> doesn't have the identifiers and therefore cannot commute as prescribed
>> by 'dependencies'. I was therefore ready to admit we may actually need
>> something like your PrimContainer class.
> 
> Good point that a name wrapper around unit wouldn't work. But I think I
> could just reuse PrimCore for the name as well as for the primitive
> patch contents, and then use a helper function to guarantee that the
> same is used for both. (Which is logically equivalent to what I already
> have, as the Ident instance returns the PrimCore.)

But that would mean we have to duplicate the prim patch data, which I
find ugly. Your PrimContainer class would fit a better.

> Thanks! I've resent these two to patch1898 and will continue the 
> discussion there as this one is completely out of control :-)

I agree. I have completely lost track of what is in this patch now and
what isn't. But this one is still listed as needs-review and I would
like to do that.

This actual patch is purely about these two bundles:

http://bugs.darcs.net/file7890/refactor-the-commute-implementation-for-
namedprims.dpatch
http://bugs.darcs.net/file7906/make-d_p_v3_core-independent-of-
namedprim_primpatchid.dpatch

I think we sort of agreed on changes to the second one to not
try to maintain the abstractness of RepoPatchV3 in the RepoPatchV3.Core
module, but I haven't done them yet. (and it could probably be a followup
that just removes both mkPrim and the pattern synonyms, rather than an
amendment)

This is a new version of the patch that moves the FromPrim
instance to D.P.V3 from D.P.V3.Core, that just exposes the
constructors directly.

I will screen this now and mark the old version as dead.

Then I will send a followup to a new patch that introduces
PrimContainer so we can decide whether to apply it or not.

I'm not removing the pattern synonym immediately because
depending on whether we accept PrimContainer or not, it'll
cause varying degrees of churn in the test code. Once we've
decided on that I'll get rid of the synonym unless we
want the test harness to respect the same abstraction
boundaries as other code (which I doubt).

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

patch ad42379e60af4a3d80757921eba3de557b65b9ff
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Sep  2 10:25:25 BST 2019
  * make D.P.V3.Core independent of PrimPatchId/Prim.Named
  
  This involves moving the FromPrim instance to V3.hs and
  generalising the pattern synonyms used by the test harness.
  
  We now expose the constructors of RepoPatchV3 from D.P.V3.Core
  (but not from D.P.V3, which is the intended external interface).

Attachments

• make-d_p_v3_core-independent-of-primpatchid_prim_named.dpatch
• patch-preview.txt
• unnamed

and one more, must have forgotten to write it initially
1 patch for repository darcs-unstable@darcs.net:screened:

patch dfcdb9ed135d06195957646175224023ec5bb147
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Mon Sep  2 11:18:02 BST 2019
  * add missing haddock to PrimWithName

Attachments

• add-missing-haddock-to-primwithname.dpatch
• patch-preview.txt
• unnamed

There are three bundles still acticve here. Are they all in screened?

> There are three bundles still acticve here. Are they all in screened?

Yes (I just double-checked)

Finally! Sorry it took me so long.