Thank you very much for working on this branch. I'm guessing that it isn't complete yet, but I decided to review it anyway. Following are my initial set of comments: [1] + +class MergeError(AutoPPAError): + """Raised when there are merge conflicts that must be resolved manually.""" + print "There are one or more merge conflicts that must be resolved manually." There shouldn't be a print statement here. One problem with this is that the output generated when the test suite is run will be polluted when MergeError's are raised. [2] +from bzrlib import branch, workingtree + self.source_branch = branch.Branch.open(self.branch) + self.source_working_tree = workingtree.WorkingTree.open(self.branch) Please use: from bzrlib.branch import Branch, bzrdir from bzrlib.workingtree import WorkingTree and + self.source_branch = Branch.open(self.branch) + self.source_working_tree = WorkingTree.open(self.branch) [3] - self.expect.run("mkdir -p %s" % self.repository, logfile=self.logfile) + try: + os.makedirs(self.repository) #Log this somehow? + except OSError: + pass #Should we abort? Please use spaces instead of tabs. There are tabs in a few other places. [4] + try: + os.makedirs(self.repository) #Log this somehow? + except OSError: + pass #Should we abort? I would be inclined remove the try/except block an let any errors bubble up to the outer application layer. [5] + conflicts = self.source_branch.merge_from_branch(self.release_branch) + if conflicts > 0: This could be simplified to: conflicts = self.source_branch.merge_from_branch( self.release_branch) if conflicts: [6] + raise MergeError A message should be provided when an exception is raised. I recommend: raise MergeError( "Couldn't merge working branch. Encountered " "conflicts: %s" % conflicts) Even better would be to include data about exactly what the conflicts are. [7] This change: - file.close() + file.flush() is followed by this further on: + file.close() I'm not getting the logic here. Is there a reason the file can't be closed where you've put the call to flush()? [8] One of the things the previous pexpect-using logic did was log the commands being run. It'd be nice to log messages about the various things that are happening with bzrlib, such as when prepared files are being reverted, when changes are committed, etc. Maybe bzrlib already does this and we need to figure out a way to wire it's logging up to AutoPPA's logging? [9] A number of existing tests are failing. They should be updated to test the new logic. I highly recommend you make changes using a test-driven development style to ensure that new functionality is tested properly and to avoid the situation where you have many breaking tests. You can run the tests using trial, the Twisted test runner. You'll want to installed python-twisted and run the test suite with: trial autoppa [10] There are a few lines longer than 80 characters. I've enjoyed what I've seen so far and am looking forward to seeing where you take this!