Patchwork [U-Boot,1/2] WIP: Changes to patman libraries

login
register
mail settings
Submitter Simon Glass
Date Oct. 31, 2012, 9:25 p.m.
Message ID <1351718752-6832-1-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/196009/
State Superseded, archived
Delegated to: Simon Glass
Headers show

Comments

Simon Glass - Oct. 31, 2012, 9:25 p.m.
These changes are required to the patman libraries. This is not a proper
patch yet, just sometime to try out.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 tools/patman/checkpatch.py      |    2 -
 tools/patman/command.py         |   86 ++++++--
 tools/patman/cros_subprocess.py |  402 +++++++++++++++++++++++++++++++++++++++
 tools/patman/gitutil.py         |  129 ++++++++++++-
 tools/patman/patchstream.py     |   38 +++-
 tools/patman/terminal.py        |   30 ++-
 6 files changed, 642 insertions(+), 45 deletions(-)
 create mode 100644 tools/patman/cros_subprocess.py
Wolfgang Denk - Oct. 31, 2012, 10:42 p.m.
Dear Simon Glass,

In message <1351718752-6832-1-git-send-email-sjg@chromium.org> you wrote:
> These changes are required to the patman libraries. This is not a proper
> patch yet, just sometime to try out.

...are required.  So.  And why exactly?  Or what is the purpose of
these changes?

Best regards,

Wolfgang Denk
Simon Glass - Oct. 31, 2012, 11:12 p.m.
Hi Wolfgang,

On Wed, Oct 31, 2012 at 3:42 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1351718752-6832-1-git-send-email-sjg@chromium.org> you wrote:
>> These changes are required to the patman libraries. This is not a proper
>> patch yet, just sometime to try out.
>
> ...are required.  So.  And why exactly?  Or what is the purpose of
> these changes?

Just so that people can try the builder if they want to. The patches
enhance functions in patman, mostly on the git side, so that the
builder can do its job. For example it needs to clone a repo, checkout
code into a different directory and work with branches a bit more.

This is not a useful patch for any other purpose (e.g. review) - it is
just a lump of code. If there is interest in this it will need to be
turned into proper patches.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Nobody goes to that restaurant anymore. It's too crowded.
Wolfgang Denk - Oct. 31, 2012, 11:40 p.m.
Dear Simon,

In message <CAPnjgZ3us4yoeqOHxozQ6VPHXhaKJdLWfb0HPWHWvnnUaZ0Ghg@mail.gmail.com> you wrote:
> 
> >> These changes are required to the patman libraries. This is not a proper
> >> patch yet, just sometime to try out.
> >
> > ...are required.  So.  And why exactly?  Or what is the purpose of
> > these changes?
> 
> Just so that people can try the builder if they want to. The patches
> enhance functions in patman, mostly on the git side, so that the
> builder can do its job. For example it needs to clone a repo, checkout
> code into a different directory and work with branches a bit more.
> 
> This is not a useful patch for any other purpose (e.g. review) - it is
> just a lump of code. If there is interest in this it will need to be
> turned into proper patches.

Hm...  I apologize, but I'm just an old man, and a bit slow of wits
these days.  I cannot review any such code without knowing what it is
suppose to acchieve.  Maybe you should add such explanations to the
commit message, even if it's only a WIP patch?  I would definitely
appreciate this (as it would help me to understand what this is all
about).

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Half of the people in the world are below average.
Simon Glass - Nov. 1, 2012, 12:04 a.m.
Hi Wolfgang,

On Wed, Oct 31, 2012 at 4:40 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon,
>
> In message <CAPnjgZ3us4yoeqOHxozQ6VPHXhaKJdLWfb0HPWHWvnnUaZ0Ghg@mail.gmail.com> you wrote:
>>
>> >> These changes are required to the patman libraries. This is not a proper
>> >> patch yet, just sometime to try out.
>> >
>> > ...are required.  So.  And why exactly?  Or what is the purpose of
>> > these changes?
>>
>> Just so that people can try the builder if they want to. The patches
>> enhance functions in patman, mostly on the git side, so that the
>> builder can do its job. For example it needs to clone a repo, checkout
>> code into a different directory and work with branches a bit more.
>>
>> This is not a useful patch for any other purpose (e.g. review) - it is
>> just a lump of code. If there is interest in this it will need to be
>> turned into proper patches.
>
> Hm...  I apologize, but I'm just an old man, and a bit slow of wits
> these days.  I cannot review any such code without knowing what it is
> suppose to acchieve.  Maybe you should add such explanations to the
> commit message, even if it's only a WIP patch?  I would definitely
> appreciate this (as it would help me to understand what this is all
> about).

:-) OK I see. I will add a proper commit message so it is properly
described, but please do understand it is WIP so far.

Regards,
Simon

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Half of the people in the world are below average.
Simon Glass - Nov. 1, 2012, 8:59 p.m.
Hi Wolfgang,

On Wed, Oct 31, 2012 at 5:04 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Wolfgang,
>
> On Wed, Oct 31, 2012 at 4:40 PM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Simon,
>>
>> In message <CAPnjgZ3us4yoeqOHxozQ6VPHXhaKJdLWfb0HPWHWvnnUaZ0Ghg@mail.gmail.com> you wrote:
>>>
>>> >> These changes are required to the patman libraries. This is not a proper
>>> >> patch yet, just sometime to try out.
>>> >
>>> > ...are required.  So.  And why exactly?  Or what is the purpose of
>>> > these changes?
>>>
>>> Just so that people can try the builder if they want to. The patches
>>> enhance functions in patman, mostly on the git side, so that the
>>> builder can do its job. For example it needs to clone a repo, checkout
>>> code into a different directory and work with branches a bit more.
>>>
>>> This is not a useful patch for any other purpose (e.g. review) - it is
>>> just a lump of code. If there is interest in this it will need to be
>>> turned into proper patches.
>>
>> Hm...  I apologize, but I'm just an old man, and a bit slow of wits
>> these days.  I cannot review any such code without knowing what it is
>> suppose to acchieve.  Maybe you should add such explanations to the
>> commit message, even if it's only a WIP patch?  I would definitely
>> appreciate this (as it would help me to understand what this is all
>> about).

Here are the notes for the changes. When I split this into commits I
will put these comments with each commit.

checkpatch.py:
- remove two commented-out lines which are no-longer needed

command.py:

Change these functions to return a CommandResult class instead of just
a return code.
This allows us to capture output, errors and return codes. Should we
need to expand
the functionality in the future, it will be fairly easy - just add new
members to the class.

Also provide an option to raise an exception on error (non-zero return
code), rather than
always just returning the return code. This can make it easier to
handle unforseen
errors.

Provide a back-door way of killing all tasks (although this needs further work).

cros_subprocess.py:

This is a slight enhancement of the build-in python subprocess module.
It permits
access to command output while it is in progress. This is important if
we want to
filter it for errors, etc., while still displaying it on the terminal.
Since some tasks can
take a minute to complete, it is not acceptable to show no output
during this time.

gitutils.py:

Several functions are enhanced so that you can specify a --git-dir
with the command,
and also a --work-tree. This allows us to work with git repositories outside the
current directory. This is needed for buildman, since it has multiple
threads working
in their own place with their own checked-out commit.

New methods are added to clone a repo, fetch updates from a repo, checkout a
commit, and obtain an expression for the list of commits in a branch.

patchstream.py

GetMetaDataForList() now supports --git-dir, and also allows an
existing Series to be
added too. This allows us to call it twice with different commit
ranges and get a single,
unified series. The old GetMetaData() function now calls this.

terminal.py:

This now supports both bright and normal ANSI colours.

Colours are automatically supressed if the stdout is not a terminal.
The avoids getting
ANSI characters in piped output.

Regards,
Simon

Patch

diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py
index d831087..4b6748a 100644
--- a/tools/patman/checkpatch.py
+++ b/tools/patman/checkpatch.py
@@ -70,8 +70,6 @@  def CheckPatch(fname, verbose=False):
                 '~/bin directory')
     item = {}
     stdout = command.Output(chk, '--no-tree', fname)
-    #pipe = subprocess.Popen(cmd, stdout=subprocess.PIPE)
-    #stdout, stderr = pipe.communicate()
 
     # total: 0 errors, 0 warnings, 159 lines checked
     re_stats = re.compile('total: (\\d+) errors, (\d+) warnings, (\d+)')
diff --git a/tools/patman/command.py b/tools/patman/command.py
index 4b00250..a67ade3 100644
--- a/tools/patman/command.py
+++ b/tools/patman/command.py
@@ -20,53 +20,95 @@ 
 #
 
 import os
-import subprocess
+import cros_subprocess
 
 """Shell command ease-ups for Python."""
 
-def RunPipe(pipeline, infile=None, outfile=None,
-            capture=False, oneline=False, hide_stderr=False):
+class CommandResult:
+    """A class which captures the result of executing a command.
+
+    Members:
+        stdout: stdout obtained from command, as a string
+        stderr: stderr obtained from command, as a string
+        return_code: Return code from command
+        exception: Exception received, or None if all ok
+    """
+    def __init__(self):
+        self.stdout = None
+        self.stderr = None
+        self.return_code = None
+        self.exception = None
+
+
+def RunPipe(pipe_list, infile=None, outfile=None,
+            capture=False, capture_stderr=False, oneline=False,
+            raise_on_error=True, cwd=None, **kwargs):
     """
     Perform a command pipeline, with optional input/output filenames.
 
-    hide_stderr     Don't allow output of stderr (default False)
+    Args:
+        pipe_list: List of command lines to execute. Each command line is
+            piped into the next, and is itself a list of strings. For
+            example [ ['ls', '.git'] ['wc'] ] will pipe the output of
+            'ls .git' into 'wc'.
+        infile: File to provide stdin to the pipeline
+        outfile: File to store stdout
+        capture: True to capture output
+        capture_stderr: True to capture stderr
+        oneline: True to strip newline chars from output
+        kwargs: Additional keyword arguments to cros_subprocess.Popen()
+    Returns:
+        CommandResult object
     """
+    result = CommandResult()
     last_pipe = None
+    pipeline = list(pipe_list)
     while pipeline:
         cmd = pipeline.pop(0)
-        kwargs = {}
         if last_pipe is not None:
             kwargs['stdin'] = last_pipe.stdout
         elif infile:
             kwargs['stdin'] = open(infile, 'rb')
         if pipeline or capture:
-            kwargs['stdout'] = subprocess.PIPE
+            kwargs['stdout'] = cros_subprocess.PIPE
         elif outfile:
             kwargs['stdout'] = open(outfile, 'wb')
-        if hide_stderr:
-            kwargs['stderr'] = open('/dev/null', 'wb')
+        if capture_stderr:
+            kwargs['stderr'] = cros_subprocess.PIPE
 
-        last_pipe = subprocess.Popen(cmd, **kwargs)
+        try:
+            last_pipe = cros_subprocess.Popen(cmd, cwd=cwd, **kwargs)
+        except Exception, err:
+            result.exception = err
+            print 'exception', pipe_list, err
+            raise Exception("Error running '%s': %s" % (pipe_list, str))
 
     if capture:
-        ret = last_pipe.communicate()[0]
-        if not ret:
-            return None
-        elif oneline:
-            return ret.rstrip('\r\n')
-        else:
-            return ret
+        result.stdout, result.stderr, result.combined = (
+                last_pipe.CommunicateFilter(None))
+        if result.stdout and oneline:
+            result.output = result.stdout.rstrip('\r\n')
+        result.return_code = last_pipe.wait()
     else:
-        return os.waitpid(last_pipe.pid, 0)[1] == 0
+        result.return_code = os.waitpid(last_pipe.pid, 0)[1]
+    if raise_on_error and result.return_code:
+        raise Exception("Error running '%s'" % pipe_list)
+    return result
 
 def Output(*cmd):
-    return RunPipe([cmd], capture=True)
+    return RunPipe([cmd], capture=True, raise_on_error=False).stdout
 
-def OutputOneLine(*cmd):
-    return RunPipe([cmd], capture=True, oneline=True)
+def OutputOneLine(*cmd, **kwargs):
+    raise_on_error = kwargs.pop('raise_on_error', True)
+    return (RunPipe([cmd], capture=True, oneline=True,
+            raise_on_error=raise_on_error,
+            **kwargs).stdout.strip())
 
 def Run(*cmd, **kwargs):
-    return RunPipe([cmd], **kwargs)
+    return RunPipe([cmd], **kwargs).stdout
 
 def RunList(cmd):
-    return RunPipe([cmd], capture=True)
+    return RunPipe([cmd], capture=True).stdout
+
+def StopAll():
+    cros_subprocess.stay_alive = False
diff --git a/tools/patman/cros_subprocess.py b/tools/patman/cros_subprocess.py
new file mode 100644
index 0000000..8b89387
--- /dev/null
+++ b/tools/patman/cros_subprocess.py
@@ -0,0 +1,402 @@ 
+#!/usr/bin/python
+
+# Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+#
+# Copyright (c) 2003-2005 by Peter Astrand <astrand@lysator.liu.se>
+# Licensed to PSF under a Contributor Agreement.
+# See http://www.python.org/2.4/license for licensing details.
+
+"""Subprocress execution
+
+This module holds a subclass of subprocess.Popen with our own required
+features.
+"""
+
+#TODO: Fix up indentation
+
+import errno
+import os
+import pty
+import select
+import subprocess
+import sys
+import unittest
+
+
+# Import these here so the caller does not need to import subprocess also.
+PIPE = subprocess.PIPE
+STDOUT = subprocess.STDOUT
+PIPE_PTY = -3   # Pipe output through a pty
+stay_alive = True
+
+
+class Popen(subprocess.Popen):
+  """Like subprocess.Popen with ptys and incremental output
+
+  This class deals with running a child process and filtering its output on
+  both stdout and stderr while it is running. We do this so we can monitor
+  progress, and possibly relay the output to the user if requested.
+
+  The class is similar to subprocess.Popen, the equivalent is something like:
+
+    Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
+  But this class has many fewer features, and two enhancement:
+
+  1. Rather than getting the output data only at the end, this class sends it
+     to a provided operation as it arrives.
+  2. We use pseudo terminals so that the child will hopefully flush its output
+     to us as soon as it is produced, rather than waiting for the end of a
+     line.
+
+  Use CommunicateFilter() to handle output from the subprocess.
+
+  """
+
+  def __init__(self, args, stdin=None, stdout=PIPE_PTY, stderr=PIPE_PTY,
+      shell=False, cwd=None, env=None, **kwargs):
+    """Cut-down constructor
+
+    Args:
+      args: Program and arguments for subprocess to execute.
+      stdin: See subprocess.Popen()
+      stdout: See subprocess.Popen(), except that we support the sentinel
+          value of cros_subprocess.PIPE_PTY.
+      stderr: See subprocess.Popen(), except that we support the sentinel
+          value of cros_subprocess.PIPE_PTY.
+      shell: See subprocess.Popen()
+      cwd: Working directory to change to for subprocess, or None if none.
+      env: Environment to use for this subprocess, or None to inherit parent.
+      kwargs: No other arguments are supported at the moment.  Passing other
+          arguments will cause a ValueError to be raised.
+    """
+    stdout_pty = None
+    stderr_pty = None
+
+    if stdout == PIPE_PTY:
+      stdout_pty = pty.openpty()
+      stdout = os.fdopen(stdout_pty[1])
+    if stderr == PIPE_PTY:
+      stderr_pty = pty.openpty()
+      stderr = os.fdopen(stderr_pty[1])
+
+    super(Popen, self).__init__(args, stdin=stdin,
+        stdout=stdout, stderr=stderr, shell=shell, cwd=cwd, env=env,
+        **kwargs)
+
+    # If we're on a PTY, we passed the slave half of the PTY to the subprocess.
+    # We want to use the master half on our end from now on.  Setting this here
+    # does make some assumptions about the implementation of subprocess, but
+    # those assumptions are pretty minor.
+
+    # Note that if stderr is STDOUT, then self.stderr will be set to None by
+    # this constructor.
+    if stdout_pty is not None:
+      self.stdout = os.fdopen(stdout_pty[0])
+    if stderr_pty is not None:
+      self.stderr = os.fdopen(stderr_pty[0])
+
+    # Insist that unit tests exist for other arguments we don't support.
+    if kwargs:
+      raise ValueError("Unit tests do not test extra args - please add tests")
+
+  def CommunicateFilter(self, output):
+    """Interact with process: Read data from stdout and stderr.
+
+    This method runs until end-of-file is reached, then waits for the
+    subprocess to terminate.
+
+    The output function is sent all output from the subprocess and must be
+    defined like this:
+
+      def Output([self,] stream, data)
+      Args:
+        stream: the stream the output was received on, which will be
+            sys.stdout or sys.stderr.
+        data: a string containing the data
+
+    Note: The data read is buffered in memory, so do not use this
+    method if the data size is large or unlimited.
+
+    Args:
+      output: Function to call with each fragment of output.
+
+    Returns:
+      A tuple (stdout, stderr, combined) which is the data received on
+      stdout, stderr and the combined data (interleaved stdout and stderr).
+
+      Note that the interleaved output will only be sensible if you have
+      set both stdout and stderr to PIPE or PIPE_PTY. Even then it depends on
+      the timing of the output in the subprocess. If a subprocess flips
+      between stdout and stderr quickly in succession, by the time we come to
+      read the output from each we may see several lines in each, and will read
+      all the stdout lines, then all the stderr lines. So the interleaving
+      may not be correct. In this case you might want to pass
+      stderr=cros_subprocess.STDOUT to the constructor.
+
+      This feature is still useful for subprocesses where stderr is
+      rarely used and indicates an error.
+
+      Note also that if you set stderr to STDOUT, then stderr will be empty
+      and the combined output will just be the same as stdout.
+    """
+
+    read_set = []
+    write_set = []
+    stdout = None # Return
+    stderr = None # Return
+
+    if self.stdin:
+      # Flush stdio buffer.  This might block, if the user has
+      # been writing to .stdin in an uncontrolled fashion.
+      self.stdin.flush()
+      if input:
+        write_set.append(self.stdin)
+      else:
+        self.stdin.close()
+    if self.stdout:
+      read_set.append(self.stdout)
+      stdout = []
+    if self.stderr and self.stderr != self.stdout:
+      read_set.append(self.stderr)
+      stderr = []
+    combined = []
+
+    input_offset = 0
+    while read_set or write_set:
+      try:
+        rlist, wlist, _ = select.select(read_set, write_set, [], 0.2)
+      except select.error, e:
+        if e.args[0] == errno.EINTR:
+          continue
+        raise
+
+      if not stay_alive:
+          self.terminate()
+
+      if self.stdin in wlist:
+        # When select has indicated that the file is writable,
+        # we can write up to PIPE_BUF bytes without risk
+        # blocking.  POSIX defines PIPE_BUF >= 512
+        chunk = input[input_offset : input_offset + 512]
+        bytes_written = os.write(self.stdin.fileno(), chunk)
+        input_offset += bytes_written
+        if input_offset >= len(input):
+          self.stdin.close()
+          write_set.remove(self.stdin)
+
+      if self.stdout in rlist:
+        data = ""
+        # We will get an error on read if the pty is closed
+        try:
+          data = os.read(self.stdout.fileno(), 1024)
+        except OSError:
+          pass
+        if data == "":
+          self.stdout.close()
+          read_set.remove(self.stdout)
+        else:
+          stdout.append(data)
+          combined.append(data)
+          if output:
+            output(sys.stdout, data)
+      if self.stderr in rlist:
+        data = ""
+        # We will get an error on read if the pty is closed
+        try:
+          data = os.read(self.stderr.fileno(), 1024)
+        except OSError:
+          pass
+        if data == "":
+          self.stderr.close()
+          read_set.remove(self.stderr)
+        else:
+          stderr.append(data)
+          combined.append(data)
+          if output:
+            output(sys.stderr, data)
+
+    # All data exchanged.  Translate lists into strings.
+    if stdout is not None:
+      stdout = ''.join(stdout)
+    else:
+      stdout = ''
+    if stderr is not None:
+      stderr = ''.join(stderr)
+    else:
+      stderr = ''
+    combined = ''.join(combined)
+
+    # Translate newlines, if requested.  We cannot let the file
+    # object do the translation: It is based on stdio, which is
+    # impossible to combine with select (unless forcing no
+    # buffering).
+    if self.universal_newlines and hasattr(file, 'newlines'):
+      if stdout:
+        stdout = self._translate_newlines(stdout)
+      if stderr:
+        stderr = self._translate_newlines(stderr)
+
+    self.wait()
+    return (stdout, stderr, combined)
+
+
+# Just being a unittest.TestCase gives us 14 public methods.  Unless we
+# disable this, we can only have 6 tests in a TestCase.  That's not enough.
+#
+# pylint: disable=R0904
+
+class TestSubprocess(unittest.TestCase):
+  """Our simple unit test for this module"""
+
+  class MyOperation:
+    """Provides a operation that we can pass to Popen"""
+    def __init__(self, input_to_send=None):
+      """Constructor to set up the operation and possible input.
+
+      Args:
+        input_to_send: a text string to send when we first get input. We will
+          add \r\n to the string.
+      """
+      self.stdout_data = ''
+      self.stderr_data = ''
+      self.combined_data = ''
+      self.stdin_pipe = None
+      self._input_to_send = input_to_send
+      if input_to_send:
+        pipe = os.pipe()
+        self.stdin_read_pipe = pipe[0]
+        self._stdin_write_pipe = os.fdopen(pipe[1], 'w')
+
+    def Output(self, stream, data):
+      """Output handler for Popen. Stores the data for later comparison"""
+      if stream == sys.stdout:
+        self.stdout_data += data
+      if stream == sys.stderr:
+        self.stderr_data += data
+      self.combined_data += data
+
+      # Output the input string if we have one.
+      if self._input_to_send:
+        self._stdin_write_pipe.write(self._input_to_send + '\r\n')
+        self._stdin_write_pipe.flush()
+
+  def _BasicCheck(self, plist, oper):
+    """Basic checks that the output looks sane."""
+    self.assertEqual(plist[0], oper.stdout_data)
+    self.assertEqual(plist[1], oper.stderr_data)
+    self.assertEqual(plist[2], oper.combined_data)
+
+    # The total length of stdout and stderr should equal the combined length
+    self.assertEqual(len(plist[0]) + len(plist[1]), len(plist[2]))
+
+  def test_simple(self):
+    """Simple redirection: Get process list"""
+    oper = TestSubprocess.MyOperation()
+    plist = Popen(['ps']).CommunicateFilter(oper.Output)
+    self._BasicCheck(plist, oper)
+
+  def test_stderr(self):
+    """Check stdout and stderr"""
+    oper = TestSubprocess.MyOperation()
+    cmd = 'echo fred >/dev/stderr && false || echo bad'
+    plist = Popen([cmd], shell=True).CommunicateFilter(oper.Output)
+    self._BasicCheck(plist, oper)
+    self.assertEqual(plist [0], 'bad\r\n')
+    self.assertEqual(plist [1], 'fred\r\n')
+
+  def test_shell(self):
+    """Check with and without shell works"""
+    oper = TestSubprocess.MyOperation()
+    cmd = 'echo test >/dev/stderr'
+    self.assertRaises(OSError, Popen, [cmd], shell=False)
+    plist = Popen([cmd], shell=True).CommunicateFilter(oper.Output)
+    self._BasicCheck(plist, oper)
+    self.assertEqual(len(plist [0]), 0)
+    self.assertEqual(plist [1], 'test\r\n')
+
+  def test_list_args(self):
+    """Check with and without shell works using list arguments"""
+    oper = TestSubprocess.MyOperation()
+    cmd = ['echo', 'test', '>/dev/stderr']
+    plist = Popen(cmd, shell=False).CommunicateFilter(oper.Output)
+    self._BasicCheck(plist, oper)
+    self.assertEqual(plist [0], ' '.join(cmd[1:]) + '\r\n')
+    self.assertEqual(len(plist [1]), 0)
+
+    oper = TestSubprocess.MyOperation()
+
+    # this should be interpreted as 'echo' with the other args dropped
+    cmd = ['echo', 'test', '>/dev/stderr']
+    plist = Popen(cmd, shell=True).CommunicateFilter(oper.Output)
+    self._BasicCheck(plist, oper)
+    self.assertEqual(plist [0], '\r\n')
+
+  def test_cwd(self):
+    """Check we can change directory"""
+    for shell in (False, True):
+      oper = TestSubprocess.MyOperation()
+      plist = Popen('pwd', shell=shell, cwd='/tmp').CommunicateFilter(oper.Output)
+      self._BasicCheck(plist, oper)
+      self.assertEqual(plist [0], '/tmp\r\n')
+
+  def test_env(self):
+    """Check we can change environment"""
+    for add in (False, True):
+      oper = TestSubprocess.MyOperation()
+      env = os.environ
+      if add:
+        env ['FRED'] = 'fred'
+      cmd = 'echo $FRED'
+      plist = Popen(cmd, shell=True, env=env).CommunicateFilter(oper.Output)
+      self._BasicCheck(plist, oper)
+      self.assertEqual(plist [0], add and 'fred\r\n' or '\r\n')
+
+  def test_extra_args(self):
+    """Check we can't add extra arguments"""
+    self.assertRaises(ValueError, Popen, 'true', close_fds=False)
+
+  def test_basic_input(self):
+    """Check that incremental input works
+
+    We set up a subprocess which will prompt for name. When we see this prompt
+    we send the name as input to the process. It should then print the name
+    properly to stdout.
+    """
+    oper = TestSubprocess.MyOperation('Flash')
+    prompt = 'What is your name?: '
+    cmd = 'echo -n "%s"; read name; echo Hello $name' % prompt
+    plist = Popen([cmd], stdin=oper.stdin_read_pipe,
+        shell=True).CommunicateFilter(oper.Output)
+    self._BasicCheck(plist, oper)
+    self.assertEqual(len(plist [1]), 0)
+    self.assertEqual(plist [0], prompt + 'Hello Flash\r\r\n')
+
+  #TODO(sjg): Add test for passing PIPE in case underlying subprocess breaks.
+  #TODO(sjg): Add test for passing a file handle also.
+
+  def test_isatty(self):
+    """Check that ptys appear as terminals to the subprocess"""
+    oper = TestSubprocess.MyOperation()
+    cmd = ('if [ -t %d ]; then echo "terminal %d" >&%d; '
+        'else echo "not %d" >&%d; fi;')
+    both_cmds = ''
+    for fd in (1, 2):
+      both_cmds += cmd % (fd, fd, fd, fd, fd)
+    plist = Popen(both_cmds, shell=True).CommunicateFilter(oper.Output)
+    self._BasicCheck(plist, oper)
+    self.assertEqual(plist [0], 'terminal 1\r\n')
+    self.assertEqual(plist [1], 'terminal 2\r\n')
+
+    # Now try with PIPE and make sure it is not a terminal
+    oper = TestSubprocess.MyOperation()
+    plist = Popen(both_cmds, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+        shell=True).CommunicateFilter(oper.Output)
+    self._BasicCheck(plist, oper)
+    self.assertEqual(plist [0], 'not 1\n')
+    self.assertEqual(plist [1], 'not 2\n')
+
+if __name__ == '__main__':
+  unittest.main()
diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py
index 72d37a0..5958439 100644
--- a/tools/patman/gitutil.py
+++ b/tools/patman/gitutil.py
@@ -23,11 +23,12 @@  import command
 import re
 import os
 import series
-import settings
 import subprocess
 import sys
 import terminal
 
+import settings
+
 
 def CountCommitsToBranch():
     """Returns number of commits between HEAD and the tracking branch.
@@ -40,10 +41,123 @@  def CountCommitsToBranch():
     """
     pipe = [['git', 'log', '--no-color', '--oneline', '@{upstream}..'],
             ['wc', '-l']]
-    stdout = command.RunPipe(pipe, capture=True, oneline=True)
+    stdout = command.RunPipe(pipe, capture=True, oneline=True).stdout
+    patch_count = int(stdout)
+    return patch_count
+
+def GetUpstream(git_dir, branch):
+    """Returns the name of the upstream for a branch
+
+    Args:
+        git_dir: Git directory containing repo
+        branch: Name of branch
+
+    Returns:
+        Name of upstream branch (e.g. 'upstream/master') or None if none
+    """
+    remote = command.OutputOneLine('git', '--git-dir', git_dir, 'config',
+            'branch.%s.remote' % branch)
+    merge = command.OutputOneLine('git', '--git-dir', git_dir, 'config',
+            'branch.%s.merge' % branch)
+    if remote == '.':
+        return merge
+    elif remote and merge:
+        leaf = merge.split('/')[-1]
+        return '%s/%s' % (remote, leaf)
+    else:
+        raise ValueError, ("Cannot determine upstream branch for branch "
+                "'%s' remote='%s', merge='%s'" % (branch, remote, merge))
+
+
+def GetRangeInBranch(git_dir, branch, include_upstream=False):
+    """Returns an expression for the commits in the given branch.
+
+    Args:
+        git_dir: Directory containing git repo
+        branch: Name of branch
+    Return:
+        Expression in the form 'upstream..branch' which can be used to
+        access the commits.
+    """
+    upstream = GetUpstream(git_dir, branch)
+    return '%s%s..%s' % (upstream, '~' if include_upstream else '', branch)
+
+def CountCommitsInBranch(git_dir, branch, include_upstream=False):
+    """Returns the number of commits in the given branch.
+
+    Args:
+        git_dir: Directory containing git repo
+        branch: Name of branch
+    Return:
+        Number of patches that exist on top of the branch
+    """
+    range_expr = GetRangeInBranch(git_dir, branch, include_upstream)
+    pipe = [['git', '--git-dir', git_dir, 'log', '--oneline', range_expr],
+            ['wc', '-l']]
+    result = command.RunPipe(pipe, capture=True, oneline=True)
+    patch_count = int(result.stdout)
+    return patch_count
+
+def CountCommits(commit_range):
+    """Returns the number of commits in the given range.
+
+    Args:
+        commit_range: Range of commits to count (e.g. 'HEAD..base')
+    Return:
+        Number of patches that exist on top of the branch
+    """
+    pipe = [['git', 'log', '--oneline', commit_range],
+            ['wc', '-l']]
+    stdout = command.RunPipe(pipe, capture=True, oneline=True).stdout
     patch_count = int(stdout)
     return patch_count
 
+def Checkout(commit_hash, git_dir=None, work_tree=None, force=False):
+    """Checkout the selected commit for this build
+
+    Args:
+        commit_hash: Commit hash to check out
+    """
+    pipe = ['git']
+    if git_dir:
+        pipe.extend(['--git-dir', git_dir])
+    if work_tree:
+        pipe.extend(['--work-tree', work_tree])
+    pipe.append('checkout')
+    if force:
+        pipe.append('-f')
+    pipe.append(commit_hash)
+    result = command.RunPipe([pipe], capture=True, raise_on_error=False)
+    if result.return_code != 0:
+        raise OSError, 'git checkout (%s): %s' % (pipe, result.stderr)
+
+def Clone(git_dir, output_dir):
+    """Checkout the selected commit for this build
+
+    Args:
+        commit_hash: Commit hash to check out
+    """
+    pipe = ['git', 'clone', git_dir, '.']
+    result = command.RunPipe([pipe], capture=True, cwd=output_dir)
+    if result.return_code != 0:
+        raise OSError, 'git clone: %s' % result.stderr
+
+def Fetch(git_dir=None, work_tree=None):
+    """Fetch from the origin repo
+
+    Args:
+        commit_hash: Commit hash to check out
+    """
+    pipe = ['git']
+    if git_dir:
+        pipe.extend(['--git-dir', git_dir])
+    if work_tree:
+        pipe.extend(['--work-tree', work_tree])
+    pipe.append('fetch')
+    result = command.RunPipe([pipe], capture=True)
+    if result.return_code != 0:
+        raise OSError, 'git fetch: %s' % result.stderr
+
 def CreatePatches(start, count, series):
     """Create a series of patches from the top of the current branch.
 
@@ -352,7 +466,8 @@  def GetAliasFile():
     Returns:
         Filename of git alias file, or None if none
     """
-    fname = command.OutputOneLine('git', 'config', 'sendemail.aliasesfile')
+    fname = command.OutputOneLine('git', 'config', 'sendemail.aliasesfile',
+            raise_on_error=False)
     if fname:
         fname = os.path.join(GetTopLevel(), fname.strip())
     return fname
@@ -384,6 +499,14 @@  def Setup():
     if alias_fname:
         settings.ReadGitAliases(alias_fname)
 
+def GetHead():
+    """Get the hash of the current HEAD
+
+    Returns:
+        Hash of HEAD
+    """
+    return command.OutputOneLine('git', 'show', '-s', '--pretty=format:%H')
+
 if __name__ == "__main__":
     import doctest
 
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index ad280cc..db2cc6c 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -237,7 +237,8 @@  class PatchStream:
         # Detect the start of a new commit
         elif commit_match:
             self.CloseCommit()
-            self.commit = commit.Commit(commit_match.group(1)[:7])
+            # TODO: We should store the whole hash, and just display a subset
+            self.commit = commit.Commit(commit_match.group(1)[:8])
 
         # Detect tags in the commit message
         elif tag_match:
@@ -334,26 +335,47 @@  class PatchStream:
         self.Finalize()
 
 
-def GetMetaData(start, count):
+def GetMetaDataForList(commit_range, git_dir=None, count=None,
+                       series = Series()):
     """Reads out patch series metadata from the commits
 
     This does a 'git log' on the relevant commits and pulls out the tags we
     are interested in.
 
     Args:
-        start: Commit to start from: 0=HEAD, 1=next one, etc.
-        count: Number of commits to list
+        commit_range: Range of commits to count (e.g. 'HEAD..base')
+        git_dir: Path to git repositiory (None to use default)
+        count: Number of commits to list, or None for no limit
+        series: Series object to add information into. By default a new series
+            is started.
+    Returns:
+        A Series object containing information about the commits.
     """
-    pipe = [['git', 'log', '--no-color', '--reverse', 'HEAD~%d' % start,
-	'-n%d' % count]]
-    stdout = command.RunPipe(pipe, capture=True)
-    series = Series()
+    params = ['git', 'log', '--no-color', '--reverse', commit_range]
+    if count is not None:
+        params[2:2] = ['-n%d' % count]
+    if git_dir:
+        params[1:1] = ['--git-dir', git_dir]
+    pipe = [params]
+    stdout = command.RunPipe(pipe, capture=True).stdout
     ps = PatchStream(series, is_log=True)
     for line in stdout.splitlines():
         ps.ProcessLine(line)
     ps.Finalize()
     return series
 
+def GetMetaData(start, count):
+    """Reads out patch series metadata from the commits
+
+    This does a 'git log' on the relevant commits and pulls out the tags we
+    are interested in.
+
+    Args:
+        start: Commit to start from: 0=HEAD, 1=next one, etc.
+        count: Number of commits to list
+    """
+    return GetMetaDataForList('HEAD~%d' % start, None, count)
+
 def FixPatch(backup_dir, fname, series, commit):
     """Fix up a patch file, by adding/removing as required.
 
diff --git a/tools/patman/terminal.py b/tools/patman/terminal.py
index 838c828..337a2a4 100644
--- a/tools/patman/terminal.py
+++ b/tools/patman/terminal.py
@@ -24,24 +24,32 @@ 
 This module handles terminal interaction including ANSI color codes.
 """
 
+import os
+import sys
+
+# Selection of when we want our output to be colored
+COLOR_IF_TERMINAL, COLOR_ALWAYS, COLOR_NEVER = range(3)
+
 class Color(object):
   """Conditionally wraps text in ANSI color escape sequences."""
   BLACK, RED, GREEN, YELLOW, BLUE, MAGENTA, CYAN, WHITE = range(8)
   BOLD = -1
-  COLOR_START = '\033[1;%dm'
+  BRIGHT_START = '\033[1;%dm'
+  NORMAL_START = '\033[22;%dm'
   BOLD_START = '\033[1m'
   RESET = '\033[0m'
 
-  def __init__(self, enabled=True):
+  def __init__(self, colored=COLOR_IF_TERMINAL):
     """Create a new Color object, optionally disabling color output.
 
     Args:
       enabled: True if color output should be enabled. If False then this
         class will not add color codes at all.
     """
-    self._enabled = enabled
+    self._enabled = (colored == COLOR_ALWAYS or
+        (colored == COLOR_IF_TERMINAL and os.isatty(sys.stdout.fileno())))
 
-  def Start(self, color):
+  def Start(self, color, bright=True):
     """Returns a start color code.
 
     Args:
@@ -52,7 +60,8 @@  class Color(object):
       otherwise returns empty string
     """
     if self._enabled:
-      return self.COLOR_START % (color + 30)
+        base = self.BRIGHT_START if bright else self.NORMAL_START
+        return base % (color + 30)
     return ''
 
   def Stop(self):
@@ -63,10 +72,10 @@  class Color(object):
       returns empty string
     """
     if self._enabled:
-      return self.RESET
+        return self.RESET
     return ''
 
-  def Color(self, color, text):
+  def Color(self, color, text, bright=True):
     """Returns text with conditionally added color escape sequences.
 
     Keyword arguments:
@@ -78,9 +87,10 @@  class Color(object):
       returns text with color escape sequences based on the value of color.
     """
     if not self._enabled:
-      return text
+        return text
     if color == self.BOLD:
-      start = self.BOLD_START
+        start = self.BOLD_START
     else:
-      start = self.COLOR_START % (color + 30)
+        base = self.BRIGHT_START if bright else self.NORMAL_START
+        start = base % (color + 30)
     return start + text + self.RESET