stacking and fetching bugs, dev6 issues

[Robert Collins]

On Fri, 2009-05-08 at 19:32 -0500, John Arbash Meinel wrote:
> 
> Well, I missed the part where it said
> "self.to_repository._fallback_repositories", but it still remains that
> it does updates the missing_keys directly, rather than having the
> 'StreamSink.insert_stream' return them.
> 
> So I guess with stacked repos, it will just do the wrong thing on the
> second push, and parents aren't filled for non-stacked.

The call to insert them asks for delta closures to be satisfied, so
every text pushed up can be reconstructed; we're not adding revision
objects so another round trip isn't needed. I still don't see the 'wrong
thing' going on here.

> > 
> >>   b) It depends on the first 'insert_stream' to not actually commit
> >> the
> >>      incomplete repository (because it expects there will be
> missing
> >>      keys due to compression parents). However, for dev6
> repositories
> >>      there will be no incomplete texts. Which means that instead we
> >>      commit the required texts for the given revisions, and then
> 'come
> >>      back later', and fill in the fulltexts for other inventories.
> > 
> > No, it doesn't depend on that; adjacent inventories are not
> incomplete
> > in 1.9 repos either; all it depends on is suspending the write
> group.
> > 
> 
> But the write group is *only* suspended if there are missing
> compression
> parents. Otherwise at the end of 'insert_stream' it will have
> committed
> the new pack.
> 
> And since the _parent_inventories check is done outside of
> 'insert_stream' ....

inside insert_stream:
        missing_keys =
self.target_repo.get_missing_parent_inventories()     

We do the check for missing parent inventories twice:
once in the server, to tell all clients to do it.
once in the client, to send them up if we are using sftp or something.

So no the server will not have committed the new pack.

> >> (a) is something that I think we just need a generic fix for. I
> think
> >> it
> >> could be something as simple as:
> > ...
> >> I'd like Robert or Andrew to confirm, though.
> > 
> > Please write a test/code we can play with to demonstrate it. In
> theory
> > you are wrong, but in practice you may be right. We should confirm
> it
> > before adding code that isn't needed.
> > 
> >> for (b) this is a bit trickier, but it also seems like a generic
> >> problem
> >> for current formats. It just 'happens' that we rarely will commit
> the
> >> data before filling in the extra inventories. But certainly one
> could
> >> construct a situation where the first insert doesn't have any
> missing
> >> compression parents, and then a user does ^C at the right time.
> >> (Consider the case of a merge commit that doesn't introduce
> anything
> >> new, and ends on a delta boundary for the inventory. The new
> inventory
> >> is a fulltext, but it then has to upload the fulltext for both
> merge
> >> parents, which have long delta chains, giving plenty of time for
> ^C.)
> > 
> > Again, we don't commit, so the data isn't visible.
> > 
> 
> Yes we do. Now if the "missing_keys.update(_parent_inventories())" was
> inside 'StreamSink.insert_stream' and it chose that moment to
> 'suspend_write_group', then I would agree with you.

Good, we agree then, because it is :).

-Rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090509/0be6c8e5/attachment.pgp