[darcs-devel] [patch1880] decouple RepoPatchV3 impl from NamedPrim

[Ben Franksen]

Ben Franksen <ben.franksen at online.de> added the comment:

> 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.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1880>
__________________________________