[MERGE] dpush / foreign vcs testing

[John Arbash Meinel]

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

Jelmer Vernooij wrote:
...

>>>>> I'm still looking for review of this patch; it would be nice if it
>>>>> could land in 1.14 so that bzr-git doesn't have to depend on bzr-svn
>>>>> for dpush... :-)
>>>> Updated patch with against bzr.dev.
>>> Here's the final version, with a slight performance improvement and a
>>> fix to handle nested files.
>> Is there any chance somebody could review this patch? It would be nice
>> if it could make it into 1.14. It's been waiting in the queue for two
>> months now.
> here's an updated patch, with latest bzr.dev merged (and conflicts
> resolved).
> 
> Cheers,
> 
> Jelmer
> 

I think there is a reasonable use case for dpush, mostly from tracking a
foreign branch and not wanting to "interrupt" the upstream. I don't
particularly like doing it this way, but having read the discussions in
the past, I can understand why people feel it would be necessary.

#1) The command should be hidden (IMO), you can do that with "hidden =
True" in the class definition. At least for now, I would prefer not
having someone stumble across the command and try to use it.

I *do* see that you at least add a command that it will fail for most
bzr format implementations.

#2) I find it a little bit odd in design that "dpush" is implemented in
terms of "dpull" being present on the target branch. But it isn't a huge
deal.

#3)


+        try:
+            source_wt = WorkingTree.open_containing(directory)[0]
+            source_branch = source_wt.branch
+        except NoWorkingTree:
+            source_branch = Branch.open_containing(directory)[0]
+            source_wt = None

^- We have BzrDir.open_containing_tree_branch_or_repository() for that
sort of thing. Though personally I prefer:

+        try:
+            source_wt = WorkingTree.open_containing(directory)[0]
+            source_branch = source_wt.branch
+        except NoWorkingTree:
+            source_branch = Branch.open(directory)
+            source_wt = None

(When you are in a WT it makes sense to open the containing object, but
without the WT, it makes more sense to require the exact path.)


...
+    def dpull(self, source, stop_revision=None):
+        """Pull deltas from another branch.
+
+        :note: This does not, like pull, retain the revision ids from
+            the source branch and will, rather than adding bzr-specific
+            metadata, push only those semantics of the revision that
can be
+            natively represented by this branch' VCS.
+
+        :param source: Source branch
+        :param stop_revision: Revision to pull, defaults to last revision.
+        """
+        raise NotImplementedError(self.dpull)

^- You define the :param: to supply, but you don't define the return
value. I can see that it is some sort of revid_map by inspecting the
code. A bit of clarification would be nice. (Does it map everything,
just map the ones that were modified, etc.)

...

+            if not no_rebase:
+                _, old_last_revid = source_branch.last_revision_info()
+                new_last_revid = revid_map[old_last_revid]
+                source_branch.pull(target_branch, overwrite=True,
+                                   stop_revision=new_last_revid)

^- It seems a bit odd that you would use the revid_map to map the old
last revision to the new one, rather than doing something like:
source_branch.pull(target_branch, overwrite=True)

Since you just *pushed* the last revision for the target branch...

...

+def determine_fileid_renames(old_inv, new_inv):

^- This seems like it should be an implementation detail, and thus a
private function.

I would probably say that you should check if the "file_id" exists in
the target before you translate back into paths, simply because checking
the 10,000 files that *didn't* change by mapping through paths seems a
bit expensive. Also, you just computed the path in the local inventory
because it is exactly the path in the old inventory. So calling
'id2path' yet again on the new_inv seems wasteful.

That can come later, just a some recommendations.


...

+def update_workinginv_fileids(wt, old_inv, new_inv):

^- this looks dangerously destructive. You call 'unversion()' on a
containing directory, but there is no guarantee that all files
underneath that directory will have new file ids.

I didn't audit the code paths particularly closely, it may be that you
have defined it to unversion everything from the target, and re-add
everything back. Rather inefficient, but I guess it "works".
...

=== added file 'bzrlib/tests/blackbox/test_dpush.py'

+from bzrlib.tests.test_foreign import DummyForeignVcsDirFormat
+
+import os
+
+class TestDpush(ExternalBase):

minor style issue. stdlib imports come before bzrlib imports, and we
want 2 blank spaces between imports an the class definitions.


+    def setUp(self):
+        BzrDirFormat.register_control_format(DummyForeignVcsDirFormat)
+        super(TestDpush, self).setUp()
+
+    def tearDown(self):
+        try:
+
BzrDirFormat.unregister_control_format(DummyForeignVcsDirFormat)
+        except ValueError:
+            pass
+        super(TestDpush, self).tearDown()

^- I'm pretty sure it is easier to addCleanup() than do tearDown, but I
think either works.


+    def test_dpush_native(self):
+        tree = self.make_branch_and_tree("dp")
+        self.run_bzr("init dc")
+        error = self.run_bzr("dpush -d dc dp", retcode=3)[1]
+        self.assertContainsRe(error, 'not a foreign branch, use regular
push')

^- We generally prefer if you use "make_branch_and_tree" for everything
except the final "dpush" command. That actually goes for a lot of your
"dpush" tests, where we would prefer you to use things like
"branch.bzrdir.sprout()" rather than "run_bzr('branch a b')".

I realize you may have copied the cruft from a "push" test suite, it
would be nice not to propagate it, though.

BB:tweak

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

iEYEARECAAYFAknWGvgACgkQJdeBCYSNAAO8VACgqkCmNbtWj5siG6Zp9u8Dk4hU
uqMAnRzSVK/5sYUdExmgBRCNhJ6a5BSE
=JiUp
-----END PGP SIGNATURE-----