On Fri, May 15, 2009 at 8:44 AM, Eric Kow <[email protected]> wrote: > Woo! Thanks for the review... Here's my contribution to your first stab. > > I still don't fully understand what's going on, but I believe that this > is a perfectly safe patch to apply, so I'll go ahead and apply it. > > On Wed, May 13, 2009 at 18:38:17 -0700, Jason Dagit wrote: >> with_selected_last_changes_to_files' "rollback" opts >> - existing_files (sort_coalesceFL $ effect ps) $ \ (_:>ps'') >> - >> + existing_files (concatFL $ mapFL_FL canonize $ >> + sort_coalesceFL $ effect ps) $ \ (_:>ps'') >> - > >> Understanding this patch is really understanding why we want to canonize. >> Looking in Darcs.Patch.Prim I see: > > Note that we have to concat because canonize returns a list because... > >> It can sometimes be handy to have a canonical representation of a given >> patch. We achieve this by defining a canonical form for each patch type, >> and a function ``{\tt canonize}'' which takes a patch and puts it into >> canonical form. This routine is used by the diff function to create an >> optimal patch (based on an LCS algorithm) from a simple hunk describing the >> old and new version of a file. > >> canonize :: Prim C(x y) -> FL Prim C(x y) >> canonize (Split ps) = sort_coalesceFL ps >> canonize p | IsEq <- is_identity p = NilFL >> canonize (FP f (Hunk line old new)) = canonizeHunk f line old new > > If you study the code behind canonizeHunk, you get the impression that > it just does an LCS.getChanges on the before and after parts of the hunk > patch which returns a list of changes (if you do a diff on a block of > text, you naturally expect a list of changes because different parts of > that block may have changed). This is cute :-), canonicalising a patch > by taking a diff on its before and after bits. > >> I think, all this does is change the representation back and forth a bit >> trying to break things apart and reassemble them to be minimal then >> concatenate the whole thing back together. > > OK, so let's aim for that higher level explanation. I suppose the claim > that is implicitly made in this patch is that coalescing the primitive > patches that we are trying to rollback can result in non-minimal hunks, > in other words, hunks whose before and after bits have things in common. > If this is true, this explains why we running canonize is indeed > smarter; it means we get rid of the boring common bits and generate a > rollback patch with more smaller hunks instead of fewer larger hunks. > > What do you think of this explanation? It's mostly an rash intuitive > leap, so it could well be totally bogus. For this to be more convincing > to me, I would need somebody to explain to me what could cause the > coalescing process to generate these non-minimal hunks, at the very > least, in the context of rollback. Maybe a simple example would do? > > -- huh, why the effect -- of the patches we are rolling back > >> I do wonder about the operations at the high level. For example, it seems >> as though we can dodge a call to mapFL_FL by doing a concatFL between the >> sort_coalesceFL and the canonize. At first I thought maybe the >> sort_coalesceFL followed by a canonize was redundant and the sort_coalesceFL >> could be removed, but it may be hard to verify that and I doubt we have any >> test cases for rollback that would check if it is equivalent. > > Well, it's not surprising to me that we would want to coalesce these > patches and them "reorder" (commutation permitting them) in some > canonical order. Rollback allows us to select several patches, so it > would certainly be comfortable if the patch we generate was presented > in coalesced form as though the user were doing a darcs record. See > what I mean? > >> Bottom line: It looks like it breaks things apart and tries to simplify >> them as it puts them back together. > > I would say the reverse (smoosh them and *then* berak them apart). > Rollback sans this patch smooshes things together for our convenience, > and this patch adds a little bit of finesse (breaking apart) to this > smooshing. If this is the case, what I still do not understand is why > we need this finesse. > >> There may be some redundant cases that would offer an unknown amount >> of optimization, but the current implentation is probably more >> conservative (meaning safe) and not likely to be a bottle neck. > > Do you agree that coalescing is useful in this case? And if so, do > you still feel there are redundant cases here?
I'm not sure what to think, other than I think I would have to see examples. Jason _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
