[MERGE][bug 248932] reconfigure --standalone creates incompatibe repository

[Martin von Gagern]

Aaron Bentley wrote:
> bb:approve
> 
> I think this is an improvement with no drawbacks.

Thanks.

>> The attached patch contains a blackbox test and a solution to the
>> problem.
> 
> Why?
> 
> Blackbox tests aren't meant to be thorough, and they're slower than unit
> tests.  I would expect a blackbox test to demonstrate that "reconfigure
> --tree" works at all, and a unit test to ensure that Reconfigure.apply()
> creates a repository in the correct format.

Basically because I know how to use bzr, but I don't know the internal
API too well. So it's easy to express what I expect in terms of black
box behaviour, but I expect a unit test would require some more reading
of sources.

Especially as I understand problems of the kind "after reconfiguring,
reverting fails" from the user perspective, but I don't feel confident
enough to test for the actual cause of this kind of problem, and I don't
think that tests performing different operations on a working tree fit
the scope of aunit test.

So while I agree with you that from a performance point of view, unit
tests would be preferable, and I also believe that checking proper
compatibility in terms of repository, especially in the case of expected
format equality, should be feasible, I don't have the time to read
enough API to write such a test case.

> Your patch changes Reconfigure.apply, but doesn't add tests to prevent
> reconfigure.apply from regressing.

Regressing in terms of this bug, or in terms of another bug? I thought
the blackbox test would serve as a regression test for now.

> If I were coding it, I'd try to set self.bzrdir's repository format
> correctly, so that self.bzrdir.create_respoitory could be used
> unconditionally.  It seems we need a hybrid between BzrDir.__init__ and
> BzrDir.make_cloning_metadir.

I had thought along those lines at first as well, but I had some fears
about modifying an existing _format object, and wasn't sure about the
correct way to clone the format either. I'm not yet perfectly fluent in
Python, I notice. If you want to, revisit my first draft, from this mail
thread, and see whether you would prefer that as a base for addressing
this issue.

>> -            repo = self.bzrdir.create_repository()
>> +            if self.local_branch and not self._destroy_branch:
>> +                old_repo = self.local_branch.repository
> 
> You are not using self.local_repository here, so in cases where there is
> no branch, this will not do the right thing.
> 
> Really, though, I think self.repository is The Right Thing.  If there is
> a local branch, it will be correct.  If there's no local branch, it will
> still be correct.
> 
>> +            elif self._create_branch and self.referenced_branch is not None:
>> +                old_repo = self.referenced_branch.repository
> 
> Similarly here, I don't understand why the condition's on
> _create_branch, not _create_repository.  In fact, really, all of this
> can be conditional on _create_repository.

Those two conditions mirror the two cases causing repo.fetch to get
invoked later on. The idea is that whenever I do a fetch, and if
_create_repository is set, then I create a repository, taking the format
of the repository I plan to fetch from as the template. After spending
some time trying to figure out when what attributes will be set, None,
or completely unavailable, I decided to stop being clever and came up
with this code instead.

> If we're deleting a branch reference, then the repository we create
> should match the deleted reference's format.  Otherwise, we should match
> the existing repository format.
> 
> So I would probably do something like:
> 
> if self._destroy_reference:
>     (self.bzrdir._repository_format =
>      self.referenced_branch.repository._format)
> elif self._repository is not None:
>     self.bzrdir._repository_format = self.repository._format

Shouldn't that be self.bzrdir._format.repository_format? I thinks so.

> And then the self.bzrdir.create_repository could be used as-is.

Hmmm... well, if I understood lifeless correctly, he preferred
RepositoryFormat.initialize over BzrDir.create_repository. I have no
strong feelings either way, but know that this approach helped me avoi
the headache of deciding whether or not and if so how to clone
BzrDir._format before changing its repository_format attribute.

> Aaron

What next? You voted approve, but listed quite a number of wishes for
modification. I would hope for a second approve to get this merged, and
then revisit it to improve upon when I find the time, unless some
developer beats me to it. Is that OK?

Greetings,
 Martin


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 261 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090427/bc60e4b7/attachment.pgp