[MERGE] Allow insert_record_stream to partially insert a stream rather than error when there are missing compression parents

[John Arbash Meinel]

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

Andrew Bennetts wrote:
> This patch relaxes VersionedFiles.insert_record_stream a little.  For
> repositories where it is possible to do so safely, it will allow records
> missing a compression parent to be written.  For knits that would be writing
> a record to the .knit but not .kndx.  When suspendable write groups lands
> packs will support it too (by disallowing the insertion of a pack when there
> are still missing references, although such a pack may be suspended).

I see that you moved the code that was raising an error, but I don't see
you actually call it. Which means that if this patch lands, we *might*
push records that contain a missing delta parent to a stacked repository.

(You introduce "get_missing_compression_parents" but you never actually
call the function.)

On that basis this is:
  BB:resubmit

I think this is the right path to go, it just isn't there yet.

> 
> Callers of insert_record_stream can discover which compression parents are
> missing via the new get_missing_compression_parents API on VersionedFiles.
> 
> The point of this is to allow smart push to push records safely and
> efficiently to stacked repositories in two round-trips:
> 
>   1. send delta records for all the revisions we're pushing, then
>   2. send fulltexts for records missing compression parents — if any.
> 
> (Other strategies would be possible if we want to experiment with different
> tradeoffs for number of round-trips vs. number of excess fulltexts sent.
> This is essentially what the VFS code does today, although it looks before
> it leaps.)

I would assume you would pull out a lot of the current "unpack as you
go" that we have now?

I'll also note that if you have a smart server on the other end, it
could do the unpacking *for* you, for a stacked branch. The code in
insert_record_stream() does that right now (if it sees a delta whose
basis is only in the stacked-on location, it expands it to a fulltext).
I suppose we can change this to having the client expand things in a
second pass, but why not just have the server do it?

> 
> For repositories that can't support this (weaves, for example), their
> versioned files' insert_record_stream may still raise RevisionNotPresent.
> 
> Oh, and this patch also fixes the test for insert_record_stream with missing
> parents to test what it claims to, as well as updating it for the new
> behaviour!
> 
> -Andrew.

Actually, you changed the functionality, so that it no longer raises an
exception if the stream is incomplete (and cannot be made complete by
the stacked-on location). Based on the "supports_partial_insertion" flag.

Certainly we could support a single "insert_record_stream()" allowing
the insertion of incomplete records, but we need to be raising if
"get_missing_compression_parent_keys()" returns non-empty at some point
in the future.

So perhaps this patch can just be merged (almost as is) when you
actually introduce the 2-pass logic, for now, though, it would silently
create a corrupted repository if a delta parent really wasn't present.

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

iEYEARECAAYFAkmbHjgACgkQJdeBCYSNAAPRqgCeOpU/9CSR0JGxDwU1qk1cd43N
FfsAoIIKZrQeO+bYu4W4Zu5SZpyM6cb4
=r2dN
-----END PGP SIGNATURE-----