[RFC][PATCH 2/4] Speed improvement in fetch/clone: file_involved( ) test

[Robert Collins]

On Sat, 2005-12-10 at 19:15 +0100, Goffredo Baroncelli wrote:
> This patch add the test case for the function file_involved( ). Moreover
> changes a bit the fetch test in order to adapt this test to a different behavior 
> of the function greedy_fetcher: before this function returned the revision
> fetched ( they didn't fetch a revision already fetched ); now the function
> greedy_fetcher returns the number of the revision in which the two histories differ.
> 
> === added file 'bzrlib/tests/test_file_involved.py'
> --- /dev/null
> +++ bzrlib/tests/test_file_involved.py
> @@ -0,0 +1,179 @@
> +
> +
> +
> +from bzrlib.tests import TestCaseInTempDir
> +import os
> +from bzrlib.commit import commit
> +from bzrlib.add import smart_add
> +from bzrlib.branch import Branch
> +from bzrlib.clone import copy_branch
> +from bzrlib.merge import merge
> +from bzrlib.workingtree import WorkingTree
> +from bzrlib.delta import compare_trees
> +
> +class TestFileInvolved(TestCaseInTempDir):
> +
> +    def touch(self,filename):
> +        f = file(filename,"a")
> +        f.write("appended line\n")
> +        f.close( )
> +
> +

coding style here: 
1 blank line between methods in classes.
a space between parameters: def touch(self, filename):
no space with empty calls: f.close()

> +    def merge( self, branch_from, force=False ):
> +        from bzrlib.merge_core import ApplyMerge3
> +

no whitespace between brackets: def merge(self, branch_from,
force=False):

> +        merge([branch_from,-1],[None,None], merge_type=ApplyMerge3,
> +            check_clean=(not force) )

spaces after commas:
        merge([branch_from, -1], [None, None], merge_type=ApplyMerge3,
            check_clean=(not force))

> +
> +    def setUp(self):
> +        super(TestFileInvolved, self).setUp()
> +        # create three branches, and merge it
> +        #
> +        #           /-->J --->K                (branch2)
> +        #          /          \
> +        #  A ---> B --->C ---->D               (main)
> +        #  \           /      /
> +        #   \---> E---/----> F                 (branch1)
> +
> +        os.mkdir("main")
> +        os.chdir("main")
> +
> +        main_branch = Branch.initialize('.')
> +        self.build_tree(["a","b","c"])
> +
> +        smart_add('.')
> +        commit(Branch.open("."), "Commit one", rev_id="rev-A")
> +        #-------- end A -----------
> +
> +        copy_branch(main_branch,"../branch1")
> +        os.chdir("../branch1")
> +
> +        #branch1_branch = Branch.open(".")
> +        self.build_tree(["d"])
> +        smart_add(".")
> +        commit(Branch.open("."), "branch1, Commit one", rev_id="rev-E")
> +
> +        #-------- end E -----------
> +
> +        os.chdir("../main")
> +        self.touch("a")
> +        commit(Branch.open("."), "Commit two", rev_id="rev-B")
> +
> +        #-------- end B -----------
> +
> +        copy_branch(Branch.open("."),"../branch2")
> +        os.chdir("../branch2")
> +
> +        branch2_branch = Branch.open(".")
> +        os.chmod("b",0770)
> +        commit(Branch.open("."), "branch2, Commit one", rev_id="rev-J")
> +
> +        #-------- end J -----------
> +
> +        os.chdir("../main")
> +
> +        self.merge("../branch1")
> +        commit(Branch.open("."), "merge branch1, rev-11", rev_id="rev-C")
> +
> +        #-------- end C -----------
> +
> +        os.chdir("../branch1")
> +        tree = WorkingTree('.', Branch.open("."))
> +        tree.rename_one("d","e")
> +        commit(Branch.open("."), "branch1, commit two", rev_id="rev-F")
> +
> +
> +        #-------- end F -----------
> +
> +        os.chdir("../branch2")
> +
> +        self.touch("c")
> +        smart_add('.')
> +        commit(Branch.open("."), "branch2, commit two", rev_id="rev-K")
> +
> +        #-------- end K -----------
> +
> +        os.chdir("../main")
> +
> +        self.touch("b")
> +        self.merge("../branch1",force=True)
> +
> +        commit(Branch.open("."), "merge branch1, rev-12", rev_id="rev-D")
> +
> +        # end D
> +
> +        self.merge("../branch2")
> +        commit(Branch.open("."), "merge branch1, rev-22",  rev_id="rev-G")
> +
> +        # end G
> +        os.chdir("../main")
> +        self.branch = Branch.open(".")


All the chdirs above should not be needed, that would make the code
shorter and easier to comprehend I think.
i.e. self.touch("main/b")

generally speaking, none of our code depends, or should depend, on cwd -
except for the topmost ui layers.

The rest of the code looks good, modulo the same layout issues I've
raised above.


> === modified file 'bzrlib/tests/test_fetch.py'
> --- bzrlib/tests/test_fetch.py
> +++ bzrlib/tests/test_fetch.py
> @@ -86,7 +86,7 @@
>          self.assertFalse(has_revision(br_a3, br_a.revision_history()[revno]))
>      self.assertEqual(greedy_fetch(br_a3, br_a2, br_a.revision_history()[2])[0], 3)
>      fetched = greedy_fetch(br_a3, br_a2, br_a.revision_history()[3])[0]
> -    self.assertEquals(fetched, 3, "fetched %d instead of 3" % fetched)
> +    self.assertEquals(fetched, 6, "fetched %d instead of 6" % fetched)
>      # InstallFailed should be raised if the branch is missing the revision
>      # that was requested.
>      self.assertRaises(bzrlib.errors.InstallFailed, greedy_fetch, br_a3,

I don't understand this change here - surely as fetch.py is not altered
[yet], this test cannot pass with this change.

Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20051215/f0be44c8/attachment.pgp