[MERGE] Reduce round trips pushing new branches substantially.
Andrew Bennetts
andrew.bennetts at canonical.com
Mon Apr 27 05:05:18 BST 2009
Robert Collins wrote:
[...]
> So, broadly 'there is something that might break and we don't test'.
>
> :). I'd like to leave it as is because:
> - it passes automatic and manual tests
> - your analysis might be wrong
> - we can't tell either way without getting deep into that code, and if
> so I think its new development. A good time to do that would be when
> looking at the 'how do I force stacking off' bug I think.
Well, it looks to me more like “there is a new way for this to fail, and
we're adding code to hide the error even though the intent is that it should
fail in this case.”
It seems pretty clear to me that when _require_stacking is true (and when
there's something to stack on) that this method's contract is “I add the
fallback, and if I can't raise an exception”. It doesn't look very deep to
me. I don't think it should be possible to have require_stacking=True and
then silently not get stacking... the sprout code is muddy enough without
that!
Given that nothing is likely to hit this bug soon I guess it'd be ok to have
an XXX or a bug report or something about it, but really it seems easier to
just add the two lines and fix it.
[...]
> > Ugh, that's right. How about a module-global function in bzrdir.py? e.g.
> >
> > def get_default_stacking_format(rich_root):
> > if rich_root:
> > repo_format = pack_repo.RepositoryFormatKnitPack6RichRoot()
> > else:
> > repo_format = pack_repo.RepositoryFormatKnitPack6()
> > branch_format = branch.BzrBranchFormat7()
> > return branch_format, repo_format
> >
> > And then update BzrDirMetaFormat1.require_stacking to use it too — it has
> > the same basic logic as yours, but uses RepositoryFormatKnitPack5* not 6.
> > It seems wrong to let these places get out of sync.
>
> Hmm, yes. How about a follow up branch? I'm betting there will be test
> fallout.
Yes, probably :(
Fair enough.
-Andrew.
More information about the bazaar
mailing list