[merge][#288751] avoid creating text deltas spanning repository stacks

[John Arbash Meinel]

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

Martin Pool wrote:
> This patch fixes a bug which causes some trouble with
> stacked repositories.  The design was that text deltas would
> not span repository stacks, for the sake of robustness, and
> so that we can upgrade the inventory representation in
> underlying repositories without causing trouble to the
> stacked repository.  However, this was not checked properly.
> 
> The KnitPackRepository was previously trying to enforce this
> constraint, but it didn't cover all paths, and it only
> checked on file texts not on inventories.
> 
> This adds has_key(key) to some index(like) objects.  I
> realize we don't generally want people checking for one key
> at a time, but in some cases that's all the caller
> reasonably has, and it's better than making them repeat
> boilerplate to use get_parent_map.
> 
> There are some effort tests for stacking that I updated.
> I'm glad they're there.  I do a bit more or less work in
> some cases; I think it's still reasonable.
> 
> If this is merged well before 1.10 I think it would be worth putting
> into a 1.9.1 release.
> 
> This won't automatically repack existing repositories (unless I'm
> really lucky :-)
> 
> --
> Martin <http://launchpad.net/~mbp/>
> 

This patch conflicts slightly with bzr.dev, because of a small change in
the _abort_write_group logic.


I'm not particularly fond of 'has_key()' though I can agree that having
all callers have to repeat the "key in get_parent_map([key])" is not
particularly pleasant either.

However, is there a reason you have to be checking the keys one-by-one
rather than all in one go?

Specifically, when the fetch is almost done, why not check external
references then, and then you can do a get_parent_map() across all of
them at once, rather than one at a time? It also handles when you get
entries in "reverse" order. As Aaron points out, I think if you had the
revision graph:

   A
   |
   B
   |
   C

But you got the inventories in the order, C, B, A, this would cause C to
be inserted as a full text, then B, then A, and none of them have a
delta to build against.


Anyway, I can understand getting a fix in, as we need to close the
currently open bug now.

Also, to reply to Aaron "bzr pack" as it stands does *not* fill in the
missing holes. It would certainly be reasonable, even though the fix
would really prefer to be in "bzr reconcile", even though that is a bit
too heavyweight right now.

John
=:->



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkkh+UsACgkQJdeBCYSNAAM+gQCfWZ1n6so/7h4TKR9npZnbNKKZ
OmMAnRoutKu26oysjEiA1dUH2RIyDUKo
=dxSU
-----END PGP SIGNATURE-----