Merge lp:~maxb/bzr-svn/remove-overcomplexity-find_commit_paths into lp:bzr-svn/1.0

[Max Bowsher]

As I was looking at layout/guess.py, I noticed some code which was unnecessarily overcomplicated.

[Jelmer Vernooij (jelmer)]

This complexity is there for good reason. Imagine e.g. a commit where a user changes both /trunk/bla/foo and /wiki in the same commit.

Previously we would add a +1 for TrunkLayout(). With these changes, we would add one for RootLayout().

I'd be interested in merging the common_prefix tests though.

review: Disapprove (code)

[Max Bowsher (maxb)]

> This complexity is there for good reason. Imagine e.g. a commit where a user
> changes both /trunk/bla/foo and /wiki in the same commit.
>
> Previously we would add a +1 for TrunkLayout(). With these changes, we would
> add one for RootLayout().

Huh. I was under the impression that these changes were functionally neutral. I will look again.

[Max Bowsher (maxb)]

> > This complexity is there for good reason. Imagine e.g. a commit where a user
> > changes both /trunk/bla/foo and /wiki in the same commit.
> >
> > Previously we would add a +1 for TrunkLayout(). With these changes, we would
> > add one for RootLayout().
>
> Huh. I was under the impression that these changes were functionally neutral.
> I will look again.

I looked again. I understand why you would desire changing trunk and wiki to result in a TrunkLayout, however, this is *not* what the current 1.0 code actually does - it returns RootLayout. lp:~maxb/bzr-svn/test-guess-layout is my attempt to demonstrate this.

[Jelmer Vernooij (jelmer)]

On Fri, 2010-10-01 at 01:25 +0000, Max Bowsher wrote:
> > > This complexity is there for good reason. Imagine e.g. a commit where a user
> > > changes both /trunk/bla/foo and /wiki in the same commit.
> > >
> > > Previously we would add a +1 for TrunkLayout(). With these changes, we would
> > > add one for RootLayout().
> >
> > Huh. I was under the impression that these changes were functionally neutral.
> > I will look again.
>
> I looked again. I understand why you would desire changing trunk and
> wiki to result in a TrunkLayout, however, this is *not* what the
> current 1.0 code actually does - it returns RootLayout.
> lp:~maxb/bzr-svn/test-guess-layout is my attempt to demonstrate this.
Your test case is actually a corner case - we'd end up with the same
score for TrunkLayout and RootLayout so which one we should pick isn't
clear, but I see your point.

Patches welcome. :-)

Cheers,

Jelmer

Unmerged revisions

3467. By Max Bowsher on 2010-09-15

Remove layout.guess.find_commit_paths and associated overcomplexity.

3466. By Jelmer Vernooij on 2010-09-01

Properly encode paths when annotating.

3465. By Jelmer Vernooij on 2010-09-01

fix fetching of subvertpy repository.

3464. By Jelmer Vernooij on 2010-08-31

Fix compatibility with older versions of bzr.

3463. By Jelmer Vernooij on 2010-08-31

Fix root change.

3462. By Jelmer Vernooij on 2010-08-31

Cope with lockableconfig in newer versions of bzr.

3461. By Jelmer Vernooij on 2010-08-31

Fix svn parent tree retrieval.

3460. By Jelmer Vernooij on 2010-08-31

Simplify branch continuation handling.

3459. By Jelmer Vernooij on 2010-08-31

Some reformatting, don't let __len__ of RevisionMetadataBrowser depend on number of items iterated yet.

3458. By Jelmer Vernooij on 2010-08-29

Start on 1.0.5.

=== modified file 'layout/guess.py'
--- layout/guess.py	2010-08-13 01:34:44 +0000
+++ layout/guess.py	2010-09-15 00:37:50 +0000
@@ -29,15 +29,6 @@
 # Number of revisions to evaluate when guessing the repository layout
 GUESS_SAMPLE_SIZE = 300
 
-def find_commit_paths(changed_paths):
-    """Find the commit-paths used in a bunch of revisions.
-
-    :param changed_paths: List of changed_paths (dictionary with path -> action)
-    :return: List of potential commit paths.
-    """
-    for changes in changed_paths:
-        yield common_prefix(changes.keys())
-
 
 def guess_layout_from_branch_path(relpath):
     """Try to guess the branching layout from a branch path.
@@ -104,12 +95,11 @@
                       last_revnum)
             if revpaths == {}:
                 continue
-            for path in find_commit_paths([revpaths]):
-                layout = guess_layout_from_path(path)
-                if not potentials.has_key(str(layout)):
-                    potentials[str(layout)] = 0
-                potentials[str(layout)] += 1
-                layout_cache[str(layout)] = layout
+            layout = guess_layout_from_path(common_prefix(revpaths.keys()))
+            if not potentials.has_key(str(layout)):
+                potentials[str(layout)] = 0
+            potentials[str(layout)] += 1
+            layout_cache[str(layout)] = layout
     finally:
         pb.finished()
 

=== modified file 'mapping3/scheme.py'
--- mapping3/scheme.py	2010-07-26 21:59:02 +0000
+++ mapping3/scheme.py	2010-09-15 00:37:50 +0000
@@ -27,16 +27,14 @@
 from bzrlib.errors import BzrError
 from bzrlib.trace import mutter
 
-from bzrlib.plugins.svn.layout.guess import (
-    GUESS_SAMPLE_SIZE,
-    find_commit_paths,
-    )
+from bzrlib.plugins.svn.layout.guess import GUESS_SAMPLE_SIZE
 from bzrlib.plugins.svn.layout.standard import (
     CustomLayout,
     RootLayout,
     TrunkLayout,
     )
 from bzrlib.plugins.svn.errors import LayoutUnusable
+from bzrlib.plugins.svn.changes import common_prefix
 
 
 class InvalidSvnBranchPath(BzrError):
@@ -500,12 +498,11 @@
                       last_revnum)
             if revpaths == {}:
                 continue
-            for path in find_commit_paths([revpaths]):
-                scheme = guess_scheme_from_path(path)
-                if not potentials.has_key(str(scheme)):
-                    potentials[str(scheme)] = 0
-                potentials[str(scheme)] += 1
-                scheme_cache[str(scheme)] = scheme
+            scheme = guess_scheme_from_path(common_prefix(revpaths.keys()))
+            if not potentials.has_key(str(scheme)):
+                potentials[str(scheme)] = 0
+            potentials[str(scheme)] += 1
+            scheme_cache[str(scheme)] = scheme
     finally:
         pb.finished()
 

=== modified file 'tests/mapping3/test_scheme.py'
--- tests/mapping3/test_scheme.py	2009-04-01 01:24:58 +0000
+++ tests/mapping3/test_scheme.py	2010-09-15 00:37:50 +0000
@@ -28,7 +28,6 @@
     SingleBranchingSchemev0,
     SingleBranchingScheme,
     UnknownBranchingScheme,
-    find_commit_paths,
     guess_scheme_from_branch_path,
     guess_scheme_from_history,
     guess_scheme_from_path,
@@ -537,33 +536,6 @@
         self.assertEquals("single-ha/bla", str(SingleBranchingSchemev0("ha/bla")))
 
 
-class FindCommitPathsTester(TestCase):
-
-    def test_simple_trunk_only(self):
-        self.assertEquals(["trunk"], 
-            list(find_commit_paths([{"trunk": ('M', None, None)}])))
-
-    def test_branches(self):
-        self.assertEquals(["trunk", "branches/bar"], 
-            list(find_commit_paths([{"trunk": ('M', None, None)},
-                               {"branches/bar": ('A', None, None)}])))
-
-    def test_trunk_more_files(self):
-        self.assertEquals(["trunk"],
-                list(find_commit_paths([{
-                    "trunk/bfile": ('A', None, None),
-                    "trunk/afile": ('M', None, None),
-                    "trunk": ('A', None, None)
-                    }])))
-
-    def test_trunk_more_files_no_root(self):
-        self.assertEquals(["trunk"],
-                list(find_commit_paths([{
-                    "trunk/bfile": ('A', None, None),
-                    "trunk/afile": ('M', None, None)
-                    }])))
-
-
 class TestGuessBranchingSchemeFromBranchpath(TestCase):
 
     def test_guess_empty(self):

=== modified file 'tests/test_changes.py'
--- tests/test_changes.py	2010-08-09 23:48:59 +0000
+++ tests/test_changes.py	2010-09-15 00:37:50 +0000
@@ -22,6 +22,7 @@
 from bzrlib.plugins.svn.changes import (
     apply_reverse_changes,
     changes_root,
+    common_prefix,
     find_prev_location,
     path_is_child,
     under_prefixes,
@@ -148,3 +149,20 @@
 
     def test_none(self):
         self.assertTrue(under_prefixes("foo", None))
+
+
+class CommonPrefixTests(TestCase):
+
+    def test_simple_trunk_only(self):
+        self.assertEquals("trunk", common_prefix(("trunk",)))
+
+    def test_branches(self):
+        self.assertEquals("branches/bar", common_prefix(("branches/bar",)))
+
+    def test_trunk_more_files(self):
+        self.assertEquals("trunk", common_prefix(("trunk/bfile",
+            "trunk/afile", "trunk")))
+
+    def test_trunk_more_files_no_root(self):
+        self.assertEquals("trunk", common_prefix(("trunk/bfile",
+            "trunk/afile")))