[darcs-users] darcs patch: add substitution mechanism for PatchChoices (and 4 more)

[Ganesh Sittampalam]

On Wed, 23 Sep 2009, Eric Kow wrote:

> Still plodding along for this review.
>
> On Sat, Sep 19, 2009 at 11:54:08 +0100, Ganesh Sittampalam wrote:
>> Mon Aug 31 00:46:47 BST 2009  Ganesh Sittampalam <ganesh at earth.li>
>>   * add some QC properties that demonstrate the problem with canonizeFL
>
> Pushing this one... albeit with a slipshod review :-(

Apologies: it was a slipshod submission too. I forgot that it depends on 
my patch to add witnesses to Darcs.Diff. That patch probably won't go in 
in its current form because it conflicts with darcs-hs, so I'm rolling 
this back for now.

> add some QC properties that demonstrate the problem with canonizeFL
> -------------------------------------------------------------------
> Silly Eric...
>
>  data RepoModel C(x)
>   = RepoModel {
>       rmFileName :: !FileName,
>       rmFileContents :: [B.ByteString]
>     } deriving (Eq)
>
> My brain wouldn't stop reading that 'rm' as 'remove' instead of 
> 'repoModel'.  I kept wondering why we were trying to track these 
> removes.

I actually have that problem with it too despite having written it. It 
should be fixed :-)

> Does this mean in general that our unit tests operate only on one-file 
> repos?

The unit tests in that module have that property, but they are primarily 
intended to test the basics of patch theory so that's fine. I was sort of 
piggy-backing on them with this test because I knew that the pieces I 
needed for it were there. However eventually they should probably be 
generalised.

>> +canonicalDiff :: RepoModel C(x) -> RepoModel C(y) -> FL Prim C(x y)
>> +canonicalDiff rm1 rm2 = diff [IgnoreTimes] (const TextFile) (repoModelToSlurpy rm1) (repoModelToSlurpy rm2)
>
> This will be better when Neil's CmdArgs work lands

I don't think this aspect of the option handling is a big deal.

>
>> +instance Arbitrary (Sealed RepoModel) where
>> +  arbitrary = do wes <- arbitraryState initRepoModel
>> +                 let _ :: Sealed (FL Prim C(InitRepoModel)) = mapSeal wesPatch wes
>> +                 return (mapSeal wesState wes)
>
> Out of curiosity, why not just let foo :: and return foo?

Errm. Dunno :-) The let _ .. would have been added late to get past the 
type-checker, so I suspect I just didn't notice what I was doing.

> I don't really know much about QC so I'm learning as I go.  Shrink just 
> returns a list a possible shrinks - which I take to mean simpler 
> candidate values.  I'm guessing that the use for this would be to see if 
> you can get the same failure out of a shrink, in which case you should 
> just use that to avoid reporting a whole lot of gunk.  But that's just a 
> guess.

Correct.

>> +shrinkList [] = []
>> +shrinkList (x:xs) = xs:map (x:) (shrinkList xs)
>
> I think the idea here is that each candidate shrink is just the result of
> getting rid of some lines
>
> Isn't this just some common list idiom like powerset? (Which is not to say
> that we should necessarily use any of the fancy liftM tricks, but maybe just
> call it what it is).

It's not powerset, because it only removes one element at a time. I'm 
surprised it's not in the QC library somewhere, but I couldn't spot it so 
just wrote it locally.

> Now to the meaty bits.  Basically we want to check if canonicalDiff always
> gives us the same result as taking two diffs and then canonizing.
> [...]
> I guess we're looking for two things.  Canonicalness subsumes minimality, so
> I'm not sure why we test for the latter.  My guess is that it's because
> minimality is useful on its own and easier to achieve.

Correct. Though I still don't know how to do it, but at least it seems 
like it might be easier :-)

> I confess I haven't run these tests yet -- is that easy to do?

Easiest thing is to bring them up in ghci and run them manually. I 
generally get ghci going by doing cabal build -v and then copying the ghc 
command line but replacing --make with --interactive.

Ganesh