[darcs-devel] darcs patch: add an unused RepoFormat module. (and 13 more)

[Ian Lynagh]

On Mon, Jul 11, 2005 at 06:40:07AM -0400, David Roundy wrote:
> On Sun, Jul 10, 2005 at 09:34:27PM +0100, Ian Lynagh wrote:
> > 
> > > +\begin{code}
> > > +can_read :: RepoFormat -> Bool
> > > +can_read (RF ks) = all cr ks
> > > +    where cr :: [PackedString] -> Bool
> > > +          cr [] = True
> > > +          cr ps = any is_known ps
> > 
> > >  can_write :: RepoFormat -> Bool
> > > +can_write (RF ks) = all cw ks
> > > +    where cw [] = True
> > > +          cw x = all is_known x
> > 
> > These [] cases can't actually happen, can they? I think it would be
> > better if they were removed, or perhaps replaced with impossibles.
> 
> You're right.  I think impossible would be better, since if there *were* a
> [] in the list, then can_read would return False, and we'd give a bad error
> message.

I meant for both can_read and can_write, as [] isn't possible from the
way the RepoFormat is constructed (in particular the filtering of null
lines).

> > > +slurp_recorded_and_unrecorded (DarcsRepository _ rf)
> > > +    | can_read rf = DarcsRepo.slurp_recorded_and_unrecorded "."
> > > +    | otherwise = fail $ read_problem rf
> > 
> > I think it's probably better to restructure this so we do something like
> > 
> > slurp_recorded_and_unrecorded (DarcsRepository _ rf)
> >  = case can_read rf of
> >        CanRead -> DarcsRepo.slurp_recorded_and_unrecorded "."
> >        CannotRead err -> fail err
> 
> Actually, I've moved the check to a higher level:
> 
> slurp_recorded_and_unrecorded (Repo _ rf _)
>     | not $ can_read rf = fail $ read_problem rf

I'm lost as to what you mean here; it looks the same to me.

(the point of my restructuring was to avoid having to have two functions
can_read and read_problem that both did essentially the same thing, the
latter having an impossible check for when it was incorrectly used.
Similarly can_write and write_problem would be merged)

> > > +  assert_repo_writeable repository
> > 
> > Would it be better to have an am_in_writeable_repo prereq for most of
> > these?
> 
> Or perhaps even better would be to put this check in withRepoLock? Then
> we'd not be able to forget to use it, since we need to run withRepoLock any
> time we're going to modify the repository anyways.  This seems cleanest to
> me.

That sounds good. I guess we ought to do the check after taking the lock
anyway, so we can have a safe "darcs change-the-repo-format" in the
future.

> I don't think we need to check in push or send whether we know how to write
> to the target repository, since we won't actually be the one doing the
> writing.  It may be that we're pushing to a machine with a newer darcs that
> *can* write to the target repo.

Right, but I think we should be sending something that can be /read/ by
anything that can read the repo, as it's reasonable to assume the remote
darcs can read that.

> > > -{-# OPTIONS -fffi #-}
> > > +{-# OPTIONS -fffi -fno-warn-unused-binds #-}
> > 
> > What's this for?
> 
> It's the only way I could figure out to create a dummy data type for type
> safety with the Ptrs:
> 
> data GitTreeIteratorStruct = GTIS

Ah. You can say

data GitTreeIteratorStruct

but I think you need the full -fglasgow-exts hammer  :-(


Thanks
Ian