[darcs-devel] darcs patch: Added --complement to pull to allow "exc... (and 3 more)

Kevin Quick quick at sparq.org
Tue Feb 13 23:36:56 PST 2007


Elisions and responses interspersed below.

On Sun, 11 Feb 2007 10:42:36 +0100, "Eric Y. Kow" <eric.kow at gmail.com>
wrote:

> Hi Kevin (and all),
>
> > Sun Feb  4 11:13:01 MST 2007  Kevin Quick <quick at sparq.org>
> >   * Added --complement to pull to allow "exclusion" repos
> >
> > Tue Feb  6 00:18:32 MST 2007  Kevin Quick <quick at sparq.org>
> >   * 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
Kevin.Quick at Ticketmaster.com


More information about the darcs-devel mailing list