Hi,

this is sort of a draft patch. It will need careful review and a bunch of
testing. I am sending it now in the hope that the people that ran into the
issue will be able to test it. Also, it would be nice, if someone could get us
a regression test for the said issue (it shouldn't be hard: make a repo R with
patches a b c, repo S with patches a b, send -O from R to S, make repo T with
just patch a and try to apply the bundle in T).

A good way to fix that "\\" in there would be also welcome.

Yours,
   Petr.

2 patches for repository http://darcs.net/:

Wed Aug  4 21:57:38 CEST 2010  Petr Rockai <me@mornfall.net>
  * Rename findCommonAndUncommon to findUncommon (it does not find common).

Wed Aug  4 22:40:10 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1873: give nicer error when apply fails due to missing patches.

Attachments

• rename-findcommonanduncommon-to-finduncommon-_it-does-not-find-common__.dpatch
• unnamed

On Wed, Aug 04, 2010 at 20:48:49 +0000, Petr Ročkai wrote:
> (it shouldn't be hard: make a repo R with
> patches a b c, repo S with patches a b, send -O from R to S, make repo T with
> just patch a and try to apply the bundle in T).

Hmm, that's what I tried, but it seems like the complaint in my
rendition of your test (with repo names backwards, sorry!) is sane...
any thoughts on how to make it give the sort of insane error that we
were complaining about?

Or did I just do the test wrong?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, please try +44 (0)1273 64 2905.

Attachments

• failing-issue1873-apply-error.sh

Hi all,

The good news is, this patch fixes issue1873 for me. The bad news is that I, 
too, can't make a test for issue1873.

Op zaterdag 07 augustus 2010 12:03 schreef Eric Kow:
> On Wed, Aug 04, 2010 at 20:48:49 +0000, Petr Ročkai wrote:
> > (it shouldn't be hard: make a repo R with
> > patches a b c, repo S with patches a b, send -O from R to S, make repo T 
with
> > just patch a and try to apply the bundle in T).
> 
> Hmm, that's what I tried, but it seems like the complaint in my
> rendition of your test (with repo names backwards, sorry!) is sane...

I had the same problem. I even tried to insert tags between the patches as 
they are in my repos that exhibit the problem, to no avail.

When I try to apply this bundle to the 2.5 branch though, I run into issue 
1873 again. Perhaps the bundles have to be made by Petr for this issue to 
occur :)

Reinier

Now also with a regression test.

Yours,
   Petr.

3 patches for repository http://darcs.net/:

Wed Aug  4 21:57:38 CEST 2010  Petr Rockai <me@mornfall.net>
  * Rename findCommonAndUncommon to findUncommon (it does not find common).

Wed Aug  4 22:40:10 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1873: give nicer error when apply fails due to missing patches.

Sat Aug  7 18:30:13 CEST 2010  Petr Rockai <me@mornfall.net>
  * Add test for issue1873 (failed to read patch during apply).

Attachments

• rename-findcommonanduncommon-to-finduncommon-_it-does-not-find-common__.dpatch
• unnamed

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 <me@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 <me@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 <me@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

Hi,

Reinier Lamers <bugs@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.

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 <me@mornfall.net>
>   * Rename findCommonAndUncommon to findUncommon (it does not find common).
> 
> Wed Aug  4 22:40:10 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Resolve issue1873: give nicer error when apply fails due to missing patches.
> 
> Sat Aug  7 18:30:13 CEST 2010  Petr Rockai <me@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.

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-73eeccb657863b44d2bed4d45d31f3782c335797

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-9977dd1a64f7dfbafedf0cfb4a2500439f3eaae0

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-73eeccb657863b44d2bed4d45d31f3782c335797

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-9977dd1a64f7dfbafedf0cfb4a2500439f3eaae0