AutoPPA should use bzrlib instead of using pexpect to run bzr in a subprocess

[Jamu Kakar]

This would be so much better.

[Jamu Kakar (jkakar)]

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
sui...

[Andrew Hunter (rexbron)]

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

Jamu Kakar wrote:
| 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:

That is great, this was the results of my initial hacking.

|
| [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.

Then we need to move the warning in text.py and remove it from there.

|
|
| [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)
|

Sure.

| [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.
|

I will double check to make sure I am using spaces instead of tabs.

|
| [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.

I did this because os.makedirs() raises an OSError if the specified
directory already exists. Suggestions?

|
|
| [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:

Easy enough

|
|
| [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.

Ahh ok. I have not really read up on the proper way to do exceptions in
python and attempted to infer it from other bits of code.

|
|
| [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()?

In python, when you call write() on a file, it just writes to the file
object in memory. You...

[Jamu Kakar (jkakar)]

Hi Andrew,

Andrew Hunter wrote:
 > Then we need to move the warning in text.py and remove it from there.

I just had a peek at text.py and I'm not sure I follow. At this stage
it's probably fine to let any exceptions bubble up and blow up. At
some point it'd be nice to have some top-level exception handling that
presents errors to users in a nice way.

One of the things that hasn't been entirely clear to me is what the
best way to do this is. One the one hand, it's not great to let
errors blow up the application because it's (1) an ugly user interface
for users and (2) can leave cruft lying around, depending on where the
error occurs. On the other hand, I've actually wanted to have things
left in whatever state they are when problems happen so that I can
debug issues. Perhaps the right thing is to have both behaviours: we
clean up in the normal case and leave cruft lying around when (one of)
the --no-cleanup option is present or when an AUTOPPA_DEBUG
environment variable is defined.

 > I did this because os.makedirs() raises an OSError if the specified
 > directory already exists. Suggestions?

Ah, okay. I'd forgotten that detail about os.makedirs. I recommend
something like this:

if not os.path.exists(path):
     os.makedirs(path)

If the path already exists we should issue a warning, remove the
existing path and continue. I'm a little worried about the potential
to destroy user's data if, for example, they have put the wrong path
in ~/.autoppa.conf. A part of me thinks it might be better to bail
with a warning like:

The working directory $path already exists. Please remove this before
running AutoPPA again.

 > For the commit() call, I am specifying it by reading the file, but
 > re-reading the code, it makes more sense to just close the file and use
 > the changelog varriable in the commit. I'll fix that.

I think the using the changelog variable is the right way to go here,
too.

 > A Vcs system by their very nature log everything they do. Should not be
 > too difficult to use some of python's built in logging module to keep a
 > record of each action taken. I'll create a TODO list with these things
 > on it.

Yeah, that's what I figured. I'm not very familiar with bzrlib, so
this will require some investigation. Given that pexpect does it's
own logging, we'll have to look at changing existing things to use the
logging module.

 > Ok cool, I have never worked with self-testing in a codebase before
 > (self-taught programmer), but I can see the huge advantages of that. I
 > do the reading and fix the code.

It's the best way I know of to maintain production quality in a
codebase. The combination of having a rigourous test suite and
requiring code reviews of every branch before code is merged to trunk
works well to ensure that trunk is always stable. In fact, on the
projects I work on at Canonical (Landscape, Storm and AutoPPA) we
require two reviews per branch before merging to ensure that whatever
lands in trunk is stable and production-ready. If you have questions
about how to do TDD, or about any aspects of the testing
infrastructure please ask.

Thanks!

[Jamu Kakar (jkakar)]

I'm assigning this to myself since I have time to work on it right
now.

[Jamu Kakar (jkakar)]

I'm not going to fix this, in favour of pursuing a different design.