Merge ~nacc/git-ubuntu:gu-review into git-ubuntu:master

[Nish Aravamudan]

This is probably not perfect, but it's basically (right now) just a wrapper around other commands, after the refactoring to call into them directly without the shell.

Bikesheddable.

Esp.: do we want review to create local repos? Or only with a flag? This was suggested in a recent bug where behaviorally only clone is allowed to create repositories.

Even less mergeable now, but shows how the interplay between modules can work after further refactoring.

[Robie Basak (racb)]

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:e0831bb2864a48129d9143f1b0e32831634101e8
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/26/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Tests
    FAILED: Build

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/26/rebuild

review: Needs Fixing (continuous-integration)

42fab63... by Nish Aravamudan on 2017-09-08

gitubuntu/__main__: drop dead code

In 087e93ca ("git ubuntu: suppress help for override files"),
the use of help_text was dropped. Drop it's (re-)definition too.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:ad8c802eddc58f40d22c304e5ad3468a2ebf1ce5
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/39/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Style Check

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/39/rebuild

review: Needs Fixing (continuous-integration)

a237f87... by Nish Aravamudan on 2017-09-08

source_information: set _spph to None

It is checked for None in the accessor.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:ad8c802eddc58f40d22c304e5ad3468a2ebf1ce5
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/43/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    FAILED: Integration Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/43/rebuild

review: Needs Fixing (continuous-integration)

Unmerged commits

d672a16... by Nish Aravamudan on 2017-09-08

wip

4184195... by Nish Aravamudan on 2017-07-18

git ubuntu review: add new subcommand

Given a MP URL, this attempts to provide a local directory to use for
reviewing the MP (and allows one to auto-approve with lint results).

LP: #1702960

296cc80... by Nish Aravamudan on 2017-09-06

clone: have main return the resulting directory path

5d0cb19... by Nish Aravamudan on 2017-09-06

git ubuntu: refactor main to call module parse_args

This is the final step in the refactor -- rather than using per-module
classes, just import each module in __main__ in order to call its
parse_args and func (which in the current implementation is that
module's cli_main).

Before merging with master, I think we will squash all of these
together.

At this point, all errors from pylint3 are from pygit2 imports.

LP: #1707321

0e4feb7... by Nish Aravamudan on 2017-09-06

submit: refactor class

Leave only parse_args and cli_main in the class. Make cli_main nothing
more than a wrapper to the module's main function.

LP: #1707321

0c3fef9... by Nish Aravamudan on 2017-09-06

submit: use lint.derive_target_branch

submit can be used for distributed development among team members by
proposing MPs against each other's repositories, via the --target-user
flag. Rather than having submit-specific code for deriving the target
branch, use the existing code from lint, after extending it to take a
namespace argument.

30ac7a9... by Nish Aravamudan on 2017-09-06

tag: refactor class

Leave only parse_args and cli_main in the class. Make cli_main nothing
more than a wrapper to the module's main function.

LP: #1707321

7ac5d05... by Nish Aravamudan on 2017-09-06

remote: refactor class

Leave only parse_args and cli_main in the class. Make cli_main nothing
more than a wrapper to the module's main function.

LP: #1707321

e46d999... by Nish Aravamudan on 2017-09-05

queue: refactor class

Leave only parse_args and cli_main in the class. Make cli_main nothing
more than a wrapper to the module's main function.

LP: #1707321

27c18cc... by Nish Aravamudan on 2017-09-05

merge: refactor class

Leave only parse_args and cli_main in the class. Make cli_main nothing
more than a wrapper to the module's main function.

LP: #1707321

diff --git a/gitubuntu/__main__.py b/gitubuntu/__main__.py
index 1b75213..3e03b0d 100644
--- a/gitubuntu/__main__.py
+++ b/gitubuntu/__main__.py
@@ -1,5 +1,19 @@
 #!/usr/bin/env python3
 
+from collections import namedtuple
+TopLevelDefaults = namedtuple(
+    'TopLevelDefaults',
+    [
+        'verbose',
+        'retries',
+        'retry_backoffs',
+        'proto',
+        'parentfile',
+        'pullfile',
+    ],
+)
+top_level_defaults = object()
+
 def main():
     try:
         import argparse
@@ -21,6 +35,22 @@ def main():
             logging.error('Is %s installed?', pkg)
             sys.exit(1)
 
+        global top_level_defaults
+        top_level_defaults = TopLevelDefaults(
+            verbose=False,
+            retries=3,
+            retry_backoffs = [2 ** i for i in range(3)],
+            proto='https',
+            parentfile=pkg_resources.resource_filename(
+                'gitubuntu',
+                'parent_overrides.txt',
+            ),
+            pullfile=pkg_resources.resource_filename(
+                'gitubuntu',
+                'pull_overrides.txt',
+            ),
+        )
+
         logging.getLogger('keyring').setLevel(logging.WARNING)
 
         known_subcommands = {
@@ -36,6 +66,7 @@ def main():
             'remote': 'gitubuntu.remote',
             'submit': 'gitubuntu.submit',
             'lint': 'gitubuntu.lint',
+            'review': 'gitubuntu.review',
         }
 
         known_network_subcommands = {
@@ -48,6 +79,7 @@ def main():
             'queue',
             'remote',
             'submit',
+            'review',
         }
 
         parser = argparse.ArgumentParser(
@@ -68,7 +100,8 @@ def main():
         base_subparser.add_argument(
             '-v', '--verbose',
             action='store_true',
-            help='Increase verbosity'
+            help='Increase verbosity',
+            default=top_level_defaults.verbose,
         )
         network_base_subparser = argparse.ArgumentParser(add_help=False)
         network_base_subparser.add_argument(
@@ -76,7 +109,7 @@ def main():
             type=int,
             help='Number of times to attempt to retry '
                  'downloading from Launchpad.',
-            default=3,
+            default=top_level_defaults.retries,
         )
         network_base_subparser.add_argument(
             '--retry-backoffs',
@@ -92,7 +125,7 @@ def main():
             metavar='[%s]' % '|'.join(known_protos),
             choices=known_protos,
             help='Specify protocol to use for fetch. Default: %(default)s',
-            default='https',
+            default=top_level_defaults.proto,
         )
 
         width, _ = shutil.get_terminal_size()
@@ -118,10 +151,13 @@ def main():
             '-P', '--parentfile',
             type=str,
             help=argparse.SUPPRESS,
-            default=pkg_resources.resource_filename(
-                'gitubuntu',
-                'parent_overrides.txt',
-            ),
+            default=top_level_defaults.parentfile,
+        )
+        parser.add_argument(
+            '-L', '--pullfile',
+            type=str,
+            help=argparse.SUPPRESS,
+            default=top_level_defaults.pullfile,
         )
 
         argcomplete.autocomplete(parser)
diff --git a/gitubuntu/clone.py b/gitubuntu/clone.py
index e2dcedf..91f63b7 100644
--- a/gitubuntu/clone.py
+++ b/gitubuntu/clone.py
@@ -5,6 +5,7 @@ import re
 import shutil
 from subprocess import CalledProcessError
 import sys
+from gitubuntu.__main__ import top_level_defaults
 from gitubuntu.git_repository import (
     GitUbuntuRepository,
     GitUbuntuRepositoryFetchError,
@@ -30,11 +31,35 @@ def copy_hooks(src, dst):
             dst,
         )
 
-def main(directory, lp_user, proto, package):
+def main(package, directory=None, lp_user=None, proto=None):
+    """Entry point to clone subcommand
+
+    @package: Name of source package
+    @directory: directory to clone the repository into
+    @lp_user: user to authenticate to Launchpad as
+    @proto: protocol to use (one of 'http', 'https', 'git')
+
+    If directory is None, a relative directory with the same name as
+    package will be used.
+
+    If lp_user is None, value of `git config gitubuntu.lpuser` will be
+    used.
+
+    If proto is None, the default value from gitubuntu.__main__ will be
+    used (top_level_defaults.proto).
+    """
+    directory = (
+        os.path.abspath(directory)
+        if directory
+        else os.path.join(os.path.abspath(os.getcwd()), package)
+    )
     if os.path.isdir(directory):
         logging.error('directory %s exists' % directory)
         sys.exit(1)
 
+    if proto is None:
+        proto = top_level_defaults.proto
+
     local_repo = GitUbuntuRepository(
         local_dir=directory,
         lp_user=lp_user,
@@ -87,6 +112,8 @@ def main(directory, lp_user, proto, package):
             'flags to git commands (e.g., git status --ignored).'
         )
 
+    return local_repo
+
 def parse_args(subparsers=None, base_subparsers=None):
     kwargs = dict(
         description='Clone a source package git repository to a directory',
@@ -128,13 +155,13 @@ def cli_main(args):
         lp_user = args.lp_user
     except AttributeError:
         lp_user = None
-    directory = (
-        os.path.abspath(args.directory)
-        if args.directory
-        else os.path.join(os.path.abspath(os.getcwd()), args.package)
-    )
 
-    main(directory, lp_user, args.proto, args.package)
+    main(
+        package=args.package,
+        directory=args.directory,
+        lp_user=lp_user,
+        proto=args.proto,
+    )
 
 
 # vi: ts=4 expandtab
diff --git a/gitubuntu/lint.py b/gitubuntu/lint.py
index 109eb13..fc09aec 100644
--- a/gitubuntu/lint.py
+++ b/gitubuntu/lint.py
@@ -202,8 +202,9 @@ def parse_args(subparsers=None, base_subparsers=None):
     return "lint - %s" % kwargs["description"]
 
 def cli_main(args):
-    main(
-        args.directory,
+    repo = GitUbuntuRepository(args.directory)
+    do_lint(
+        repo,
         args.commitish,
         args.lint_namespace,
         args.target_branch,
@@ -702,15 +703,28 @@ def derive_target_branch(repo, commitish_string, namespace='pkg'):
 # git checkout <coworker>/<branch>
 # git ubuntu lint
 # assume repository already exists
-def main(
-    directory,
+def do_lint(
+    repo,
     commitish,
-    lint_namespace,
-    target_branch,
-    verbose,
+    target_branch=None,
+    lint_namespace=None,
+    verbose=False,
 ):
-    repo = GitUbuntuRepository(directory)
+    """Entry point for lint subcommand
+
+    @repo: GitUbuntuRepository containing @commitish to lint
+    @commitish: commitish to lint
+    @target_branch: branch to target commitish's changes to. Typically a
+    remote branch in pkg/.
+    @lint_namespace: string namespace (without trailing /) in which to
+    find tags for merges.
+    @verbose: if True, also emit success statements
 
+    If target_branch is None, it will be derived from the nearest remote
+    branch.
+
+    If lint_namespace is None, it will be derived from @commitish.
+    """
     global _verbose 
     _verbose = verbose
 
diff --git a/gitubuntu/remote.py b/gitubuntu/remote.py
index 7318b44..f31dc8a 100644
--- a/gitubuntu/remote.py
+++ b/gitubuntu/remote.py
@@ -61,29 +61,39 @@ def cli_main(args):
         directory = os.path.abspath(args.directory)
     else:
         directory = os.getcwd()
-    if args.remote_name is not None:
-        remote_name = args.remote_name
-    else:
-        remote_name = args.user
 
     main(
         args.subsubcommand,
-        args.package,
         args.user,
+        args.package,
         args.url,
         args.lp_user,
         directory,
-        remote_name,
+        args.remote_name,
         args.no_fetch,
         args.proto,
     )
 
-def do_add(repo, package, user, url, remote_name, no_fetch):
+def do_add(repo, package, user, url=None, remote_name=None, no_fetch=False):
+    """add a remote to a repository
+
+    @repo: GitUbuntuRepository to modify
+    @package: source package name (XXX: is this derive-able from @repo?)
+    @user: remote user's repository to add as remote
+    @url: URL to add as a remote
+    @remote_name: specify the name of the new remote
+    @no_fetch: if True, do not fetch the remote after adding it
+
+    If url is None, the remote URL will be derived from @package and
+    @user.
+
+    If @remote_name is not specified, it will be set to @user.
+    """
     if url:
         repo.git_run(['remote', 'add', remote_name, url])
     else:
         repo.add_remote(
-            package,
+            pkgname=package,
             repo_owner=user,
             remote_name=remote_name,
         )
@@ -102,15 +112,40 @@ def do_add(repo, package, user, url, remote_name, no_fetch):
 
 def main(
     subcommand,
-    package,
     user,
-    url,
-    lp_user,
-    directory,
-    remote_name,
-    no_fetch,
-    proto,
+    package=None,
+    url=None,
+    lp_user=None,
+    directory=None,
+    remote_name=None,
+    no_fetch=False,
+    proto=None,
 ):
+    """Entry point to remote subcommand
+    @subcommand: what action to take. Currently only 'add' supported.
+    @user: which Launchpad user's repository to add
+    @package: name of source package
+    @url: the URL of the remote to add
+    @lp_user: user to authenticate to Launchpad as 
+    @proto: protocol to use (one of 'http', 'https', 'git')
+
+    If package is None, it will be derived from HEAD's debian/changelog.
+
+    If url is None, it will be derived from @user and @package.
+
+    If lp_user is None, value of `git config gitubuntu.lpuser` will be
+    used.
+
+    If directory is None, the current directory is used.
+
+    If remote_name is None, the remote will be named @user.
+
+    If proto is None, the default value from gitubuntu.__main__ will be
+    used (top_level_defaults.proto).
+    """
+    if directory is None:
+        directory = os.path.abspath(os.getcwd())
+
     repo = GitUbuntuRepository(
         local_dir=directory,
         lp_user=lp_user,
diff --git a/gitubuntu/review.py b/gitubuntu/review.py
new file mode 100644
index 0000000..3cf9f22
--- /dev/null
+++ b/gitubuntu/review.py
@@ -0,0 +1,103 @@
+import argparse
+from contextlib import redirect_stdout
+import io
+import os
+import shutil
+import tempfile
+import urllib.parse
+import gitubuntu.clone
+import gitubuntu.lint
+from gitubuntu.source_information import launchpad_login_auth
+
+
+def parse_args(subparsers=None, base_subparsers=None):
+    kwargs = dict(
+        description="Given a Launchpad MP URL, perform a review",
+        formatter_class=argparse.RawTextHelpFormatter,
+        epilog="An exit code of 0 indicates all checks passed",
+    )
+    if base_subparsers:
+        kwargs["parents"] = base_subparsers
+    if subparsers:
+        parser = subparsers.add_parser("review", **kwargs)
+        parser.set_defaults(func=cli_main)
+    else:
+        parser = argparse.ArgumentParser(**kwargs)
+
+    parser.add_argument('subsubcommand',
+                        help='start - Begin a Launchpad MP review',
+                        metavar='start',
+                        choices=['start'],
+    )
+    parser.add_argument("url", type=str,
+        help="Full URL of Launchpad Merge Proposal to review"
+    )
+    parser.add_argument('--add-comment', action='store_true',
+        help="Add a comment to the MP with the lint results. "
+             "This may also change the review state",
+    )
+
+    if not subparsers:
+        return parser.parse_args()
+    return "review - %s" % kwargs["description"]
+
+
+def cli_main(args):
+    main(
+        args.subsubcommand,
+        args.url,
+        args.proto,
+        args.add_comment,
+    )
+
+def main(subcommand, url, proto, add_comment):
+    url = urllib.parse.urlparse(url).path
+    lp = launchpad_login_auth()
+    mp = lp.load(url)
+    target_url = mp.target_git_repository.git_https_url
+    target_branch = mp.target_git_path[len('refs/heads/'):]
+    source_url = mp.source_git_repository.git_https_url
+    source_branch = mp.source_git_path[len('refs/heads/'):]
+    srcpkg = target_url.split('/')[-1]
+    if subcommand == 'start':
+        # inherently racy
+        repo = gitubuntu.clone.main(package=srcpkg)
+        print('Using git repository at %s' % repo.local_dir)
+        os.chdir(repo.local_dir)
+        idx = source_url.index('~')
+        colleague = source_url[idx+1:source_url.index('/', idx)]
+        gitubuntu.remote.do_add(
+            repo=repo,
+            package=srcpkg,
+            user=colleague,
+        )
+
+        print(
+            "Linting merge of %s/%s into pkg/%s" % (
+                colleague,
+                source_branch,
+                target_branch
+            )
+        )
+        try:
+            f = io.StringIO()
+            with redirect_stdout(f):
+                gitubuntu.lint.do_lint(
+                    repo=repo,
+                    commitish='%s/%s' % (colleague, source_branch),
+                    target_branch='pkg/%s' % target_branch,
+                    verbose=True,
+                )
+        except SystemExit as e:
+            if e.code == 0:
+                vote='Approve'
+            else:
+                vote='Needs Fixing'
+            lint_out = f.getvalue()
+            if add_comment:
+                mp.createComment(content=lint_out, vote=vote,
+                    subject='Automated lint result'
+                )
+            else:
+                print('git ubuntu lint result:')
+                print(lint_out, end='')