Code review comment for lp:~spiv/bzr/inventory-delta

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

Andrew Bennetts wrote:
> Robert Collins wrote:
>> On Fri, 2009-08-07 at 05:39 +0000, Andrew Bennetts wrote:
> [...]
>> [debug flags]
>>> I hope we don't need to ask people to use them, but they are a cheap insurance
>>> policy.
>>>
>>> More usefully, they are helpful for benchmarking and testing. I'm ok with
>>> hiding them if you like.
>> I don't have a strong opinion. The flags are in our docs though as it
>> stands, so they should be clear to people reading them rather than
>> perhaps causing confusion.
>
> I'll leave them in, assuming I can find a way to make the ReST markup cope with
> them...
>

Well, you could always change them to IDS_always to make it consistent
with our other debug flags.

>>> I don't understand why that case isn't covered. Perhaps you should file the bug
>>> instead of me?
>> I'd like to make sure I explain it well enough first; once you
>> understand either of us can file it - and I'll be happy enough to be
>> that person.
> [...]
>> What you need to do is change the test from len(self.packs) == 1, to
>> len(packs being combined) == 1 and that_pack has the same hash.
>
> But AIUI “self.packs” on a Packer *is* “packs being combined”. If it's not then
> your explanation makes sense, but my reading of the code says otherwise.

So he might have been thinking of PackCollection.packs, but you are
right. Packer.packs is certainly the "packs being combined".

...

> Oh, actually, it does matter. When hint == [], no packs should be repacked.
> When hint is None, all packs should be repacked.

Agreed.

...

> A tangent: when we transfer parent inventories to a stacked repo, should we
> transfer the texts introduced by those inventories or not?

No. The point of transfering those parent inventories is so that we can
clearly determine what texts are in the children that are not in the
parents.

> I'm seeing a
> worrisome half-and-half behaviour from the streaming code, where the synthesised
> rich root, but no other texts, are transferred for inventories requested via
> get_stream_for_missing_keys. I think the intention is not (if those parent
> revisions are ever filled in later, then at that time the corresponding texts
> would be backfilled too). I've now fixed this too.

Right. I'm personally not very fond of what we've had to do with
stacking. We had an abstraction in place that made stacked branches
auto-fallback to their parents, then we break that abstraction, but only
when accessing via Remote, which then means we need to fill in some
data, which starts breaking the other assumptions like having an
inventory means having all the texts, etc.

Anyway, I think you have it correct, that the *new* assertion is
something along the lines of:

    If we have a revision, then we have its inventory, and all the new
    texts introduced by that revision, relative to all of its parents.

>
> The net result of this is that the stacking tests in
> per_interrepository/test_fetch.py are starting to get pretty large. I suspect
> we can do better here... (but after this branch lands, I hope!)
>

Well, ideally most of that would be in "per_repository_reference" which
is the formats that can be stacked.

I realize there is a bit of a push, because then you also have
per_interrepo_reference :).

...

> (It's “review needs-fixing”, btw. Not sure when that got added.)
>

or needs_fixing or needsfixing
or possibly +0

> I've attached a psuedo-incremental diff (I merged latest bzr.dev into the tip of
> this branch, and separately bzr.dev into the last reviewed revision, and then
> produced the diff of those two). So if anything looks funny in the diff you
> might want to check the actual branch.
>
> -Andrew.

     def _iter_inventories(self, revision_ids, ordering):
         """Iterate over many inventory objects."""
+ if ordering is None:
+ ordering = 'unordered'

^- I thought we determined that if it wasn't passed it needed to be
"as_requested" ?

I would probably change it so that you have a:
stream_ordering = XXX
versus
logical_ordering = YYY

Given the ugliness underneath introduced by the flag, I almost wonder if
it wouldn't be clearer to have 2 separate helper functions:

def _iter_inventories_as_requested()

vs

def _iter_inventories_unordered()

I realize there would be *some* duplication, but it would probably be
clearer to figure out what was going on.

Anyway, think about it, I wouldn't block the patch on it.

The rest seems fine to me.

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

iEYEARECAAYFAkqEToQACgkQJdeBCYSNAAM05gCgmqmahotTEg4OVxTkPsEgA/IT
GSYAoKqOxYVsR+dxwSDBoSKTGnAxpnmJ
=kUs6
-----END PGP SIGNATURE-----