Elisions and responses interspersed below. On Sun, 11 Feb 2007 10:42:36 +0100, "Eric Y. Kow" <[EMAIL PROTECTED]> wrote:
> Hi Kevin (and all), > > > Sun Feb 4 11:13:01 MST 2007 Kevin Quick <[EMAIL PROTECTED]> > > * Added --complement to pull to allow "exclusion" repos > > > > Tue Feb 6 00:18:32 MST 2007 Kevin Quick <[EMAIL PROTECTED]> > > * Resolve conflict between complement add and get_recorded_unsorted. > > I'm going to accept these since the feature does not seem itself > uncontroversial and any bugs it may have seem to self-contained. Thank you. > WARNING: I don't fully understand the code in patchset_complement > function in depends, although I have looked at it, and from the > comment, have a rough idea what it's for. Necessity being the mother of invention, I have a local darcs tree which contains a mixture of primary work and miscellaneous fixes. The miscellaneous fixes are often to core library code that needs careful peer review and moderate uptake for the principle repository (which unfortunately is CVS, but that's not strictly relevant). Having a local tree means I can save time by addressing minor, miscellaneous library issues immediately on encountering them without the risk of checking them back into the core repository. Although I do trickle these back to the core as peer review and personal use validates them, this is a slow process; at the same time I have current active work that does need to be propagated to the core quickly. Over time, the miscellenae that is not ready to be pushed to the core tends to bunch up at the "beginning" of the patch list. Thus, each time I wanted to pull and important patch back to the core, I found myself typing 'n' some 80 or 90 times to skip this unready bunch. With the "complement" functionality, I can pull the 80 or 90 unready patches into a "not-yet" darcs repo, then do: $ darcs pull --complement work-tree not-yet-tree and all I see are the primary candidates from the work tree. > Definitely appreciate the tests. Thanks! > > Unfortunately, I get this error: > chgrec '3iline2.1\nline2.2' inssub2 > sed: 1: "3iline2.1\nline2.2 > ": command i expects \ followed by text > > Could you resubmit? Done, although I don't see the error locally, so if it still fails, feel free to contact me off-list to coordinate the fixes OOB. > This is code I'm accepting anyway, but I can rarely resist the urge to > nitpick. One man's nitpick is another's education---I welcome the input as I am often myself guilty of the former and desireous of the latter, and most especially as I am a Haskell newbie. Any rebuttals I offer below are in the spirit of general discussion; feel free to make any changes you feel necessary to improve or align the code better. > Perhaps this could be made easier to understand with the case () of _ pattern, > something like this > > return $ case () of _ | Intersection `elem` opts -> > (patchset_intersection rs, []) > | Complement `elem` opts -> (head rs, > patchset_union $ tail rs) > | otherwise -> (patchset_union > rs, []) Interesting pattern... I had not seen that before. Seems like a bit of work for the compiler just for the sake of prettiness in the code, but OTOH, that's what compilers are for, so this looks fine to me. > Also, I tend to dislike uses of head, tail and the like. I guess > there's little risk of something blowing up, given the read_repos _ [] > case, but I wonder if there would be an elegant way to avoid this. I assume your concern is head operations on an empty list; as you noted that can't happen here, and it seems like the cleanest method to separate the first repo from the remainder. Then again, see my newbie disclaimer above. > > Depends.lhs > ----------- > > +patchset_complement x1 xs = > > I think you could make the code a bit more readable by making > push_to_end an independent function in Depends. I left it within patchset_complement to limit the scope (along the lines of the "self containment" in your first comment of your email). As presented, I don't envision it being useful outside of the patchset_complement function, so I don't think there would be anything to gain by externalizing it other than the readability potential you aver. > > > + with_partial_intersection x1 (patchset_union [xs]) $ > > + \_common a b -> let common' = (map fst a) `intersect` (map fst b) > > Interestingly, GHC does not complain about the unused _common presumably > because of the initial underscore (for example, it *does* complain about > xxxxxxx). Bug or feature? In any case, we should probably get rid of > this. My understanding was that this was a feature. I used this notation to make it easier to understand what with_partial_intersection was supplying to the sub-function thereby perhaps aiding in understanding the sub-function. It's clearly not necessary though. > Also, one thing I did not understand is taking the intersection of > (map fst a) and (map fst b). Perhaps I am assuming wrong, thinking > that a and b (from with_partial_intersection) are disjoint? Can you > explain this to me? Not entirely. :-) I used patch_intersection and patch_union as a model, and my analysis of with_partial_intersection was that it was primarily length-matching the list of patches to consider and doing a first-pass estimation to give the sub-function a head start on deciding which patches needed more careful evaluation. > > pushing_add = liftM2 (++) pushing $ liftM2 (flip (:)) (return []) ep > > before_add = liftM2 (++) before . return . listof . curry id pinfo . > > actually > > This code has made my head explode. I feel hoist by my own monad-loving > pointfree-liking petard. > > Is there any chance that this could be rewritten in a more obvious > fashion? Any comments from the list? I don't want to start any style > debates or anything; maybe this is the ideal form because of its > compactness. But maybe there are some simplifications lurking in this > code. > > (The monad in question is Either MissingPatch, as far as I understand) It could be expanded into multiple stages, but I don't know that it would improve the readability IMHO. Both "pushing" and "before" are [(Either a b)] lists (as you noted), utilizing the Either's monading binding feature of Left predominance. The functions above add (append) a new element to the named list, preserving this Either monadic property. The tail part of each function above is primarily to convert the input into the form needed for the corresponding list. Since this is highly specific to each list, a co-function doesn't seem to clarify and a sub-function seems to add another level of let scoping that would obscure any readability gain. Much of this code is modelled on get_extra, but with adjusted functionality more appropriate to the complementing task. ***Side note: I did have to spend a significant amount of time determining the proper sequencing to utilize when working with lists of patches, and I presently have the (possibly incorrect) impression that this is a dangerous part of the internals of darcs. It is simply too easy to apply "reverse" to a set of patches and end up with uncommuted reversal of patches, yet on the other hand, this reversal is sometimes necessary for the algorithm at hand. I would suggest that subtle patch re-ordering bugs could be avoided by using a different container (other than a standard list) that did not allow patch re-ordering without proper commutation; however, this would be a fairly pervasive change and would require proper analysis of those conditions (like complement) where reversal without commutation seems to be necessary for the algorithm's operation. > > > ind_ps = listof . reverse > > listof = flip (:) [] > > Speaking of simplification, it seems like these helpers could be > rewritten as > > ind_ps x = [reverse x] > listof x = [x] They could. My understanding was that by writing them as pure functions, GHC could more easily recognize their status as such and perform optimizations based on this knowledge. In the case of complement, I believe that optimizations are useful because I believe this to be the most expensive form of pulling because it requires examination (and commutation) of nearly every single candidate patch in all specified repositories. Note that my use of "believe" several times in the above statement means that I have not fully proven or empirically tested these and that they are therefore untested hypotheses on my part. -- -- Kevin Quick quick at org after sparq -- -- Kevin Quick Ticketmaster R&D [EMAIL PROTECTED] _______________________________________________ darcs-devel mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-devel
