Hi,

this does away with a [PatchInfo] parameter to makeBundle, which was quite
dangerous and accounted for two bugs in bundle creation. The "darcs fetch"
bundles would not break off at a tag and both darcs fetch and darcs obliterate
-o would create bundles with reversed (FL) contexts (oldest patch first).

Applying such busted patches would hang darcs apply if the repository was a
little bigger (it certainly hangs in darcs darcs repo).

Testing coverage would be welcome, but I currently don't have time to provide
it.

Yours,
   Petr.

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

Thu Jul 15 10:19:08 CEST 2010  Petr Rockai <me@mornfall.net>
  * Remove [DarcsFlag] usage from Darcs.Patch.Bundle.

Thu Jul 15 11:38:42 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1892: Improve safety of makeBundle* and fix a couple of related bugs.

Attachments

• remove-_darcsflag_-usage-from-darcs_patch_bundle_.dpatch
• unnamed

Hi Florent, would you be the right person to review this bundle?

Hi Florent, may I chase up on this?  Thanks!

> Message du 25/07/10 23:56
> De : "Eric Kow" 
> A : darcs-users@darcs.net, florent.becker@ens-lyon.org, kowey@darcs.net, me@mornfall.net
> Copie à : 
> Objet : [patch305] Resolve issue1892: Improve safety of makeBundle* and fix a couple of
>
> 
> 
> Eric Kow  added the comment:
> 
> Hi Florent, may I chase up on this? Thanks!
> 
Sorry,

I am currently away on holiday, for one more week.

Florent

Une messagerie gratuite, garantie à vie et des services en plus, ça vous tente ?
Je crée ma boîte mail www.laposte.net

Ah, no problem! I'll take over when I have a round tuit

May I sneak in a reminder that the associated bug is marked as a 2.5 
blocker? Thanks!

On Thu, Jul 15, 2010 at 09:39:32 +0000, Petr Ročkai wrote:
> Thu Jul 15 10:19:08 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Remove [DarcsFlag] usage from Darcs.Patch.Bundle.

Looks like this was applied from another bundle.

> Thu Jul 15 11:38:42 CEST 2010  Petr Rockai <me@mornfall.net>
>   * Resolve issue1892: Improve safety of makeBundle* and fix a couple of related bugs.

The bug in question confusion on the order that patch bundle code
expects and returns patches.

This patch uses directed lists to avoid these kinds of bugs in the
future.

I guess there's no reason for me not to apply this patch, but let's
wait for your explanation on fetchPatches/makeBundle so that I'm up
to speed too.  Some day I'll get this stuff...

Resolve issue1892: Improve safety of makeBundle* and fix a couple of related bugs.
----------------------------------------------------------------------------------
The core part of this patch seems fairly cut and dry

My main stumbling block in reviewing this patch was sheer unfamiliarity
(embarrassing!) with the witnesses and also a bit with newset.

General comments:

 * I wish Darcs.Patch.Set.PatchSet was haddocked.

Darcs.Patch.Bundle
~~~~~~~~~~~~~~~~~~
Would there be any use in having a witnessesed PatchInfo?

> -    makeBundle2 the_s (mapRL info ps ++ [info t]) to_be_sent to_be_sent
> +    makeBundle2 the_s (ps +<+ (t :<: NilRL)) to_be_sent to_be_sent

QUESTION: do functions like newset2RL do what you want here, or is that
something totally different?

> -    makeBundle2 the_s (mapRL info ps) to_be_sent to_be_sent
> +    makeBundle2 the_s ps to_be_sent to_be_sent

We avoid losing the fact that the PatchInfo being passed to makeBundle2
are in RL order.  There are no witnesses associated with PatchInfo so we
just pass the patches.

> hunk ./src/Darcs/Patch/Bundle.hs 64
> -makeBundle :: RepoPatch p => Maybe (Tree IO) -> [PatchInfo] -> FL (Named p) C(x y) -> IO Doc
> +makeBundle :: RepoPatch p => Maybe (Tree IO) -> RL (PatchInfoAnd p) C(start x)
> +              -> FL (Named p) C(x y) -> IO Doc

makeBundle just thinly wraps around makeBundle2 which now demands
PatchInfoAnd and specifies RL

> -makeBundle2 :: RepoPatch p => Maybe (Tree IO) -> [PatchInfo]
> +makeBundle2 :: RepoPatch p => Maybe (Tree IO) -> RL (PatchInfoAnd p) C(start x)
>               -> FL (Named p) C(x y) -> FL (Named p) C(x y) -> IO Doc
> hunk ./src/Darcs/Patch/Bundle.hs 75
> -makeBundle2 the_s common to_be_sent to_be_sent2 =
> +makeBundle2 the_s common' to_be_sent to_be_sent2 =

> +          common = mapRL info common'

Again, not much to see.  What we really want to use is the patch info
for the context file, but in order to make use of witnesses we postpone
losing the patches until the last minute.

Darcs.Commands.Pull
~~~~~~~~~~~~~~~~~~~
>  fetchPatches :: FORALL(p r u t) (RepoPatch p) => [DarcsFlag] -> [String] -> String ->
>                 Repository p C(r u r) ->
> -                   IO ( [PatchInfo]               , Sealed ((FL (PatchInfoAnd p)  :\/: FL (PatchInfoAnd p)) C(r)))
> +                   IO ( SealedPatchSet p C(Origin), Sealed ((FL (PatchInfoAnd p)  :\/: FL (PatchInfoAnd p)) C(r)))

I've taken the liberty of playing with whitespace to make the actual
change clear.

I have to rediscover this stuff over and over and over again
due to not using it frequently enough

 * (:\/:) opens the merge diamond (read bottom up)
 *
     data (a1 :\/: a2) C(x y) = FORALL(z) (a1 C(z x)) :\/: (a2 C(z y))

   The witnesses x y denote the individual end states for the two paths.
   We don't track the common starting state but we do say it has to be
   the same for both.  Quite useful to be reminded that when you see
   pairs of witnesses, they don't necessarily talk about before/after.

 * Sealed (us :\/: them)
   We agree to lose the token for them's ending state.

 * NEW: Data.Patch.Set.Origin is just data Origin
   type with no data constructor, so it really is just a universal
   witness to mean the origin, zero patches.
 
 * NEW: SealedPatchSet p C(Origin): patches common to the two
   repositories start from Origin.  We lose the token for the
   intermediate state after common and before opening the diamond

OK, that's only a very superficial reading because I'm still not 100%
comfortable working the witnesses. 

> -  common' :>> _ <- return $ findCommonWithThem us them
> -  let common = mapFL info $ newset2FL common'
> +  common :>> _ <- return $ findCommonWithThem us them

We no longer need to extract patch info as this is done closer
to the core

> -  return (common, seal $ us' :\/: to_be_pulled)
> +  return (seal common, seal $ us' :\/: to_be_pulled)

Still don't really get why we're sealing but OK

>  applyPatches ::
> -    forall p C(r u t). (RepoPatch p) => [DarcsFlag] -> Repository p C(r u r) ->
> +    forall p C(r u t a). (RepoPatch p) => [DarcsFlag] -> Repository p C(r u r) ->

QUESTION: Does the extra a serve some kind of purpose here?  I'm guessing it just
snuck in by accident?

> -    ([PatchInfo], Sealed ((FL (PatchInfoAnd p) :\/: FL (PatchInfoAnd p)) C(r)))
> +    (SealedPatchSet p C(Origin), Sealed ((FL (PatchInfoAnd p) :\/: FL (PatchInfoAnd p)) C(r)))
>      -> IO ()

Keeping in mind that pull is fetchPatches .. >>= applyPatches ..
applyPatches doesn't care about common so no need to change anything
else

>  makeBundle ::
> -    forall p C(r u t) . (RepoPatch p) => [DarcsFlag] ->
> +    forall p C(r u t x) . (RepoPatch p) => [DarcsFlag] ->

QUESTION: Same question about the 'x'

> -    ([PatchInfo],                Sealed ((FL (PatchInfoAnd p) :\/: FL (PatchInfoAnd p)) C(r)))
> +    (SealedPatchSet p C(Origin), Sealed ((FL (PatchInfoAnd p) :\/: FL (PatchInfoAnd p)) C(r)))

Whitespace jiggled for clarity.

> hunk ./src/Darcs/Commands/Pull.lhs 264
> -makeBundle opts (common,        Sealed (_ :\/: to_be_fetched)) =
> +makeBundle opts (Sealed common, Sealed (_ :\/: to_be_fetched)) =

> -      bundle <- PatchBundle.makeBundle Nothing common $
> +      bundle <-             makeBundleN Nothing (unsafeCoercePEnd common) $

OK, so this makeBundle is used for darcs fetch.

It looks like we are using makeBundleN instead of makeBundle because we
no longer smoosh the patchset (see newset2FL non-use and definition
above); that work is now delegated to makeBundleN (see question above).

The unsafeCoercePEnd was something slightly uncomfortable for me (in the
sense of being still a bit new territory for me).

My understanding is that it's because we've sealed off ending point of
the common patches, so when we unseal it the witness for the ending
point is an eigenvariable which does not unify with anything else, as
explained by Jason in http://wiki.darcs.net/DarcsInternals#type-witnesses 
In makeBundleN, however, we require that the common patches end in the
same place as the to-be-sent patches begin.

QUESTION: So then there's the question of why we have to seal off the
ending state of the common patches in fetchPatches.  Shouldn't we be
able to keep track of the fact that the common end in the same place as
the us/them patches begin?

As an aside, reviewing this patch is forcing me to look a little bit at
the NewSet code, one of the indirect benefits of the patch review
process... slowly learning the Darcs code.

As a second aside, I'm a little bit curious about how newset copes with
parallel tags (saying resulting from a merge of two repos their own
tags).  I'm guessing it's a non-issue and that the code makes no
particular assumptions about how one tag states lead to another (and the
representation is the same Darcs sequence of patches one anyway)

> hunk ./src/Darcs/Commands/Pull.lhs 269
> -                    (x:>:_)-> PatchBundle.patchFilename $ patchDesc x
> +                    (x:>:_)-> patchFilename $ patchDesc x

No need to qualify the import because we tightened it up in the imports
list (excluded from this review)

Darcs.Commands.Put
~~~~~~~~~~~~~~~~~~
> -  bundle <- makeBundle2 Nothing [] patches patches2
> +  bundle <- makeBundle2 Nothing NilRL patches patches2

Boring change

Darcs.Commands.Unrecord
~~~~~~~~~~~~~~~~~~~~~~~
>  savetoBundle opts kept removed@(x :>: _) = do
> hunk ./src/Darcs/Commands/Unrecord.lhs 347
> -    bundle <- makeBundle Nothing (mapFL info kept)
> -              (mapFL_FL hopefully removed)
> +    bundle <- makeBundle Nothing kept (mapFL_FL hopefully removed)

Boring change

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

On Thu, Aug 05, 2010 at 11:50:24 +0000, Eric Kow wrote:
> > Thu Jul 15 11:38:42 CEST 2010  Petr Rockai <me@mornfall.net>
> >   * Resolve issue1892: Improve safety of makeBundle* and fix a couple of related bugs.

Got some feedback on IRC from Petr, so I'll apply this.

> QUESTION: So then there's the question of why we have to seal off the
> ending state of the common patches in fetchPatches.  Shouldn't we be
> able to keep track of the fact that the common end in the same place as
> the us/them patches begin?

From http://irclog.perlgeek.de/darcs/2010-08-05#i_2667135

kowey: mornfall: a bit of patch305 went over my head
       could you explain why we need to seal off the ending state of common in
       fetchPatches?
       I think I understand that the unsafeCoerceEndP in makeBundle
       (Darcs.Commands.Pull) is just a straightforward consequence of the
       sealing off

...

mornfall: kowey: I think the answer here is that the :\/: has no start state,
          so the end state of the stuff before :\/: won't be very useful anyway.
          :\/: works as a seal for the start states IIUIC
          So I think that's why I sealed the endstate of common (since there's
          nothing to pair it up with...)

kowey: I think I still have trouble reading and interpreting
          data (a1 :\/: a2) C(x y) = FORALL(z) (a1 C(z x)) :\/: (a2 C(z y))
        I mean, even if we knock out the macros:
          data (a1 :\/: a2) cX cY = forall cZ . (a1 cZ cX) :\/: (a2 cZ cY)

[a few moments later]

kowey: oh ok, actually maybe I don't have any issue reading that
       so it's just
          (:\/:) :: forall cZ . a1 cZ cX -> a2 cZ cY -> (a1 :\/: a2) cX cY
       and like you're saying, no start state... that sound right?

mornfall: Yes.

• kowey buys it...

Eric

(I think I find GADT-style syntax less confusing than traditional because
 it involves me doing fewer substitutions in my head...)

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

Eric Kow <kowey@darcs.net> writes:

>> -    makeBundle2 the_s (mapRL info ps ++ [info t]) to_be_sent to_be_sent
>> +    makeBundle2 the_s (ps +<+ (t :<: NilRL)) to_be_sent to_be_sent
>
> QUESTION: do functions like newset2RL do what you want here, or is that
> something totally different?
Not in this case, since here we cut off everything after the first clean
tag. With newset2RL, we would include all of the repo history in the RL.

>>  applyPatches ::
>> -    forall p C(r u t). (RepoPatch p) => [DarcsFlag] -> Repository p C(r u r) ->
>> +    forall p C(r u t a). (RepoPatch p) => [DarcsFlag] -> Repository p C(r u r) ->
>
> QUESTION: Does the extra a serve some kind of purpose here?  I'm guessing it just
> snuck in by accident?
Uh oh. An accident. Let me see if I can amend those away.

>>  makeBundle ::
>> -    forall p C(r u t) . (RepoPatch p) => [DarcsFlag] ->
>> +    forall p C(r u t x) . (RepoPatch p) => [DarcsFlag] ->
>
> QUESTION: Same question about the 'x'
Same as above. (I guess this was from the time before I sealed off the
end-state of common.)

> QUESTION: So then there's the question of why we have to seal off the
> ending state of the common patches in fetchPatches.  Shouldn't we be
> able to keep track of the fact that the common end in the same place as
> the us/them patches begin?
I don't think the type of :\/: allows us to keep track of that.

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

Amended away those extra forall'd type variables. Unchanged otherwise.

1 patch for repository http://darcs.net/:

Thu Aug  5 14:23:46 CEST 2010  Petr Rockai <me@mornfall.net>
  * Resolve issue1892: Improve safety of makeBundle* and fix a couple of related bugs.

Attachments

• resolve-issue1892_-improve-safety-of-makebundle_-and-fix-a-couple-of-related-bugs_.dpatch
• unnamed

Whoops, sorry Petr, didn't pre-empt any sort of duplicate concurrent
work (I pushed the patch just as you were amending it).  Please feel
free to just push the amendment as it's quite trivial.  Next time in a
similar situation I should try to say something like "I'm pushing now"

Eric Kow wrote:
>> +          common = mapRL info common'
> 
> Again, not much to see.  What we really want to use is the patch info
> for the context file, but in order to make use of witnesses we
> postpone losing the patches until the last minute.  

I think I added a WPatchInfo type that is just a PatchInfo wrapped with witnesses. That might provide a way to make this code better.

Ganesh

=============================================================================== 
Please access the attached hyperlink for an important electronic communications disclaimer: 
http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html 
===============================================================================

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