[darcs-users] [patch273] Haddock merge2FL and fastRemoveFL in Pat... (and 4 more)

Eric Kow kowey at darcs.net
Wed Jun 23 11:07:16 UTC 2010


On Mon, Jun 07, 2010 at 20:06:18 +0000, Petr Ročkai wrote:
> Mon Jun  7 20:48:49 CEST 2010  Petr Rockai <me at mornfall.net>
>   * Haddock merge2FL and fastRemoveFL in Patch.Depends.

> Mon Jun  7 20:50:41 CEST 2010  Petr Rockai <me at mornfall.net>
>   * Extend the issue1014 test to check that named patches are not duplicated.

Pushed in my first review from 2010-06-08

> Mon Jun  7 20:50:10 CEST 2010  Petr Rockai <me at mornfall.net>
>   * Extend the get test to cover --context interaction with tags.

Applied, thanks!

> Mon Jun  7 20:49:34 CEST 2010  Petr Rockai <me at mornfall.net>
>   * Use merge2FL instead of plain merge in tentativelMergePatches.

Applied, thanks!

> Mon Jun  7 21:59:12 CEST 2010  Petr Rockai <me at mornfall.net>
>   * Make partitionFL do a 3-way split, and detect commutation bugs in Depends.

More review later

We also talked about this bundle a little bit on IRC
  http://irclog.perlgeek.de/darcs/2010-06-23#i_2471430

To sum, the context to this patch bundle is that the recent NewSet works
introduces some silent-but-deadly situations:

 (A) We no longer notice the anomalous situation where two different
     patches have the same patchinfo.  This sometimes arises when you
     split a CVS repository into two Darcs repositories and then try
     to merge them back together in Darcs, eg. with nofib and GHC
     (because CVS does not have atomic commits, but it lets you recycle
     the commit message across different files)

 (B) The issue1014 test no longer complains, but now instead of crashing
     it causes your history to have TWO instances of the same patch
     (turning your patchset into an unfortunate patchbag)

The game here is to attempt to resolve (A) and try to prepare for a
future resolution of (B); so two independent patches which can be
applied independently of each other.

Haddock merge2FL and fastRemoveFL in Patch.Depends.
---------------------------------------------------
> +-- | Merge two FLs (say L and R), starting in a common context. The result is a
> +-- FL starting in the original end context of L, going to a new context that is
> +-- the result of applying all patches from R on top of patches from L. This
> +-- version also correctly handles duplicate patches.

While trying to review the patch below, I found myself having lots of
little questions about how merge2FL relates to mergeFL, which Petr
clarified.  I'll be following up with a haddock patch of my own, which
Petr has acked on IRC:

http://hpaste.org/fastcgi/hpaste.fcgi/view?id=26477#a26477

Use merge2FL instead of plain merge in tentativelMergePatches.
--------------------------------------------------------------
As above, this patch does not actually have any effect; it's merely
rearranging the furniture in anticipation for future work on issue1014.

>  Ignore-this: 9fde612f399cab5553e9cf57e0764685
> hunk ./src/Darcs/Repository/Merge.hs 58
>  tentativelyMergePatches_ mc r cmd opts usi themi =
>    do let us = mapFL_FL hopefully usi
>           them = mapFL_FL hopefully themi
> -     _ :/\: pc <- return $ merge (progressFL "Merging them" them :\/: progressFL "Merging us" us)
> +     Sealed pc <- return $ merge2FL (progressFL "Merging them" usi) (progressFL "Merging us" themi)

We switch from merge to merge2FL.

Notes with thanks to Petr:

- this involves flipping the order of arguments simply because the two
  functions have different conventions

- merge2FL should not be confused with mergeFL.  Merge2FL does this
  extra work of knocking out patches that share a patchinfo, which will
  be important in the future when we try to fix issue1014 (did anyone
  else that the new post 2008 Darcs team has been starting to wade into
  the core yet?)
  
- this actually has no effect because the patches with identical
  patchinfo will already been filtered out in findCommonAndUncommon
  But in future work, that may change...

The rest of this patch is just using hopefully because we're now
manipulating PatchInfoAnd (Hopefully captures the business of partial
repositories, if I understand correctly, Kudos to whoever haddocked it!
It made reading things a lot faster.)

> -     anonpend <- anonymous (fromPrims pend)
> +     anonpend <- n2pia `fmap` anonymous (fromPrims pend)
>       pend' :/\: pw <- return $ merge (pc :\/: anonpend :>: NilFL)

> -     let pwprim = joinPatches $ progressFL "Examining patches for conflicts" $ mapFL_FL patchcontents pw
> +     let pwprim = joinPatches $ progressFL "Examining patches for conflicts" $
> +                                mapFL_FL (patchcontents . hopefully) pw

> -     have_unrecorded_conflicts <- checkUnrecordedConflicts opts pc
> +     have_unrecorded_conflicts <- checkUnrecordedConflicts opts $ mapFL_FL hopefully pc

Extend the get test to cover --context interaction with tags.
-------------------------------------------------------------
> +cd temp1
> +darcs tag -m tt
> +echo x > x
> +darcs rec -lam "x" x
> +darcs changes --context > my_context
> +cd ..
> +
> +rm -rf temp2
> +darcs get temp1 --context="${abs_to_context}" temp2
> +darcs changes --context --repo temp2 > repo2_context
> +diff -u ${abs_to_context} repo2_context

Here we test for the case where the context file contains a tag (but not
as its most recent patch).  Seems to make sense.

Make partitionFL do a 3-way split, and detect commutation bugs in Depends.
--------------------------------------------------------------------------
I hope to review this after lunch or failing that, tomorrow.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100623/64b4716e/attachment.pgp>


More information about the darcs-users mailing list