[darcs-users] [patch327] Resolve issue1873: give nicer error when apply failsdue tomissing

[Petr Rockai]

Hi,

Reinier Lamers <bugs at darcs.net> writes:
>> 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?

The semantics have changed back when I did NewSet. I just misnamed the
function, due to its similarity with get_common_and_uncommon. However,
findUncommon (as it should be properly called) only gives you the
"uncommon" part of get_common_and_uncommon: that is the bottom half of
the merge diamond between two PatchSet's.

I.e. the two sides of :\/: represent how you'd get from a common point
to either of the endpoints of the two original repositories. There's a
guarantee (currently) that there are no common patches interspersed into
this half-diamond.

> 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?

The problem is that to compute them', we may need to commute some
patches in "them" (because they are not common) and these patches may
actually not be available. This can happen if the patches in the
original sequences are interspersed (common v. uncommon).

> 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?

We have to check them, because we need to commute all those patches up
through some of our own patches. This would fail if we don't have the
actual patches. Normally, them' would exactly consist of "new patches"
from the bundle: when this is not the case, things can go awry. The old
code would work in some cases, but not in others -- depending on when
exactly we'd try to commute things.

If the commutes happened in findUncommon, we were screwed. The rest of
the code was fine already.

> 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.

Yeah, I like that too. :)

>> 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?

That's due to the commutation issue above: with just 3 patches, the
findUncommon commute was a no-op and the later code's been OK.

Yours,
   Petr.