I wrote this at the same time Reinier was reviewing the patch... On Sat, Aug 07, 2010 at 16:27:54 +0000, Petr Ročkai wrote: > Now also with a regression test.
Will be pushing this along with my restyling to verify that it
worked correctly in darcs 2.4
> Wed Aug 4 21:57:38 CEST 2010 Petr Rockai <m...@mornfall.net>
> * Rename findCommonAndUncommon to findUncommon (it does not find common).
>
> Wed Aug 4 22:40:10 CEST 2010 Petr Rockai <m...@mornfall.net>
> * Resolve issue1873: give nicer error when apply fails due to missing
> patches.
>
> Sat Aug 7 18:30:13 CEST 2010 Petr Rockai <m...@mornfall.net>
> * Add test for issue1873 (failed to read patch during apply).
Rename findCommonAndUncommon to findUncommon (it does not find common).
-----------------------------------------------------------------------
> -import Darcs.Patch.Depends ( findCommonAndUncommon )
> +import Darcs.Patch.Depends ( findUncommon )
Seemingly little to see here. I confirm that this function does seem
to just return an us :\/: them
These could have been replace patches for easier review and commutation
since it's just a straightforward renaming and not some subtle semantic
jiggle.
Resolve issue1873: give nicer error when apply fails due to missing patches.
----------------------------------------------------------------------------
> - (us':\/:them') <- return $ findUncommon us them -- FIXME handling missing
> patches : - (
> - let their_ps = mapFL_FL (n2pia . conscientiously (text ("We cannot apply
> this patch "
> - ++"bundle, since
> we're missing:") $$))
> - them'
> - (hadConflicts, Sealed their_ps_filtered) <- filterOutConflicts opts
> (reverseFL us') repository their_ps
So previously we were relying on on findUncommon to tell us what patches
are in them (new patches + their context) but are not also in us. The
idea was to reuse the Hopefully machinery for handling partial repos,
basically try to read the patches but complain if they can't be found.
In the case of issue1873, this code never fired. We only got a
complaint later on when trying to actually use the patch (where exactly)
The example where this doesn't quite work on is when
us: a b c d
them: a b c y d (x)
Here x is the new patch we want to apply. So
* expected behaviour : Darcs complains that 'y' is missing
* actual behaviour : Darcs complains that 'd' is missing
(and with a different error message!)
So why do we fail to complain about 'y' here?
The trick is that findUncommon works by doing two sets of commutation.
Using the patch infos to determine commonality,
we commute us into _common1 :>> us'
and them into _common2 :>> them'
returning us' and them' as the uncommon patches.
Do you see how this could go wrong? The problem is that the patch d
happens to be behind the new patch x. Fair enough, we just have to
commute it out as part of the common set, right? But wait, we can't!
We don't have them's version of d! And so we explode, confusingly
complaining that the patch d is missing (even though it's actually a
common patch).
So the reason we don't complain about 'y' is that we never even get
around to it.
We need to introduce a different, bundle-sensitive mechanism for
checking commonality.
> + common :>> ours <- return $ findCommonWithThem us them
> +
> + -- all patches that are in "them" and not in "common" need to be
> available; check that
> + let common_i = mapRL info $ newset2RL common
> + them_i = mapRL info $ newset2RL them
> + required = them_i \\ common_i -- FIXME quadratic?
> + check :: RL (PatchInfoAnd p) C(x y) -> [PatchInfo] -> IO ()
> + check (p :<: ps) bad = case hopefullyM p of
> + Nothing | info p `elem` required -> check ps (info p : bad)
> + _ -> check ps bad
> + check NilRL [] = return ()
> + check NilRL bad = fail . renderString $ vcat $ map humanFriendly bad ++
> + [ text "\nFATAL: Cannot apply this bundle. We are
> missing the above patches." ]
> +
> + check (newset2RL them) []
And this is it. Well, the first part of finding the uncommon patches is
as before, we just commute the common us patches out. As we saw above,
commuting common "them" patches out can't work if they're stuck behind
uncommon patches.
The new check is kind of stupid: since we already know what the common
patches are from the first direction, we simply walk the list of them
patches and compare patch infos. Any common patches we can't read are
marked as bad.
BTW: Isn't FATAL a little bit too scary sounding?
> + (us':\/:them') <- return $ findUncommon us them
> + (hadConflicts, Sealed their_ps) <- filterOutConflicts opts (reverseFL us')
> repository them'
The rest of this is just percolating the their_ps_filtered to their_ps
rename.
Add test for issue1873 (failed to read patch during apply).
-----------------------------------------------------------
> +darcs init --repo R
> +echo a > R/a
> +darcs rec -lam a --repo R --ignore-times
> +echo b > R/a
> +darcs rec -lam b --repo R --ignore-times
> +echo x > R/x
> +darcs rec -lam x --repo R --ignore-times
> +echo c > R/a
> +darcs rec -lam c --repo R --ignore-times
> +echo y > R/y
> +darcs rec -lam y --repo R --ignore-times
> +echo d > R/a
> +darcs rec -lam d --repo R --ignore-times
> +
> +darcs unpull -p x -a --repo R -O
> +darcs unpull -p y -a --repo R
The test above describes the scenario in my description of Petr's main
patch. I've tested in succeeds in Darcs 2.5, fails in HEAD and succeeds
with Petr's patch.
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list darcs-users@darcs.net http://lists.osuosl.org/mailman/listinfo/darcs-users
