Merge lp:~abentley/bzr/last-revision into lp:~bzr/bzr/trunk-old

Proposed by Aaron Bentley on 2009-05-22
Status: Rejected
Rejected by: Aaron Bentley on 2009-05-28
Proposed branch: lp:~abentley/bzr/last-revision
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 124 lines
To merge this branch: bzr merge lp:~abentley/bzr/last-revision
Reviewer Review Type Date Requested Status
Martin Pool Needs Information on 2009-05-28
John A Meinel Needs Information on 2009-05-28
Ian Clatworthy 2009-05-22 Approve on 2009-05-26
Review via email: mp+6742@code.launchpad.net
To post a comment you must log in.
Aaron Bentley (abentley) wrote :

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

Hi all,

This patch cleans up our support for last_revision. This API was
supposed to be deprecated, but has turned out to be too useful.
Therefore, I've added tests and a default implementation on Tree.

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

iEYEARECAAYFAkoWlIEACgkQ0F+nu1YWqI0a/QCfSCzyrts6HY0902CCsW2Mr2F7
rN4An05UjaT5msCupEINNLe3CxPaypof
=GsVB
-----END PGP SIGNATURE-----

review: Approve
John A Meinel (jameinel) wrote :

I use Branch.last_revision() all the time, but I don't feel that maps well into "Tree.last_revision()".

I would really prefer it to use a different name, since it doesn't return the 'same' results. For Tree/WorkingTree it seems to be returning the revision of the left-hand parent, while for Branch it is giving the *current* revision.

Can you define what/where you are using it? (I overlooked this patch originally because I was thinking about Branch.last_revision().)

The code change is fine, I'm just trying to understand the utility of "give me one of, but not all of the parents". Especially considering what answer do we want to give if the left-hand parent is a ghost, etc.

review: Needs Information
Martin Pool (mbp) wrote :

I agree with John, this seems kind of questionable.

review: Needs Information
lp:~abentley/bzr/last-revision updated on 2009-05-28
4378. By Aaron Bentley on 2009-05-28

Merge bzr.dev ito last-revision.

Unmerged revisions

4378. By Aaron Bentley on 2009-05-28

Merge bzr.dev ito last-revision.

4377. By Aaron Bentley on 2009-05-28

Merge bzr.dev into last-revision.

4376. By Aaron Bentley on 2009-05-22

Clean up last_revision support.

4375. By Aaron Bentley on 2009-05-22

Implement and test Tree.last_revision.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/mutabletree.py'
2--- bzrlib/mutabletree.py 2009-04-09 20:23:07 +0000
3+++ bzrlib/mutabletree.py 2009-05-28 12:36:11 +0000
4@@ -246,22 +246,6 @@
5 """
6 return (self.get_file(file_id, path), None)
7
8- @needs_read_lock
9- def last_revision(self):
10- """Return the revision id of the last commit performed in this tree.
11-
12- In early tree formats the result of last_revision is the same as the
13- branch last_revision, but that is no longer the case for modern tree
14- formats.
15-
16- last_revision returns the left most parent id, or None if there are no
17- parents.
18-
19- last_revision was deprecated as of 0.11. Please use get_parent_ids
20- instead.
21- """
22- raise NotImplementedError(self.last_revision)
23-
24 def lock_tree_write(self):
25 """Lock the working tree for write, and the branch for read.
26
27
28=== modified file 'bzrlib/tests/tree_implementations/test_tree.py'
29--- bzrlib/tests/tree_implementations/test_tree.py 2009-03-23 14:59:43 +0000
30+++ bzrlib/tests/tree_implementations/test_tree.py 2009-05-28 12:36:11 +0000
31@@ -18,6 +18,7 @@
32 errors,
33 conflicts,
34 osutils,
35+ revision,
36 revisiontree,
37 tests,
38 workingtree_4,
39@@ -292,3 +293,30 @@
40 self.addCleanup(tree.unlock)
41 expected = osutils.sha_strings('file content')
42 self.assertEqual(expected, tree.get_file_sha1('file-id'))
43+
44+
45+class TestParentIDs(TestCaseWithTree):
46+
47+ def test_get_parent_ids_NULL(self):
48+ work_tree = self.make_branch_and_tree('tree')
49+ tree = self._convert_tree(work_tree)
50+ self.assertEqual([], tree.get_parent_ids())
51+
52+ def test_last_revsion_NULL(self):
53+ work_tree = self.make_branch_and_tree('tree')
54+ tree = self._convert_tree(work_tree)
55+ self.assertEqual(revision.NULL_REVISION, tree.last_revision())
56+
57+ def make_tree_with_commit(self):
58+ work_tree = self.make_branch_and_tree('tree')
59+ rev_id = work_tree.commit('message')
60+ tree = self._convert_tree(work_tree)
61+ return tree, rev_id
62+
63+ def test_get_parent_ids_one(self):
64+ tree, rev_id = self.make_tree_with_commit()
65+ self.assertEqual([rev_id], tree.get_parent_ids())
66+
67+ def test_last_revision_one(self):
68+ tree, rev_id = self.make_tree_with_commit()
69+ self.assertEqual(rev_id, tree.last_revision())
70
71=== modified file 'bzrlib/tree.py'
72--- bzrlib/tree.py 2009-05-23 04:55:52 +0000
73+++ bzrlib/tree.py 2009-05-28 12:36:11 +0000
74@@ -124,6 +124,22 @@
75 """
76 raise NotImplementedError(self.get_parent_ids)
77
78+ def last_revision(self):
79+ """Return the lefthand parent of the tree.
80+
81+ For working trees, this will be the last revision committed to the
82+ tree.
83+
84+ For trees with no obvious parents, this is revision.NULL_REVISION
85+ The default implementation simply uses get_parent_ids.
86+ :return: revision_id, as a unicode string.
87+ """
88+ parent_ids = self.get_parent_ids()
89+ if len(parent_ids) > 0:
90+ return parent_ids[0]
91+ else:
92+ return _mod_revision.NULL_REVISION
93+
94 def has_filename(self, filename):
95 """True if the tree has given filename."""
96 raise NotImplementedError(self.has_filename)
97
98=== modified file 'bzrlib/workingtree_4.py'
99--- bzrlib/workingtree_4.py 2009-05-23 04:55:52 +0000
100+++ bzrlib/workingtree_4.py 2009-05-28 12:36:11 +0000
101@@ -57,6 +57,7 @@
102 textui,
103 trace,
104 transform,
105+ tree,
106 urlutils,
107 views,
108 xml5,
109@@ -583,13 +584,9 @@
110 return kind
111
112 @needs_read_lock
113- def _last_revision(self):
114+ def last_revision(self):
115 """See Mutable.last_revision."""
116- parent_ids = self.current_dirstate().get_parent_ids()
117- if parent_ids:
118- return parent_ids[0]
119- else:
120- return _mod_revision.NULL_REVISION
121+ return tree.Tree.last_revision(self)
122
123 def lock_read(self):
124 """See Branch.lock_read, and WorkingTree.unlock."""