On Thu, Jul 15, 2010 at 09:39:32 +0000, Petr Ročkai wrote: > Thu Jul 15 10:19:08 CEST 2010 Petr Rockai <[email protected]> > * Remove [DarcsFlag] usage from Darcs.Patch.Bundle.
Looks like this was applied from another bundle. > Thu Jul 15 11:38:42 CEST 2010 Petr Rockai <[email protected]> > * Resolve issue1892: Improve safety of makeBundle* and fix a couple of > related bugs. The bug in question confusion on the order that patch bundle code expects and returns patches. This patch uses directed lists to avoid these kinds of bugs in the future. I guess there's no reason for me not to apply this patch, but let's wait for your explanation on fetchPatches/makeBundle so that I'm up to speed too. Some day I'll get this stuff... Resolve issue1892: Improve safety of makeBundle* and fix a couple of related bugs. ---------------------------------------------------------------------------------- The core part of this patch seems fairly cut and dry My main stumbling block in reviewing this patch was sheer unfamiliarity (embarrassing!) with the witnesses and also a bit with newset. General comments: * I wish Darcs.Patch.Set.PatchSet was haddocked. Darcs.Patch.Bundle ~~~~~~~~~~~~~~~~~~ Would there be any use in having a witnessesed PatchInfo? > - makeBundle2 the_s (mapRL info ps ++ [info t]) to_be_sent to_be_sent > + makeBundle2 the_s (ps +<+ (t :<: NilRL)) to_be_sent to_be_sent QUESTION: do functions like newset2RL do what you want here, or is that something totally different? > - makeBundle2 the_s (mapRL info ps) to_be_sent to_be_sent > + makeBundle2 the_s ps to_be_sent to_be_sent We avoid losing the fact that the PatchInfo being passed to makeBundle2 are in RL order. There are no witnesses associated with PatchInfo so we just pass the patches. > hunk ./src/Darcs/Patch/Bundle.hs 64 > -makeBundle :: RepoPatch p => Maybe (Tree IO) -> [PatchInfo] -> FL (Named p) > C(x y) -> IO Doc > +makeBundle :: RepoPatch p => Maybe (Tree IO) -> RL (PatchInfoAnd p) C(start > x) > + -> FL (Named p) C(x y) -> IO Doc makeBundle just thinly wraps around makeBundle2 which now demands PatchInfoAnd and specifies RL > -makeBundle2 :: RepoPatch p => Maybe (Tree IO) -> [PatchInfo] > +makeBundle2 :: RepoPatch p => Maybe (Tree IO) -> RL (PatchInfoAnd p) C(start > x) > -> FL (Named p) C(x y) -> FL (Named p) C(x y) -> IO Doc > hunk ./src/Darcs/Patch/Bundle.hs 75 > -makeBundle2 the_s common to_be_sent to_be_sent2 = > +makeBundle2 the_s common' to_be_sent to_be_sent2 = > + common = mapRL info common' Again, not much to see. What we really want to use is the patch info for the context file, but in order to make use of witnesses we postpone losing the patches until the last minute. Darcs.Commands.Pull ~~~~~~~~~~~~~~~~~~~ > fetchPatches :: FORALL(p r u t) (RepoPatch p) => [DarcsFlag] -> [String] -> > String -> > Repository p C(r u r) -> > - IO ( [PatchInfo] , Sealed ((FL > (PatchInfoAnd p) :\/: FL (PatchInfoAnd p)) C(r))) > + IO ( SealedPatchSet p C(Origin), Sealed ((FL > (PatchInfoAnd p) :\/: FL (PatchInfoAnd p)) C(r))) I've taken the liberty of playing with whitespace to make the actual change clear. I have to rediscover this stuff over and over and over again due to not using it frequently enough * (:\/:) opens the merge diamond (read bottom up) * data (a1 :\/: a2) C(x y) = FORALL(z) (a1 C(z x)) :\/: (a2 C(z y)) The witnesses x y denote the individual end states for the two paths. We don't track the common starting state but we do say it has to be the same for both. Quite useful to be reminded that when you see pairs of witnesses, they don't necessarily talk about before/after. * Sealed (us :\/: them) We agree to lose the token for them's ending state. * NEW: Data.Patch.Set.Origin is just data Origin type with no data constructor, so it really is just a universal witness to mean the origin, zero patches. * NEW: SealedPatchSet p C(Origin): patches common to the two repositories start from Origin. We lose the token for the intermediate state after common and before opening the diamond OK, that's only a very superficial reading because I'm still not 100% comfortable working the witnesses. > - common' :>> _ <- return $ findCommonWithThem us them > - let common = mapFL info $ newset2FL common' > + common :>> _ <- return $ findCommonWithThem us them We no longer need to extract patch info as this is done closer to the core > - return (common, seal $ us' :\/: to_be_pulled) > + return (seal common, seal $ us' :\/: to_be_pulled) Still don't really get why we're sealing but OK > applyPatches :: > - forall p C(r u t). (RepoPatch p) => [DarcsFlag] -> Repository p C(r u r) > -> > + forall p C(r u t a). (RepoPatch p) => [DarcsFlag] -> Repository p C(r u > r) -> QUESTION: Does the extra a serve some kind of purpose here? I'm guessing it just snuck in by accident? > - ([PatchInfo], Sealed ((FL (PatchInfoAnd p) :\/: FL (PatchInfoAnd p)) > C(r))) > + (SealedPatchSet p C(Origin), Sealed ((FL (PatchInfoAnd p) :\/: FL > (PatchInfoAnd p)) C(r))) > -> IO () Keeping in mind that pull is fetchPatches .. >>= applyPatches .. applyPatches doesn't care about common so no need to change anything else > makeBundle :: > - forall p C(r u t) . (RepoPatch p) => [DarcsFlag] -> > + forall p C(r u t x) . (RepoPatch p) => [DarcsFlag] -> QUESTION: Same question about the 'x' > - ([PatchInfo], Sealed ((FL (PatchInfoAnd p) :\/: FL > (PatchInfoAnd p)) C(r))) > + (SealedPatchSet p C(Origin), Sealed ((FL (PatchInfoAnd p) :\/: FL > (PatchInfoAnd p)) C(r))) Whitespace jiggled for clarity. > hunk ./src/Darcs/Commands/Pull.lhs 264 > -makeBundle opts (common, Sealed (_ :\/: to_be_fetched)) = > +makeBundle opts (Sealed common, Sealed (_ :\/: to_be_fetched)) = > - bundle <- PatchBundle.makeBundle Nothing common $ > + bundle <- makeBundleN Nothing (unsafeCoercePEnd common) $ OK, so this makeBundle is used for darcs fetch. It looks like we are using makeBundleN instead of makeBundle because we no longer smoosh the patchset (see newset2FL non-use and definition above); that work is now delegated to makeBundleN (see question above). The unsafeCoercePEnd was something slightly uncomfortable for me (in the sense of being still a bit new territory for me). My understanding is that it's because we've sealed off ending point of the common patches, so when we unseal it the witness for the ending point is an eigenvariable which does not unify with anything else, as explained by Jason in http://wiki.darcs.net/DarcsInternals#type-witnesses In makeBundleN, however, we require that the common patches end in the same place as the to-be-sent patches begin. QUESTION: So then there's the question of why we have to seal off the ending state of the common patches in fetchPatches. Shouldn't we be able to keep track of the fact that the common end in the same place as the us/them patches begin? As an aside, reviewing this patch is forcing me to look a little bit at the NewSet code, one of the indirect benefits of the patch review process... slowly learning the Darcs code. As a second aside, I'm a little bit curious about how newset copes with parallel tags (saying resulting from a merge of two repos their own tags). I'm guessing it's a non-issue and that the code makes no particular assumptions about how one tag states lead to another (and the representation is the same Darcs sequence of patches one anyway) > hunk ./src/Darcs/Commands/Pull.lhs 269 > - (x:>:_)-> PatchBundle.patchFilename $ patchDesc x > + (x:>:_)-> patchFilename $ patchDesc x No need to qualify the import because we tightened it up in the imports list (excluded from this review) Darcs.Commands.Put ~~~~~~~~~~~~~~~~~~ > - bundle <- makeBundle2 Nothing [] patches patches2 > + bundle <- makeBundle2 Nothing NilRL patches patches2 Boring change Darcs.Commands.Unrecord ~~~~~~~~~~~~~~~~~~~~~~~ > savetoBundle opts kept removed@(x :>: _) = do > hunk ./src/Darcs/Commands/Unrecord.lhs 347 > - bundle <- makeBundle Nothing (mapFL info kept) > - (mapFL_FL hopefully removed) > + bundle <- makeBundle Nothing kept (mapFL_FL hopefully removed) Boring change -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> For a faster response, please try +44 (0)1273 64 2905.
pgpNt4H2KoUai.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
