I'm pushing these in. I have a few questions > Tue Aug 28 09:07:25 PDT 2007 David Roundy <[EMAIL PROTECTED]> > * add format infrastructure for darcs-2 repo format. > > Tue Aug 28 10:19:13 PDT 2007 David Roundy <[EMAIL PROTECTED]> > * 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?
> Wed Aug 29 10:00:12 PDT 2007 David Roundy <[EMAIL PROTECTED]>
> * Refactor merging functionality into Repository.
Overall, I like it. You asked for a review, so...
======================================================================
Apply and Pull
======================================================================
> hunk ./src/Darcs/Commands/Apply.lhs 26
> + pw <- tentativelyMergePatches repository "apply" opts
> + (map hopefully $ reverse $ head us') to_be_applied
Seems fine. Both this and Pull appear unchanged. Glad to see us
no longer importing from Darcs.Pull :-)
> 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).
> -merge_with_us_and_pending :: [DarcsFlag] -> ([Patch],[Patch]) ->
> - IO (Patch, Patch)
> -merge_with_pending :: [DarcsFlag] -> Patch -> IO Patch
(Note to self: consolidated into tentativelyMerge)
> -check_unrecorded_conflicts :: [DarcsFlag] -> Patch -> IO Bool
> -check_unrecorded_conflicts opts pc =
Moved to Darcs.Repository.Internal with no changes.
======================================================================
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 :-)
> 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
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.
Make sense to me otherwise.
> hunk ./src/Darcs/Commands/Unrevert.lhs 100
> - pend <- read_pending repository
> - let pend_and_p = case pend of
> - Nothing -> join_patches p
> - Just pending -> join_patches (pending : p)
> + tentativelyAddToPending repository opts $ join_patches p
Makes sense. write_unrevert need only look at the updated pending
> withSignalsBlocked $
> - do add_to_pending repository $ join_patches p
> + do finalizeRepositoryChanges repository
Any reason to worry about withSignalsBlocked within withSignalsBlocked?
======================================================================
Internal
======================================================================
> +data MakeChanges = MakeChanges | DontMakeChanges deriving ( Eq )
See comment re: UpdatePristine
> +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...
> + 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.
> + pw_resolved <- fmap join_patches $
Looks right. Plus the old (map hopefully $ reverse $ head us') was
yucky
> + putVerbose "Applying patches to the local directories..."
> + when (mc == MakeChanges) $
> + do mapM_ (tentativelyAddPatch r opts) $ fromJust $ unjoin_patches
> pc
> + tentativelyAddToPending r opts pw_resolved
> + return pw_resolved
> +announce_merge_conflicts :: String -> [DarcsFlag] -> [Patch] -> IO Bool
> +announce_merge_conflicts cmd opts resolved_pw =
Indeed, this function looks the same in both apply/pull (except for
print-outs, but that was only because of pull not supporting
--mark-conflicts.
> 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?
> +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.
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.
> +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.
======================================================================
Miscellaneous
======================================================================
> 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.
--
Eric Kow http://www.loria.fr/~kow
PGP Key ID: 08AC04F9 Merci de corriger mon français.
pgp3bMCAEWPvY.pgp
Description: PGP signature
_______________________________________________ darcs-devel mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-devel
