There is no test for issue1922, as i'm hoping that someone who had it happen
for real can cook (or help me cook) a minimal failing example. I tested my fix
on the darcs darcs repository, and it works, in cases where darcs used to fail.
Any help on how to test more thoroughly will be much appreciated.

4 patches for repository http://darcs.net/screened:

Tue Nov  9 16:36:26 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
  * Remove redundant (:>>) type constructor

Tue Nov  9 17:47:48 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
  * allow rollback and unpull to work under tags
  
  Quick preliminary tests seem to work ok performance-wise,
  but obliterate -a is now a very bad idea indeed.

Tue Nov  9 18:04:08 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
  * exit faster on null selection in Unrecord

Tue Nov  9 22:36:33 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
  * resolve issue1922: output correct context in darcs unpull -O

Attachments

• remove-redundant-_____-type-constructor.dpatch
• unnamed

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

It seems that allowing unpull to work under tag is more detrimental to 
performance than I thought. I'm sending a followup.

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

Wed Nov 10 12:27:06 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
  * Limit unpull and rollback to last tag again

Attachments

• limit-unpull-and-rollback-to-last-tag-again.dpatch
• unnamed

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-1362af6e048fea00883c8c1f07414c3f845e1097

There must be a cleaner way, but this should do for now.

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

Thu Nov 11 14:37:10 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
  * Warn when running cabal test with -fno-test

Attachments

• unnamed
• warn-when-running-cabal-test-with-_fno_test.dpatch

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-855027b8a5bbd46777ec00d56ad81eff65d0d97a

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Le 11/11/2010 14:44, Florent Becker a écrit :
> 
> Florent Becker <florent.becker@ens-lyon.org> added the comment:
> 
> There must be a cleaner way, but this should do for now.
> 
> 1 patch for repository http://darcs.net/screened:
> 
> Thu Nov 11 14:37:10 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
>   * Warn when running cabal test with -fno-test
> 
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch458>
> __________________________________

Of course, this has nothing to do with patch 458. Please review
separately. Sorry for the mess.

Florent

- --
take 100 . vsep . repeat $ text "I should not use Ctrl-r in the shell"
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzb9VIACgkQTCPcDztjGo6khQCfTPEvsZofycaIZtnuZWRu/oV5
AeoAnjEQ48yz18dXsfUnCnpA4zoyfUpR
=fZmN
-----END PGP SIGNATURE-----

On Thu, 11 Nov 2010, Florent Becker wrote:

>> Thu Nov 11 14:37:10 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
>>   * Warn when running cabal test with -fno-test
>>
>> __________________________________
>> Darcs bug tracker <bugs@darcs.net>
>> <http://bugs.darcs.net/patch458>
>> __________________________________
>
> Of course, this has nothing to do with patch 458. Please review
> separately. Sorry for the mess.

I've resubmitted as patch463 so we can track it separately, and will 
delete the attachment from this patch.

Ganesh

I had accidentaly disabled unpulling the last tag. This is now ok, as is
unpull --match sth, when there is no match after the last tag. I'm not adding
tests for this right now, given that I plan to remove the "cannot unpull after
a tag" restriction soon.

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

Mon Nov 15 14:53:20 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
  * Reinstantiate matchingHead

Mon Nov 15 16:23:27 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
  * test for issue 1922

Attachments

• reinstantiate-matchinghead.dpatch
• unnamed

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

I'm pushing the removal of (:>>). Haven't looked at the rest.

Hi all,

On Tue, Nov 09, 2010 at 21:49:40 +0000, Florent Becker wrote:
> Tue Nov  9 17:47:48 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
>   * allow rollback and unpull to work under tags
>   
>   Quick preliminary tests seem to work ok performance-wise,
>   but obliterate -a is now a very bad idea indeed.

Bad idea in the UI sense and not the performance sense, I presume.  I'm
going to wait till Wednesday to apply this depending on what people say.

> Tue Nov  9 18:04:08 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
>   * exit faster on null selection in Unrecord

Going to try to push this now.

> Tue Nov  9 22:36:33 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
>   * resolve issue1922: output correct context in darcs unpull -O

This looks good, but I'm going to wait till I've had a look at the rest
of this bundle, ie. the tests for this issue.

Also, I wanted to get into the habit of writing a "if I wanted to write
test this code, how would I go about it" section to learn what a more
test-centric style of development would entail.  But it took me all day
to puzzle over the underlying Darcs code and do this review and I got
tired. :-(

It does seem to be something we (I?) missed in review.  What could we
have reasonably unit-tested for here?  Seems like the sort of thing
that's covered better by a functional test.

allow rollback and unpull to work under tags
--------------------------------------------
This appears to be related to http://bugs.darcs.net/issue1241

That issue asks for more discussion, but maybe we don't need one.  I'm
not worried about people disagreeing so much, as there being some
pitfall we haven't considered to making these commands look past tags by
default:

 - some kind of crazy performance implication?
 - third-party tools going subtly wrong?
 - UI mishaps of one sort or another due to old expectations?
   (obliterate -a is now double plus dangerous)

Anyway, this patch doesn't solve issue1241 and it doesn't actually seem
to change rollback's behaviour, but it seems like an improvement anyway.
I would love to understand issue1241 better.  All I think I get is that
in rollback file1, the restriction to touching files happens in
Darcs.SelectChanges... but since rollback seems to be giving all the
patches anyway, shouldn't it be working?

OLD BEHAVIOUR:

- darcs rollback (--match) works on all patches
  (at least from 2.3.1 and up)
- darcs rollback file1 stops at... not sure
  observed behaviour is at the latest tag, but things may be more
  subtle than that
- darcs obliterate (--match) stops at the tag before the first match
  (this is subtly different from how most people perceive it,
  but it degrades to the perceived case of stopping at the
  latest tag in the usual case)

NEW BEHAVIOUR:

- darcs rollback (--match): same
- darcs rollback file1...   same? [so this patch doesn't fix issue1241]
- NEW! darcs obliterate (--match) works on all patches

> Florent Becker <florent.becker@ens-lyon.org>**20101109164748
> ] hunk ./src/Darcs/Commands/Rollback.lhs 46
> - -  FlippedSeal patches <- return $ if firstMatch opts
> - -                                  then getLastPatches opts allpatches
> - -                                  else FlippedSeal $ newset2RL allpatches
> +  (_ :> patches) <- return $ if firstMatch opts
> +                             then getLastPatches opts allpatches
> +                             else (PatchSet NilRL NilRL):>newset2FL allpatches

No behaviour change here because rollback already works under tags
(with the exception of issue1241).

This is just catching the code up with changes in getLastPatches which
now incorporates the reversing to FL.  Any particular reason for that?
Is it just to be tidy or is there something deeper I'm missing?

> hunk ./src/Darcs/Commands/Rollback.lhs 111
> - -  (_ :> ps) <- runSelection (selectChanges LastReversed (reverseRL patches)) patches_context
> +  (_ :> ps) <- runSelection (selectChanges LastReversed patches) patches_context

I suppose we've merely moved the reverse to a different place
(getLastPatches or newset2FL)

> hunk ./src/Darcs/Commands/Unrecord.lhs 169
> - -  FlippedSeal patches <- return $ if firstMatch opts
> - -                                  then getLastPatches opts allpatches
> - -                                  else matchingHead opts allpatches


> +  (_ :> patches) <- return $ if firstMatch opts
> +                             then getLastPatches opts allpatches
> +                             else (PatchSet NilRL NilRL
> +                                        :> newset2FL allpatches)

Here's the interesting change, replacing matchingHead with
(PatchSet NilRL NilRL :> newset2FL allpatches)

The result is that unrecord will work on all patches, not just the
ones matchingHead returns (more on that later)

>  getLastPatches :: RepoPatch p => [DarcsFlag] -> PatchSet p C(Origin r)
> - -                 -> FlippedSeal (RL (PatchInfoAnd p)) C(r)
> +                 -> ((PatchSet p) :> (FL (PatchInfoAnd p))) C(Origin r)
>  getLastPatches opts ps =
>    case matchFirstPatchset opts ps of
> hunk ./src/Darcs/Commands/Unrecord.lhs 192
> - -  Sealed p1s -> case findCommonWithThem ps p1s of _ :> us -> FlippedSeal (reverseFL us)
> +  Sealed p1s -> findCommonWithThem ps p1s

"them" : all patches
"us:   : matching patches

I was going to ask why we would want to return the non-matching
patches (ie. the common ones), but it seems like this is important
for the issue1922 resolution

> - -  FlippedSeal patches <- return $ if firstMatch opts
> - -                                  then getLastPatches opts allpatches
> - -                                  else matchingHead opts allpatches
> +  (_ :> patches)
> +      <- return $ if firstMatch opts
> +                  then getLastPatches opts allpatches
> +                  else  (PatchSet NilRL NilRL
> +                         :> newset2FL allpatches)

Same change for obliterate that we made for unrecord earlier).

> - -  (kept :> removed) <- runSelection (selector (reverseRL patches)) context
> +  (kept :> removed) <- runSelection (selector patches) context

I think this is just avoiding double-reverse

> - -matchingHead :: RepoPatch p => [DarcsFlag] -> PatchSet p C(Origin r)
> - -             -> FlippedSeal (RL (PatchInfoAnd p)) C(r)
> - -matchingHead opts set@(PatchSet x _)
> - -    | or (mapRL (matchAPatchread opts) x) = contextPatches set
> - -matchingHead opts (PatchSet x (Tagged t _ ps :<: ts))
> - -    = (x +<+) `mapFlipped` matchingHead opts (PatchSet (t:<:ps) ts)
> - -matchingHead _ (PatchSet _ NilRL) = FlippedSeal NilRL
> - -

I took an interest in this function because I wanted to know why exactly
we were getting the stop at latest tag behaviour.  See attached
'matchingHead' thinking.  It's not super-relevant since it's code that's
going away.

exit faster on null selection in Unrecord
-----------------------------------------
> ] hunk ./src/Darcs/Commands/Unrecord.lhs 320
>    (kept :> removed) <- runSelection (selector patches) context
> +  when (nullFL removed) $ do putStrLn "No patches selected!"
> +                             exitWith ExitSuccess
>    case commute (effect removed :> pend) of
>      Nothing -> fail $ "Can't "++ cmdname ++
>                 " patch without reverting some unrecorded change."
> hunk ./src/Darcs/Commands/Unrecord.lhs 326
>      Just (_ :> p_after_pending) -> do
> - -        when (nullFL removed) $ do putStrLn "No patches selected!"
> - -                                   exitWith ExitSuccess

This seems sensible.  I'll see if I can push it right away.

resolve issue1922: output correct context in darcs unpull -O
------------------------------------------------------------
> ] hunk ./src/Darcs/Commands/Unrecord.lhs 50
>    (_ :> patches) <- return $ if firstMatch opts
>                               then getLastPatches opts allpatches
> -                             else (PatchSet NilRL NilRL
> +                             else (PatchSet NilRL NilRL 

Looks a  bit of a whitespace change snuck in there

> - -  (_ :> patches)
> +  (auto_kept :> removal_candidates)
>        <- return $ if firstMatch opts
>                    then getLastPatches opts allpatches

Ah-ha! so that's why we want those common (non-matching patches)

> -  (kept :> removed) <- runSelection (selector patches) context
> +  (kept :> removed) <- runSelection (selector removal_candidates) context

This is just propagating the rename from patches to removal_candidates

> -             savetoBundle opts (reverseFL kept) removed
> +             savetoBundle opts (auto_kept `appendPSFL` kept) removed

The naming here tells a story, which is that you have 3 kinds of patches

non-matching (auto-kept)
matching but not selected by user (removal_candidates, kept)
matching and selected by user (removal_candidates, removed)

In implementing obliterate -O, we only included the explicitly
unselected patches to bundle, but we forgot about the non-matchers.
This is kind of interesting.

It seems like the bundles we generated without this bugfix should not
have been *that* bogus if it's just a matter of the non-matching patches
being missing.  I mean, wrong is wrong, but if the non-matching (and
not-depended-on!) patches can be viewed as a suffix of the correct
context (due to commutation), then as a sanity check, it should be
possible to restore correctness by appending the non-matching patches,
or better, just the tag...

>  savetoBundle :: RepoPatch p => [DarcsFlag]
> -             -> RL (PatchInfoAnd p) C(x z) -> FL (PatchInfoAnd p) C(z t)
> +             -> PatchSet p C(Origin z) -> FL (PatchInfoAnd p) C(z t)
>               -> IO ()
>  savetoBundle opts kept removed@(x :>: _) = do
> hunk ./src/Darcs/Commands/Unrecord.lhs 345
> -    bundle <- makeBundle Nothing kept (mapFL_FL hopefully removed)
> +    bundle <- makeBundleN Nothing kept (mapFL_FL hopefully removed)

Instead of passing just the explicitly rejected patches as context,
we pass in now the whole (unselected) repo... and makeBundleN through
the makeBundle2 comments about lazily generated streams is smart enough
for this to not mean reading every single patch in our repo.

> hunk ./src/Darcs/Patch/Set.hs 29
> +appendPSFL :: PatchSet p C(start x) -> FL (PatchInfoAnd p) C(x y)
> +           -> PatchSet p C(start y)
> +appendPSFL (PatchSet ps ts) newps = PatchSet (reverseFL newps +<+ ps) ts
> +

Continue a PatchSet.  Does this sort of thing need haddock?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

Looks like it's OK for me to apply the whole bundle.

Hmm, I should have looked at the whole ticket before I started trying
to review the first bundle.

Anyway, I also realise that I forgot to attach my attempt at
understanding matchingHead... if I understand correctly, it turns out
that attempt wasn't totally wasted after all, since we're bringing it
back.  So I'll paste it in the bottom of my review.

>   * Limit unpull and rollback to last tag again

Hmm, OK so we restore stopping at the last tag for performance reasons.
I've filed issue2022 to document this fact

>   * Reinstantiate matchingHead

What's interesting about this patch is that it fixes a bug introduced in
the Limit unpull and rollback patch.  Notice also that patch458 includes
a patch that fixes a bug in obliterate -O.

That's two errors we've managed to introduce, and lots of working
discovering them (things breaking and users going huh?), diagnosing the
errors, writing patches, reviewing them, etc.

So that thing Iago was saying on IRC last night
  http://irclog.perlgeek.de/darcs/2010-12-17#i_3100404
resonates a bit, that by investing in catching errors up front, we save
ourselves a lot of work and end up getting things done faster.  This is
slightly weaker (ie. easier to believe) than Zooko's claim that TDD
specifically would make us go faster, but it's in the same general
direction.

So yeah, personally I don't need somebody telling me that testing is a
good thing.  What I need is concrete help forward.  For example, I'd be
curious to see how a test-centric developer have avoided running into
these "English sentence" errors from our line of work here, errors like

 - Oops, I overlooked the fact that the context for obliterate -O patch
   bundles includes not just the patches excluded via selection, but the
   implicitly excluded ones?

 - Oops, I overlooked the fact that obliterate ought to let you pick out
   the last tag (so that you have a way to get at the stuff behind it)?

The latter I imagine could have been caught with more aggressive
functional tests that try all sorts of obliterate --match options
with tagged repos. What about the first one?

> Mon Nov 15 16:23:27 CET 2010  Florent Becker <florent.becker@ens-lyon.org>
>   * test for issue 1922

----------------------------------------------------------------------
Limit unpull and rollback to last tag again
Reinstantiate matchingHead
----------------------------------------------------------------------
I'm reviewing these two patches as a single diff since they seem to undo
each other a bit.

> @@ -72,7 +72,7 @@
> -  FlippedSeal ps' <- contextPatches `fmap` readRepo repository
> +  (_ :> ps') <- contextPatches `fmap` readRepo repository

I think this changes in Changes is just a consequence of contextPatches
not having to forgo making statements about the starting point of its
sequences because it knows it starts from Origin.  Perhaps the
contextPatches thing should have been a standalone refactor?

> diff -rN -u old-review-2/src/Darcs/Commands/Rollback.lhs new-review-2-1/src/Darcs/Commands/Rollback.lhs
> -                             else (PatchSet NilRL NilRL):>newset2FL allpatches
> +                             else (PatchSet NilRL NilRL):> (newset2FL allpatches)

Nothing to see here.  So rollback is unchanged from where we left off
in my review of the first bundle.

> diff -rN -u old-review-2/src/Darcs/Commands/Unrecord.lhs new-review-2-1/src/Darcs/Commands/Unrecord.lhs
> -                             else (PatchSet NilRL NilRL 
> -                                        :> newset2FL allpatches)
> +                             else matchingHead opts allpatches

On the other hand, unrecord now goes back to the matchingHead behaviour
of old, meaning that we sort of stop at the lastest pre-match tag

> -  (auto_kept :> removal_candidates)
> -      <- return $ if firstMatch opts
> -                  then getLastPatches opts allpatches
> -                  else  (PatchSet NilRL NilRL 
> -                         :> newset2FL allpatches)
> +  (auto_kept :> removal_candidates) <- return $
> +                                        if firstMatch opts
> +                                        then getLastPatches opts allpatches
> +                                        else matchingHead opts allpatches

Likewise unpull/obliterate go back to the old behaviour.  This means
I can feel more confident about just pushing this whole bundle.


|  matchingHead :: RepoPatch p => [DarcsFlag] -> PatchSet p C(Origin r)
|               -> FlippedSeal (RL (PatchInfoAnd p)) C(r)
|  matchingHead opts set@(PatchSet x _)
|      | or (mapRL (matchAPatchread opts) x) = contextPatches set
|  matchingHead opts (PatchSet x (Tagged t _ ps :<: ts))
|      = (x +<+) `mapFlipped` matchingHead opts (PatchSet (t:<:ps) ts)
|  matchingHead _ (PatchSet _ NilRL) = FlippedSeal NilRL

Above is the original matchingHead

> +-- | matchingHead returns the repository up to some tag. The tag t is
> +-- the last tag such that there is a patch after t that is matched by
> +-- the user's query.
> +matchingHead :: forall p C(r). RepoPatch p =>
> +                [DarcsFlag] -> PatchSet p C(Origin r)
> +             -> (PatchSet p :> FL (PatchInfoAnd p)) C(Origin r)
> +matchingHead opts set =
> +    case mh set of
> +      (start :> patches) -> (start :> reverseRL patches)
> +    where
> +      mh :: FORALL(x) PatchSet p C(Origin x)
> +         -> (PatchSet p :> RL (PatchInfoAnd p)) C(Origin x)
> +      mh s@(PatchSet x _)
> +          | or (mapRL (matchAPatchread opts) x) = contextPatches s
> +      mh (PatchSet x (Tagged t _ ps :<: ts))
> +          = case mh (PatchSet (t:<:ps) ts)
> +            of (start :> patches) -> (start :> x +<+ patches)
> +      mh ps = (ps :> NilRL)

This appears to be the same as before but with the same sort of "oh
actually we're starting all the way from the origin" tightening up as
below.

One quibble is that because it doesn't explicitly mention the NilRL
case (and lets it be caught by a more general matcher), we've sightly
opened the window to bugs where we didn't exhaustively cover all the
cases (by accidentally subsuming something we forgot about under the
last case)

More on matchingHead below

> -contextPatches :: RepoPatch p => PatchSet p C(Origin x) -> FlippedSeal (RL (PatchInfoAnd p)) C(x)
> +contextPatches :: RepoPatch p => PatchSet p C(Origin x) ->
> +                  (PatchSet p :> (RL (PatchInfoAnd p))) C(Origin x)
>  contextPatches set = case slightlyOptimizePatchset set of
> -  PatchSet ps (Tagged t _ _ :<: _) -> flipSeal (ps +<+ (t :<: NilRL))
> -  PatchSet ps NilRL -> flipSeal ps
> +  PatchSet ps (Tagged t _ ps' :<: ts)
> +      -> (PatchSet ps' ts) :> (ps +<+ (t :<: NilRL))
> +  PatchSet ps NilRL -> (PatchSet NilRL NilRL :> ps)

I think this was just a tightening up?

matchingHead
======================================================================

Background: Backpointers vs capping tags
----------------------------------------
Each inventory file contains

  I.  name and link to previous tag (if applicable)
  II. patches in this set

In forward order you might get repositories that look like this:

_darcs/inventories/0000 [convenient hashes, huh?]
  foo-first-edit
  bar
_darcs/inventories/0001
  t1 => 0000
  foo-second-edit
_darcs/inventories/0002
  t2 => 0001
  baz
  quux
_darcs/hashed_inventory
  t3 => 0002
  toto

So the representation on disk is based on back-pointers.  On the other
hand, for the internal representation, we don't use back-pointers so
much as capping tags.  In other words,

 on disk (back-pointer):   x1 x2    | t1 x3    | t2 x4 x5    | t3
 internally (capping tag): x1 x2 t1 |    x3 t2 |    x4 x5 t3

They're both sensible ways of doing things and it's easy to see how
they're fundamentally the same thing, but if you're like me, it can be
easy to forget this distinction and get confused.

I don't know what to call the little mini sets of patches plus their capping
tag (eg. x1 x2 t1), so I'll use the word 'clump' for now.  I'll also call the
topmost one without capping tag (corresponding to _darcs/inventory) the
"surface clump".  Except for the surface, we use the type Tagged to refer to
clumps.

matchingHead
------------
|  matchingHead :: RepoPatch p => [DarcsFlag] -> PatchSet p C(Origin r)
|               -> FlippedSeal (RL (PatchInfoAnd p)) C(r)
|  matchingHead opts set@(PatchSet x _)
|      | or (mapRL (matchAPatchread opts) x) = contextPatches set
|  matchingHead opts (PatchSet x (Tagged t _ ps :<: ts))
|      = (x +<+) `mapFlipped` matchingHead opts (PatchSet (t:<:ps) ts)
|  matchingHead _ (PatchSet _ NilRL) = FlippedSeal NilRL

Three things to notice about this function:

1. We search one clump of patches at a time. Except for the surface
   level, each set includes its capping tag.

2. We search all the way down to the first clump that contains
   a match.

   If you don't use a matcher, all patches trivially match,
   so we never look past the surface clump.  This is why
   obliterate -a never seems to go past the latest tag.

   On the other hand, if for example we are looking for files that touch
   'foo', we would stop at the clump t2 (which containts
   foo-second-edit).  So one mystery would be why doesn't obliterate
   --match 'touch foo' return the first match?  Why does it also appear
   to stop at the latest tag?

3. Because we perform slightlyOptimizePatchset on the match
   (via contextPatches) and return the patches that aren't
   behind the earliest tag in the clump.  I'm still not entirely
   sure why we do this, but I can say that if the match is already
   a Tagged clump, this means all the patches are behind a tag
   and so they get dropped.  I think I still slightly misunderstand
   slightlyOptimizePatchset.

In other words, both obliterate and obliterate --match seem to stop
at the latest tag, but they do so for quite different reasons!

test for issue 1922
-------------------
Yay tests! Now that we have test for issue1922, I guess I can apply
all of this.

> hunk ./tests/issue1922-obliterate-o-context.sh 1
> +. lib                           # Load some portability helpers.
> +darcs init      --repo R        # Create our test repos.
> +
> +rm -rf x
> +mkdir x
> +darcs init --repo x
> +echo a > x/a
> +darcs rec -lam a --repo x --ig
> +echo b > x/a
> +darcs rec -lam b --repo x --ig
> +echo a > x/b
> +darcs rec -lam a2 --repo x --ig
> +echo b > x/b
> +darcs rec -lam b2 --repo x --ig
> +echo c > x/a
> +darcs rec -lam c --repo x --ig

IMHO, by using cd repo and forgoing --ig (covered by .darcs/defaults in
the harness), you get a less noisy test...

  cd x
  echo a > a
  darcs rec -lam a
  echo b > a
  darcs rec -lam b
  echo a > b
  darcs rec -lam a2
  echo b > b
  darcs rec -lam b2
  echo c > a
  darcs rec -lam c

> +rm -f foo.dpatch
> +darcs unpull -a -o foo.dpatch --match 'name a2' --repo x --last 3
> +cat foo.dpatch
> +darcs chan --context --repo x
> +darcs apply foo.dpatch --repo x

OK, we're testing that we can apply the patch bundle we created, but is
that really informative?  Do we need to test for not being able to apply
something (or is that silly?) Would we benefit from counting patches
before and after?  Would it be worthwhile to diff the bundles that would
be generated by darcs send and by obliterate -O?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)

This patch bundle (with 4 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-d4694618d0a85c76582d864c893611c913332ad7

This patch bundle (with 1 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-1362af6e048fea00883c8c1f07414c3f845e1097

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-3dc42c7c009ea939d84278784707d408a5642a5f

This patch bundle (with 1 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-1362af6e048fea00883c8c1f07414c3f845e1097

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-3dc42c7c009ea939d84278784707d408a5642a5f

This patch bundle (with 4 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-d4694618d0a85c76582d864c893611c913332ad7

Hi Florent,

Do you happen to remember what was the performance problem you ran 
into? I'm just thinking about having another go at making unpull etc 
work under tags.

Cheers,

Ganesh