launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Review: Approve code

Overall, this looks good.. 

Perhaps I am simply paranoid (and I couldn't find the tinfoil hat today...), but anyway: I feel a bit uncomfortable about this:

+    def getStackedOnBranches(self):
+        """See `IBranch`."""
+        if not self.stacked_on:
+            return []
+        stacked_on_branches = [self.stacked_on]
+        stacked_on_branches.extend(self.stacked_on.getStackedOnBranches())
+        return stacked_on_branches

Sure, the stacked_on relation should not be circular -- but we are working on "DB copy representation" of the original Bazaar relation, which might have its own bugs, and Bazaar might also (probably inadvertently) allow circular stacked_on relations. And this would lead to an infinite loop. What about something like this:

stacked_on_branches = []
branch = self
while branch.stacked_on and branch.stacked_on not in stacked_on_branches:
    stacked_on_branches.append(branch.stacked_on)
    branch = branch.stacked_on
return stacked_on_branches


-- 
https://code.launchpad.net/~wallyworld/launchpad/view-private-stacked-branch-393566/+merge/114784
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.