[MERGE] More deprecations...

[Andrew Bennetts]

Robert Collins wrote:
> On Thu, 2008-03-27 at 16:45 +1100, Andrew Bennetts wrote:
> > 
> > Or perhaps it should be "bb:justify"...
> 
> Fair enough.
> 
> > I'm uncomfortable with all the duplicated code you've added to replace the
> > calls to deprecated methods.  It feels like a step backward to replace two
> > simple lines with ten.  I realise the point to some extent is to make it
> > harder to accidentally do work proportional to size(history), but we still
> > have several places in the code that do, and making them uglier without
> > otherwise improving them seems like a backwards step to me.  One of the
> > comments hints that the ugliness is temporary, at least in one case... if
> > that's so, then I think I can live with this, but comments at each site
> > saying so would be good.
> > 
> > Basically, I'm sure you have a good reason for wanting to duplicate this
> > code all over the place, but you're going to have to explain it to me :)
> > 
> > Further comments inline, mostly repeats of the above concern.
> 
> Firstly, _make_breadth_first_searcher is not private; its just not
> public. We took the stance some time ago that within bzrlib _ is
> advisory for *external users*, not for internal.

I realise that (and double-checked in HACKING.txt to make sure before I
sent the review).  What I meant is what Aaron pointed out: if several
diverse parts of bzrlib itself need this API, then chances are plugin
authors and other users of bzrlib will need it too.

> regarding the duplication, it boils down to a few things...
> 
> My overall goal here is to get to a VersionedFiles api which is tunable
> to perform well, and doesn't have cruft on it. O(history) apis other
> than the [effectively required] 'all keys' api - currently called
> 'versions()'.
> 
> To do this regardless of conversion approach, we have to stop using the
> old methods that are on 'VersionedFile'. We can do this a number of
> ways: we can leave a thunk permanently in place, we can use external
> functions, we can do basic macro expansion at each current site, or we
> can fix the code up properly.
> 
> I would love to fix the code up properly; the cost of using per-file-id
> apis is sufficiently high I don't want to leave the thunk that I'm going
> to use to convert the code in place, beyond a couple of very specific
> and specialised uses. Using external functions isn't any better I think.

I don't quite follow why having thunks with DeprecationWarnings isn't
ok.  You mean the replacement code (that happens to be duplicated) is
faster?

> So I chose to duplicate the code in this particular case, and I do think
> its temporary because once we have VersionedFiles we should be able to
> collapse some of the duplicate join logic down - I just don't want to
> divert off to fixing 'does ghost filling on versioned file joins' right
> now.

If it is temporary, then there should be comments at each place where
the code is duplicated indicating that this code is temporary, so people
know that it is expected to smell, and that e.g. they shouldn't
copy-and-paste this code blindly.  You could centralise this "hey this
is temporary" remark in a function that emits a warning somewhere,
avoiding duplication... ;)

-Andrew.