[darcs-devel] darcs patch: Unsafe pull_firsts_middles optimisation (and 7 more)

[David Roundy]

On Fri, May 06, 2005 at 11:15:25PM +0100, Ian Lynagh wrote:
> The graphs at
> 
>     http://urchin.earth.li/~ian/record/record.html
> 
> show (essentially identical) heap profiles for
> 
>     darcs record -a -m foo --no-test -v
> 
> and
> 
>     echo a | darcs record -m foo --no-test -v

Cool!

> with these patches plus:
> 
> hunk ./Repository.lhs 327
> -            if AnyOrder `elem` opts
> +            if True || AnyOrder `elem` opts
> 
> which stops reorder_and_coalesce being used on the pending changes and
> diff. Are there any objections to removing this sort (in which case
> you'd get things in the order they are in pending, which I assume is
> either the order you made them or its reverse, followed by the diff
> results which are already sorted)?

The problem is that then the "adds" will not be interleaved with the
initial hunks.  I suppose you could consider this to be a feature, since it
would mean all the added files are listed at the top of a patch.

The other danger (which may be addressed elsewhere in the code, but it used
to be important to address it here) is that you could get prompted for an
add and remove of the same file.  For example, if you ran

darcs add foo
rm foo
darcs record --all

I think your code would prompt you first for the add and then for the
remove (which was discovered during the diff).  Perhaps this is a rare
enough corner case not to be an issue.  Or maybe I'm confusing something
and it wouldn't actually behave this way.  But I definitely don't like this
behavior of prompting for changes that are equivalent to identity.

The only way I can imagine to efficiently handle this situation would be
through some sort of a pre-processing step, where we could look through
pending and remove any adds that don't correspond to an actual file in the
working directory.  Actually, this could perhaps be done in read_pending
itself? It'd require a bit of care with respect to file and directory
renames, but not much.

> I've also changed 'a' so that, rather than selecting everything you
> haven't said 'y' or 'n' to past the point in the patch list you are, it
> instead selects such patches from anywhere in the list. This makes it a
> lot more efficient as we can then just set all the Nothings to Just b
> rather than having to worry about dependencies. Given you can move
> backwards and forwards in the list I also think it makes more sense.

Yeah, this sounds good.
-- 
David Roundy
http://www.darcs.net