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

[Aaron Bentley]

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

Martin von Gagern wrote:
> Aaron Bentley wrote:
>> 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.

There are many existing unit tests bzrlib/tests/test_reconfigure.py.  I
don't think it would be hard to crib from them, but there are usually
developers on #bzr if you have questions.  I'll be around until 21:00 UTC.

> Especially as I understand problems of the kind "after reconfiguring,
> reverting fails"

Revert is entirely incidental.  Most commands would fail at this point.
 The problem is that the reconfiguration failed.  If you're trying to
prove that reconfiguration fails safely, that's not actually
demonstrated by your tests.

> from the user perspective, but I don't feel confident
> enough to test for the actual cause of this kind of problem

Okay, how can we give you that confidence?

>, and I don't
> think that tests performing different operations on a working tree fit
> the scope of aunit test.

Tests on working trees are perfectly acceptable as bzr unit tests.

> So while I agree with you that from a performance point of view

It is not merely performance, it is also correctness.  You should test
the code you are fixing.

>, 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?

Regressing in terms of this bug.

> I thought
> the blackbox test would serve as a regression test for now.

Not necessarily.  They don't prove that Reconfigure.apply is being used
at all, and certainly it could be used in a particular way that fixed
reconfigure, but not other instances of this bug.  For example,
cmd_reconfigure could be mutating the reconfiguration.bzrdir._format.

>> 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 meant modifying the instance you already have, since that's useless
after the reconfiguration anyhow.

>>> -            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.

Even if you don't fetch, you should still create the repository in the
correct 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.

Yes, I think so.  It was example code.

>> 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 still prefer BzrDir.create_repository, because it is what we are
expected to use in general, but not enough to block the patch.

> 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?

Certainly.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkn3C1wACgkQ0F+nu1YWqI3tVACeL83Nz/M/u9r/oikJoaD1r8St
HM8AnA+r706fIaRYqjlPth5cIeELBORN
=HN8l
-----END PGP SIGNATURE-----