[darcs-devel] darcs patch: add format infrastructure for darcs-2 re... (and 2 more)

David Roundy droundy at darcs.net
Tue Sep 4 16:00:19 UTC 2007


On Sun, Sep 02, 2007 at 02:40:41PM +0200, Eric Y. Kow wrote:
> I'm pushing these in.  I have a few questions
> 
> > Tue Aug 28 09:07:25 PDT 2007  David Roundy <droundy at darcs.net>
> >   * add format infrastructure for darcs-2 repo format.
> > 
> > Tue Aug 28 10:19:13 PDT 2007  David Roundy <droundy at darcs.net>
> >   * add infrastructure for creation for format-2 repos.
> 
> I don't I understand what identifyRepositoryFor and
> readfrom_and_writeto_problem are actually for.  The
> forwards-compatibility check (of old darcs to darcs2) is already handled
> by the pre-existing code, and the repo compatibility check is handled by
> copyInventory (as far as I understand).  What am I missing, and why is
> it only used in pull?

identifyRepositoryFor is intended to check compatibility between two
repositories.  It's for identifying a repository that you want to read from
in order to write to its argument repository.  This is needed since we
can't allow pulling from old repositories to new repositories.

Something like it should also be used in apply, but this requires including
"format" information in patch bundles, which will take a bit of thought.
Send and push should also use identifyRepositoryFor, but I'm not sure they
actually do so.

I'm cutting your positive comments, but not because I don't appreciate
them.  Thanks for reviewing this! (I'm about to send in another big patch
to be reviewed... but it'll be easier, and then followed by some harder
ones.)

> > hunk ./src/Darcs/Commands/Pull.lhs 146
> > +      pw <- tentativelyMergePatches repository "pull" (MarkConflicts:opts)
> > +                 (map hopefully $ reverse $ head us') to_be_pulled
> 
> Note that issue120 should now be even easier to implement (pull
> --allow-conflicts).

Yes, I almost implemented that...

> ======================================================================
> Revert and Unrevert
> ======================================================================
> 
> > hunk ./src/Darcs/Commands/Revert.lhs 102
> > -             write_unrevert (join_patches skipped') p rec
> 
> > +                 write_unrevert repository p rec
> 
> In principle, I like this better, although I'm not convinced the
> behaviour is exactly the same.  I'm a bit concerned that
>   pending +>+ invert unskipped
> may not be exactly the same as
>   skipped
> 
> But it makes a sort of intuitive sense, and if it's wrong, the damage is
> limited and localised :-)

It has the disadvantage that the unrevert patch bundle isn't written until
*after* the repository has been modified, so it's less safe with regard to
power outages.  But I'm pretty well convinced that it's safer with regard
to bugs being introduced, plus simpler.

> > hunk ./src/Darcs/Commands/Unrevert.lhs
> > +      pw <- considerMergeToWorking repository "pull" (MarkConflicts:opts)
> 
> You probably meant "unrevert" instead of "pull".  This only
> feedback, of which there should be none anyway, unless somebody
> starts clamouring for unrevert --allow-conflicts any time soon

Right.  Actually a --dont-allow-conflicts might be reasonable for
unrevert.  Having conflict markings added might come as a rude shock when
you're just running a "harmless" unrevert...

But we should definitely fix this in any case...

> Perhaps we could score a nicer refactor if tentativelyMerge always
> updated the working directory.  We could then use the UpdatePristine
> and DontUpdatePristine flags instead of MakeChanges/DontMakeChanges.

Hmmmm.  Sounds reasonable.  I'll postpone this (or allow you to work on
it?)...

> >          withSignalsBlocked $
> > -          do add_to_pending repository $ join_patches p
> > +          do finalizeRepositoryChanges repository
> 
> Any reason to worry about withSignalsBlocked within withSignalsBlocked?

I don't think it's a reason to worry, but it's better to avoid such
redundancy if we can...

> > +tentativelyMergePatches_ :: MakeChanges
> > +                         -> Repository -> String -> [DarcsFlag] -> [Patch] -> [Patch] -> IO Patch
> > +tentativelyMergePatches_ mc r cmd opts us them =
> 
> Seems fine/unchanged.  I wonder if the following is a common enough
> idiom to make into a function.
> 
>   case merge (p1 :\/: p2)
>     Nothing -> impossible
>     Just (p:<_) -> p
> 
> I've seen it three times so far...

Actually, I think there's a function that does just that.  actual_merge or
something.  But it'll soon be rewritten, and so there's not too much need
just now for a refactor there.

> > +     standard_resolved_pw <- standard_resolution pw
> > +     have_conflicts <- announce_merge_conflicts cmd opts standard_resolved_pw
> 
> Note that if checking for conflicts is to remain a somewhat
> time-consuming operation (on the order of applying a bunch of patches),
> it may be worth putting in a 'Checking for conflicts' verbose log.  This
> would be more accurate that the current 'Diffing dir...' you get when
> you do a pull.

Good idea.  I've some ideas (gleaned from haskell-cafe) that we could use
unsafeInterleaveIO and concurrent Haskell to provide progress updates on pure
computations.  e.g. a function:

progressNumbers :: [a] -> IO [a]

which attaches to each element of the list an action such that when it is
read, a number is printed to the screen.  Something like this should work,
and should allow us to give updates on merge progress, I hope.

> >  finalizeRepositoryChanges :: Repository -> IO ()
> >  finalizeRepositoryChanges repository@(Repo dir _ rf (DarcsRepository _ _)) =
> > +  withCurrentDirectory dir $ do
> > +  testTentative repository
> 
> Wouldn't record and amend-record may be affected by this, that is,
> wouldn't we be running the test twice?

Hmmm.  Didn't I remove those tests? I see I didn't.  Yes, those should be
fixed.

> > +testTentative :: Repository -> IO ()
> > +testTentative repository@(Repo dir opts _ _) =
> 
> Hmm.  Now we're no longer sharing code with Darcs.Test.  I wonder if a
> refactor like the one below could help, or if it would just make the
> code more opaque.  Also, http://bugs.darcs.net/issue489 could probably
> be taken into account.

Yeah, it's definitely suboptimal.  I ran into trouble with circular module
dependencies.

> runTest opts withRepo =
>     do let putInfo = if Quiet `elem` opts then const (return ()) then putStrLn
>        testline <- get_prefval "test"
>        case testline of
>          Nothing -> return ()
>          Just testcode ->
>              withRepo repository (wd "testing") $ \_ ->
>              do putInfo "Running test...\n"
>                 ec <- system testcode
>                 if ec == ExitSuccess
>                   then return TestSucceeded
>                   else do return TestFailed
>     where wd = if LeaveTestDir `elem` opts then withPermDir else withTempDir
> 
> This has a slightly different reporting (no more 'Looks like a good
> patch' and its counterpart).  The new messages seem more appropriate
> anyway.

Sounds reasonable.

> > +withTentative :: Repository -> ((FilePath -> IO a) -> IO a) -> (FilePath -> IO a) -> IO a
> > +withTentative repository@(Repo dir opts _ _) mk_dir f =
> > +    withRecorded repository mk_dir $ \d ->
> > +    do ps <- read_patches (dir ++ "/_darcs/tentative_pristine")
> 
> Seems fine otherwise.  Instead of explicitly passing a patch to test
> (Darcs.Test.test_patch), we use the tentative_pristine patch we have put
> together.  Plus we now have a handy function for doing things with
> tentative.

Yeah, that's what I like.  It moves the whole testing functionality into
the Repository framework it (what seems to me) and elegant way, that allows
us to deal with these tests in a consistent way across commands.  It also
reduces the burden on commands that support testing to just including the
flag.  :)

> > hunk ./src/Darcs/Arguments.lhs 801
> > -want_external_merge :: [DarcsFlag] -> Maybe String
> 
> > hunk ./src/Darcs/Flags.lhs 83
> > +want_external_merge :: [DarcsFlag] -> Maybe String
> 
> Yes, this does seem like a more sensible place to put this sort of
> thing.  More migrations like this to come, I guess.

Maybe more sensible, but the real reason for moving these was to avoid a
module import loop.  :( I'm still convinced (though it's trying) that
avoiding module import loops gives (in the long run) a cleaner and clearer
module structure.
-- 
David Roundy
Department of Physics
Oregon State University
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.osuosl.org/pipermail/darcs-devel/attachments/20070904/e4a2c873/attachment.pgp 


More information about the darcs-devel mailing list