Re-sending in a separate bundle for review.

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

patch 94ce414ef9af423b77da1e6f4fbe8d434f3091b4
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Sat Feb  7 14:37:23 CET 2015
  * simplified HashedRepo.removeFromTentativeInventory
  
  The change follows the outline indicated in the FIXME comment: we let
  writeTentativeInventory do the heavy lifting. Also clearly documented the
  preconditions, since this is a partial function.
  
  With this change, Darcs.Patch.Depends.commuteToEnd is no longert needed and
  has been removed.

Attachments

• patch-preview.txt
• simplified-hashedrepo_removefromtentativeinventory.dpatch
• unnamed

This basically replaces the call to commuteToEnd and then the local cutInv followed by 
removeFromPatchSet.

The only possible issue I can see is one of efficiency. removeFromPatchSet calls 
fastRemoveRL whereas commuteToEnd calls removeRL.

'removeRL x xs' is implemented by pulling each patch in xs to the head and then comparing 
it with x. I think even with laziness this is going to be significantly less efficient 
than fastRemoveRL which has a rather more direct implementation.

I can't see any reason not to replace the implementation of removeRL (and its friend 
removeFL) though, so I'll do that in a follow-up.

Have you, perhaps, done this (replace the implementation of removeRL
with the one for fastRemoveRL)? Because I cannot find fastRemoveRL in
the sources any longer. If yes, is the (my) patch accepted?

Oops, I see now that my patch removes the fastRemoveRL function because
it is no longer used. I will take a look to see if I understand the
difference between fast and non-fast variants enough to make the change
you proposed.

I haven't done this yet. I did briefly think about it the other day 
then got sidetracked by :<

As far as I could tell there's no upside to the removeFL/removeRL 
implementation - they're just inefficient.

I'll update this thread if I do find a moment to work on it again.

Hi Ganesh, don't bother. There is no better way to learn things than by
trying to do them yourself. I spent some time today trying to re-shape
removeRL along the lines of fastRemoveRL until I finally realized why
this is impossible and there are two different functions for a reason:

The trick with fastRemoveRL is that it relies on the meta data
(PatchInfo) for the equality test. This makes sense  because meta data
is context independent and thus never changes when patches are commuted!
But removeRL works on all patch types (i.e. including the anonymous
patches that a named patch consists of). Thus removeRL must compare the
actual patch data and this only makes sense if the contexts match up (on
either the left or the right side), so it has no other choice than to
commute every patch in the sequence to the end before testing for
equality. This is of course slower, but that really can't be helped.

So...

I will send a follow-up patch I made that re-adds fastRemoveRL and then
uses it inside removeFromPatchSet (with a local helper named
fastRemoveSubsequenceRL).

About the naming: "fast" isn't nice (why is is faster? why not always
use the faster version?). So I have been thinking about a re-factoring
that includes these functions in one of the classes (Commute?) so we
would automatically get the (faster) specialization when using removeRL
on a (PatchInfoAnd p).

On 15/06/2015 20:51, Ben Franksen wrote:
> 
> Ben Franksen <benjamin.franksen@helmholtz-berlin.de> added the comment:
> 
> Hi Ganesh, don't bother. There is no better way to learn things than by
> trying to do them yourself. I spent some time today trying to re-shape
> removeRL along the lines of fastRemoveRL until I finally realized why
> this is impossible and there are two different functions for a reason:
> 
> The trick with fastRemoveRL is that it relies on the meta data
> (PatchInfo) for the equality test. This makes sense  because meta data
> is context independent and thus never changes when patches are commuted!
> But removeRL works on all patch types (i.e. including the anonymous
> patches that a named patch consists of). Thus removeRL must compare the
> actual patch data and this only makes sense if the contexts match up (on
> either the left or the right side), so it has no other choice than to
> commute every patch in the sequence to the end before testing for
> equality. This is of course slower, but that really can't be helped.

Doh! I guess I would also have discovered that the hard way...

> About the naming: "fast" isn't nice (why is is faster? why not always
> use the faster version?). So I have been thinking about a re-factoring
> that includes these functions in one of the classes (Commute?) so we
> would automatically get the (faster) specialization when using removeRL
> on a (PatchInfoAnd p).

One thought: PatchInfoAnd has a specialised MyEq instance that only
compares on the PatchInfo. But I'm not sure how to get from that to a
generic removeRL implementation that would be fast on PatchInfoAnd. I
had an half-baked idea involving commuting and laziness, but I don't
think it works because commuting can fail.

Ganesh

Here are the additional patches I promised.

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

patch 42b648392e350a56b0e9577a3fc750698dc9f1f6
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Tue Jun 16 02:08:51 CEST 2015
  * added / re-worded comments in Darcs.Patch.Depends

patch 673b20856777b32a15266f31036816acc26fe87a
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Tue Jun 16 02:17:11 CEST 2015
  * re-add fastRemoveRL and use it in removeFromPatchSet

Attachments

• added-_-re_worded-comments-in-darcs_patch_depends.dpatch
• patch-preview.txt
• unnamed