Comment 1 for bug 208606

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!