Merge lp:~zyga/bzr-fastimport/fixes into lp:bzr-fastimport

Proposed by Zygmunt Krynicki on 2012-12-03
Status: Merged
Merged at revision: 357
Proposed branch: lp:~zyga/bzr-fastimport/fixes
Merge into: lp:bzr-fastimport
Diff against target: 13 lines (+2/-1)
1 file modified
branch_updater.py (+2/-1)
To merge this branch: bzr merge lp:~zyga/bzr-fastimport/fixes
Reviewer Review Type Date Requested Status
Richard Wilbur 2012-12-03 Approve on 2012-12-05
Review via email: mp+137528@code.launchpad.net

Description of the change

This branch fixes some of the bugs I've seen when interacting with git.

To post a comment you must log in.
Richard Wilbur (richard-wilbur) wrote :

What are the bugs you've seen when interacting with git? Have any of them been reported in the bazaar bug database? If so, a bug reference here would be helpful for reviewers.

If the bug reports appear in the bug database, it is normal practice to create one or more tests to validate the code. Test(s) fail without the patch and pass with it.

If this test requirement turns out to be a barrier to development, point us to the bug reports and we should be able to help create the tests.

Richard Wilbur (richard-wilbur) wrote :

I didn't read the revision comment till just now as it wasn't in the E-mail message sent to the bazaar-core dev group. Looks like this is fixing code rot in bzr-fastimport and thus would not need a test.

Please add a comment with the bug reference from bzr-stat that you mentioned.

review: Needs Information
Zygmunt Krynicki (zyga) wrote :

> What are the bugs you've seen when interacting with git? Have any of them
> been reported in the bazaar bug database? If so, a bug reference here would
> be helpful for reviewers.

As mentioned in the commit message, it replaces usage of get_ancestry(), which no longer exists, with the equivalent code that works with the current API. The relevant bug for that in bzr-stat is:

https://bugs.launchpad.net/ubuntu/+source/bzr-stats/+bug/1040560

If you want to write a test case for that sure, right now merging this is better than not merging it at all since get_ancestry is clearly gone and the code cannot be more broken.

Thanks
ZK

Richard Wilbur (richard-wilbur) wrote :

Thanks for the reference to the bzr-stat bug and for cleaning up the code rot. I wholeheartedly approve this merge!
+2

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'branch_updater.py'
2--- branch_updater.py 2011-10-06 00:11:52 +0000
3+++ branch_updater.py 2012-12-03 11:12:39 +0000
4@@ -165,7 +165,8 @@
5 # apply tags known in this branch
6 my_tags = {}
7 if self.tags:
8- ancestry = self.repo.get_ancestry(last_rev_id)
9+ graph = self.repo.get_graph()
10+ ancestry = [r for (r, ps) in graph.iter_ancestry([last_rev_id]) if ps is not None]
11 for tag,rev in self.tags.items():
12 if rev in ancestry:
13 my_tags[tag] = rev

Subscribers

People subscribed via source and target branches