Reinier Lamers <tux_roc...@reinier.de> added the comment:
Here's a review. I don't understand the code, but it works and it looks OK.
When you answer the questions below, I will apply it.
> [Rename findCommonAndUncommon to findUncommon (it does not find common).
> Petr Rockai <m...@mornfall.net>**20100804195738
This patch just does a lot of string replace, OK.
> [Resolve issue1873: give nicer error when apply fails due to missing
> patches. Petr Rockai <m...@mornfall.net>**20100804204010
>
> Ignore-this: b3ddfddeaa7e089717256aa2344ba78c
>
> ]
skipping some added imports:
> hunk ./src/Darcs/Commands/Apply.lhs 164
>
> if forwarded
>
> then exitWith ExitSuccess
> else fail er
>
> - (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
> + 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) []
> +
> + (us':\/:them') <- return $ findUncommon us them
> + (hadConflicts, Sealed their_ps) <- filterOutConflicts opts (reverseFL
> us') repository them'
>
> when hadConflicts $ putStrLn "Skipping some patches which would cause
> conflicts."
>
This is where the action is. Instead of using findUcommon to find the
patches that we have in common, and then using 'conscientiously' to see if
all patches in the bundle context are available, we now use
findCommonWithThem to find the patches we have in common and use a new
function 'check' to check if all the patches in the context of the bundle
are available.
I don't understand it.
Have the semantics of findUcommon changed, causing you to change the name to
findUncommon from findCommonAndUncommon? And does it tell us now only the
patches that we have and they haven't?
If that is the case, why doesn't the old way work anymore? We are sure we
have the "us'" patches (I suppose they're the ones we have and they don't),
but we're not sure about "them'". so we check the "them'" if they're
available. That should work, isn't it?
In the new 'check' function, we have a list of patches called 'required'
that is not in 'common' but is in 'them'. But if they're not common and they
are in their repository, that seems enough information to conclude that they
are not in our repository. Why do we still have to check them?
A good thing about this, by the way, is that the user now gets a complete
list of patches that he's missing. The old error just gave him the first
missing patch that darcs stumbled upon.
> hunk ./src/Darcs/Commands/Apply.lhs 183
> - when (nullFL their_ps_filtered) $
> + when (nullFL their_ps) $
>
> do putStr $ "All these patches have already been applied. " ++
>
> "Nothing to do.\n"
>
> exitWith ExitSuccess
>
This is just a variable rename.
> hunk ./src/Darcs/Commands/Apply.lhs 191
> - (to_be_applied :> _) <- runSelection (selector their_ps_filtered) context
> + (to_be_applied :> _) <- runSelection (selector their_ps) context
Another place where that variable is renamed.
> [Add test for issue1873 (failed to read patch during apply).
> Petr Rockai <m...@mornfall.net>**20100807163013
>
> Ignore-this: 2396ff7f429204f6f10079fb32799e32
>
> ] addfile ./tests/issue1873-apply-failed-to-read-patch.sh
> hunk ./tests/issue1873-apply-failed-to-read-patch.sh 1
> +#!/usr/bin/env bash
> +## issue1873: failed to read patch during apply
> +
> +. lib
> +
> +set -x
> +
> +rm -rf R
> +mkdir R
> +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
> +
> +not darcs apply --repo R x.dpatch 2>&1 | tee log
> +
> +not grep '^ \* d' log # does not complain about an unrelated patch
> + grep '^ \* y' log # complains about the offending one instead
Can you explain concisely why the simple test with three patches did not
give rise to the weird error message, but this test does?
Reinier
__________________________________
Darcs bug tracker <b...@darcs.net>
<http://bugs.darcs.net/patch327>
__________________________________
_______________________________________________
darcs-users mailing list
darcs-users@darcs.net
http://lists.osuosl.org/mailman/listinfo/darcs-users
