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)

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

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

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

PASSED: Continuous integration, rev:d672a163d7eb95e7385049daa1da11bdd777205a
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/56/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    SUCCESS: Integration Tests
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

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

PASSED: Continuous integration, rev:1b6612100ceab79c8fefded7075d3beb263f6bb1
https://jenkins.ubuntu.com/server/job/git-ubuntu-ci/60/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Style Check
    SUCCESS: Unit Tests
    SUCCESS: Integration Tests
    IN_PROGRESS: Declarative: Post Actions

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

review: Approve (continuous-integration)

[Andreas Hasenack (ahasenack)]

A few comments inline, just one needs-fixing really. The other one about making it work with an existing local repository is future work I think, for another branch.

There is no outstanding merge MP, so I tried running this review command against an open MP that is not a debian merge (cpaelzer's libvirt one) and also against an already-merged MP of mine (samba take 4), and got crashes in both cases. I understand this is somewhat work-in-progress, being the first new command to take advantage of the latest refactoring that is about to land. But I wonder: what do you think about having a way to hide these wip commands from --help?

review: Needs Fixing

68ddcab... by Nish Aravamudan on 2017-09-11

git_repository: use top-level-default for fetch_proto if not specified

This should not result in any functional change, but makes eventual
git-ubuntu review easier to implement.

b32d24b... by Nish Aravamudan on 2017-09-12

git_repository::fetch*: set GIT_TERMINAL_PROMPT=0 in env

When using http/https, LP will prompt for auth if a repository
is not found, because LP cannot distinguish between a private
repository (permission issue) and a non-existent repository until
the user is authenticated. This causes git itself to prompt
for authentication on the command-line, which we do not want. Since
we are only using http/https for fetch, disable the Git terminal
prompt via an environment variable. This does make clone and
review noisier, but that can be resolved in a future commit.

LP: #1716745

3f2aa06... by Nish Aravamudan on 2017-09-12

versioning: add support for non-series information in the version string

libvirt uploads (SRUs) have versions like 1.2.2-0ubuntu13.1.23 which break
our parsing, which assumes anything between ubuntu_maj and ubuntu_min
is either a series or invalid. Extend our parsing to allow for an "other"
field, which is set when the substring between maj and min cannot be
parsed as a version.

LP: #1716737

Unmerged commits

78e9c71... 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

3f2aa06... by Nish Aravamudan on 2017-09-12

versioning: add support for non-series information in the version string

libvirt uploads (SRUs) have versions like 1.2.2-0ubuntu13.1.23 which break
our parsing, which assumes anything between ubuntu_maj and ubuntu_min
is either a series or invalid. Extend our parsing to allow for an "other"
field, which is set when the substring between maj and min cannot be
parsed as a version.

LP: #1716737

b32d24b... by Nish Aravamudan on 2017-09-12

git_repository::fetch*: set GIT_TERMINAL_PROMPT=0 in env

When using http/https, LP will prompt for auth if a repository
is not found, because LP cannot distinguish between a private
repository (permission issue) and a non-existent repository until
the user is authenticated. This causes git itself to prompt
for authentication on the command-line, which we do not want. Since
we are only using http/https for fetch, disable the Git terminal
prompt via an environment variable. This does make clone and
review noisier, but that can be resolved in a future commit.

LP: #1716745

68ddcab... by Nish Aravamudan on 2017-09-11

git_repository: use top-level-default for fetch_proto if not specified

This should not result in any functional change, but makes eventual
git-ubuntu review easier to implement.

diff --git a/gitubuntu/__main__.py b/gitubuntu/__main__.py
index edeee43..27fbe14 100644
--- a/gitubuntu/__main__.py
+++ b/gitubuntu/__main__.py
@@ -66,6 +66,7 @@ def main():
             'remote': 'gitubuntu.remote',
             'submit': 'gitubuntu.submit',
             'lint': 'gitubuntu.lint',
+            'review': 'gitubuntu.review',
         }
 
         known_network_subcommands = {
@@ -78,6 +79,7 @@ def main():
             'queue',
             'remote',
             'submit',
+            'review',
         }
 
         parser = argparse.ArgumentParser(
diff --git a/gitubuntu/clone.py b/gitubuntu/clone.py
index 832945e..c4d8dbd 100644
--- a/gitubuntu/clone.py
+++ b/gitubuntu/clone.py
@@ -31,26 +31,18 @@ def copy_hooks(src, dst):
             dst,
         )
 
-<<<<<<< gitubuntu/clone.py
 def main(
     package,
     directory=None,
     lp_user=None,
     proto=top_level_defaults.proto,
 ):
-=======
-def main(package, directory=None, lp_user=None, proto=None):
->>>>>>> gitubuntu/clone.py
     """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
-<<<<<<< gitubuntu/clone.py
     @proto: string protocol to use (one of 'http', 'https', 'git')
-=======
-    @proto: protocol to use (one of 'http', 'https', 'git')
->>>>>>> gitubuntu/clone.py
 
     If directory is None, a relative directory with the same name as
     package will be used.
@@ -58,12 +50,6 @@ def main(package, directory=None, lp_user=None, proto=None):
     If lp_user is None, value of `git config gitubuntu.lpuser` will be
     used.
 
-<<<<<<< gitubuntu/clone.py
-=======
-    If proto is None, the default value from gitubuntu.__main__ will be
-    used (top_level_defaults.proto).
-
->>>>>>> gitubuntu/clone.py
     Returns the resulting GitUbuntuRepository object
     """
     directory = (
@@ -75,12 +61,6 @@ def main(package, directory=None, lp_user=None, proto=None):
         logging.error('directory %s exists' % directory)
         sys.exit(1)
 
-<<<<<<< gitubuntu/clone.py
-=======
-    if proto is None:
-        proto = top_level_defaults.proto
-
->>>>>>> gitubuntu/clone.py
     local_repo = GitUbuntuRepository(
         local_dir=directory,
         lp_user=lp_user,
diff --git a/gitubuntu/git_repository.py b/gitubuntu/git_repository.py
index 9fdf7bb..e97a0ea 100644
--- a/gitubuntu/git_repository.py
+++ b/gitubuntu/git_repository.py
@@ -16,6 +16,7 @@ from subprocess import CalledProcessError
 import sys
 import tempfile
 import time
+from gitubuntu.__main__ import top_level_defaults
 from gitubuntu.dsc import component_tarball_matches
 from gitubuntu.run import run, runq, decode_binary
 try:
@@ -426,9 +427,14 @@ class GitUbuntuRepository:
 
     To access the underlying pygit2.Repository object, use the raw_repo
     property.
+
     """
 
     def __init__(self, local_dir, lp_user=None, fetch_proto=None):
+        """
+        If fetch_proto is None, the default value from
+        gitubuntu.__main__ will be used (top_level_defaults.proto).
+        """
         if local_dir is None:
             self._local_dir = tempfile.mkdtemp()
         else:
@@ -465,6 +471,9 @@ class GitUbuntuRepository:
                 logging.error("Unable to determine Launchpad user")
                 sys.exit(1)
 
+        if fetch_proto is None:
+            fetch_proto = top_level_defaults.proto
+
         self._fetch_proto = fetch_proto
 
     def create_orphan_branch(self, branch_name, msg):
@@ -764,51 +773,68 @@ class GitUbuntuRepository:
         # self.git_run(['config', 'url.%s.insteadof' % self.lp_user, 'lpme'])
 
     def fetch_remote(self, remote_name, verbose=False):
+        # Does not seem to be working with https
+        # https://github.com/libgit2/pygit2/issues/573
+        # https://github.com/libgit2/libgit2/issues/3786
+        # self.raw_repo.remotes[remote_name].fetch()
+        kwargs = {}
+        kwargs['verbose_on_failure'] = True
+        if verbose:
+            # If we are redirecting stdout/stderr to the console, we
+            # do not need to have run() also emit it
+            kwargs['verbose_on_failure'] = False
+            kwargs['stdout'] = None
+            kwargs['stderr'] = None
         try:
-            # Does not seem to be working with https
-            # https://github.com/libgit2/pygit2/issues/573
-            # https://github.com/libgit2/libgit2/issues/3786
-            # self.raw_repo.remotes[remote_name].fetch()
-            logging.debug('Fetching remote %s' % remote_name)
-            kwargs = {}
-            kwargs['verbose_on_failure'] = True
-            if verbose:
-                # If we are redirecting stdout/stderr to the console, we
-                # do not need to have run() also emit it
-                kwargs['verbose_on_failure'] = False
-                kwargs['stdout'] = None
-                kwargs['stderr'] = None
-            self.git_run(['fetch', remote_name], **kwargs)
+            logging.debug("Fetching remote %s", remote_name)
+            cp = self.git_run(
+                args=['fetch', remote_name],
+                env={'GIT_TERMINAL_PROMPT': '0',},
+                **kwargs
+            )
         except CalledProcessError:
             raise GitUbuntuRepositoryFetchError(
-                'Unable to fetch remote %s' % remote_name
+                "Unable to fetch remote %s" % remote_name
             )
 
     def fetch_base_remotes(self, verbose=False):
         self.fetch_remote(remote_name='pkg', verbose=verbose)
 
     def fetch_remote_refspecs(self, remote_name, refspecs, verbose=False):
-        try:
-            # Does not seem to be working with https
-            # https://github.com/libgit2/pygit2/issues/573
-            # https://github.com/libgit2/libgit2/issues/3786
-            # self.raw_repo.remotes[remote_name].fetch()
-            for refspec in refspecs:
-                logging.debug('Fetching refspec %s from remote %s' %
-                             (refspec, remote_name))
-                kwargs = {}
-                kwargs['verbose_on_failure'] = True
-                if verbose:
-                    # If we are redirecting stdout/stderr to the console, we
-                    # do not need to have run() also emit it
-                    kwargs['verbose_on_failure'] = False
-                    kwargs['stdout'] = None
-                    kwargs['stderr'] = None
-                self.git_run(['fetch', remote_name, refspec], **kwargs)
-        except CalledProcessError:
-            raise GitUbuntuRepositoryFetchError(
-                'Unable to fetch %s from remote %s' % (refspecs, remote_name)
-            )
+        # Does not seem to be working with https
+        # https://github.com/libgit2/pygit2/issues/573
+        # https://github.com/libgit2/libgit2/issues/3786
+        # self.raw_repo.remotes[remote_name].fetch()
+        for refspec in refspecs:
+            kwargs = {}
+            kwargs['verbose_on_failure'] = True
+            if verbose:
+                # If we are redirecting stdout/stderr to the console, we
+                # do not need to have run() also emit it
+                kwargs['verbose_on_failure'] = False
+                kwargs['stdout'] = None
+                kwargs['stderr'] = None
+            kwargs['env'] = {
+                'GIT_TERMINAL_PROMPT': '0',
+            }
+            try:
+                logging.debug(
+                    "Fetching refspec %s from remote %s",
+                    refspec,
+                    remote_name,
+                )
+                self.git_run(
+                    args=['fetch', remote_name, refspec],
+                    env={'GIT_TERMINAL_PROMPT': '0',},
+                    **kwargs,
+                )
+            except CalledProcessError:
+                raise GitUbuntuRepositoryFetchError(
+                    "Unable to fetch %s from remote %s" % (
+                        refspecs,
+                        remote_name,
+                    )
+                )
 
     def fetch_lpuser_remote(self, verbose=False):
         if not self._fetch_proto:
diff --git a/gitubuntu/importer.py b/gitubuntu/importer.py
index 201f31e..6311047 100644
--- a/gitubuntu/importer.py
+++ b/gitubuntu/importer.py
@@ -115,7 +115,6 @@ def dsc_to_tree_hash(repo, dsc_path):
 
 def cleanup(no_clean, local_dir):
     '''Remove a local directory, conditionally
-<<<<<<< gitubuntu/importer.py
 
     no_clean: if True, do not remove @local_dir and print a message instead
     local_dir: directory to remove
@@ -188,31 +187,6 @@ def main(
 
     logging.info('Ubuntu Server Team importer v%s' % VERSION)
 
-=======
-
-    no_clean: if True, do not remove @local_dir and print a message instead
-    local_dir: directory to remove
-    '''
-    if not no_clean:
-        shutil.rmtree(local_dir)
-    else:
-        logging.info('Leaving %s as directed' % local_dir)
-
-
-# XXX: need a namedtuple to hold common arguments
-def main(pkgname, owner, no_clean, directory, user, proto, no_fetch,
-    no_push, dl_cache, fixup_devel, active_series_only, skip_orig,
-    pullfile, parentfile, retries, retry_backoffs, skip_applied,
-    reimport,
-):
-    if owner == 'usd-import-team':
-        namespace = 'importer'
-    else:
-        namespace = owner
-
-    logging.info('Ubuntu Server Team importer v%s' % VERSION)
-
->>>>>>> gitubuntu/importer.py
     repo = GitUbuntuRepository(directory, user, proto)
     if not directory:
         logging.info('Using git repository at %s', repo.local_dir)
@@ -1210,45 +1184,6 @@ def import_unapplied_spi(repo, spi, namespace, skip_orig, ubuntu_sinfo):
                             )
             except AttributeError:
                 pass
-<<<<<<< gitubuntu/importer.py
-
-        if version_compare(str(spi.version), unapplied_tip_version) <= 0:
-            logging.warn('Version to import (%s) is not after %s tip (%s)',
-                         spi.version, pretty_head_name, unapplied_tip_version
-                        )
-
-        # Walk changelog backwards until we find an imported version
-        for version in import_tree_versions:
-            unapplied_changelog_parent_tag = repo.get_import_tag(version, namespace)
-            if unapplied_changelog_parent_tag is not None:
-                # sanity check that version from d/changelog of the
-                # tagged parent matches ours
-                parent_changelog_version, _ = \
-                    repo.get_changelog_versions_from_treeish(
-                        str(unapplied_changelog_parent_tag.peel(pygit2.Tree).id),
-                                                            )
-                # if the parent was imported via a parent override
-                # itself, assume it is valid without checking its
-                # tree's debian/changelog, as it wasn't used
-                if (version not in _PARENT_OVERRIDES and
-                    parent_changelog_version != version
-                   ):
-                    logging.error('Found a tag corresponding to '
-                                  'parent version %s, but d/changelog '
-                                  'version (%s) differs. Will '
-                                  'orphan commit.' % (
-                                  version,
-                                  parent_changelog_version
-                                  )
-                                 )
-                else:
-                    unapplied_changelog_parent_commit = str(unapplied_changelog_parent_tag.peel().id)
-                    logging.debug('Changelog parent (tag) is %s',
-                        repo.tag_to_pretty_name(unapplied_changelog_parent_tag)
-                                )
-                break
-
-=======
 
         if version_compare(str(spi.version), unapplied_tip_version) <= 0:
             logging.warn('Version to import (%s) is not after %s tip (%s)',
@@ -1286,7 +1221,6 @@ def import_unapplied_spi(repo, spi, namespace, skip_orig, ubuntu_sinfo):
                                 )
                 break
 
->>>>>>> gitubuntu/importer.py
     # If the two parents are tree-identical, then favor publication
     # history. This deals with copy-forwards between series and with
     # syncs.
@@ -1668,7 +1602,6 @@ def cli_main(args):
         dl_cache = None
 
     main(
-<<<<<<< gitubuntu/importer.py
         pkgname=args.package,
         owner=args.lp_owner,
         no_clean=no_clean,
@@ -1687,24 +1620,4 @@ def cli_main(args):
         parentfile=args.parentfile,
         retries=args.retries,
         retry_backoffs=args.retry_backoffs,
-=======
-        args.package,
-        args.lp_owner,
-        no_clean,
-        directory,
-        user,
-        args.proto,
-        args.no_fetch,
-        no_push,
-        dl_cache,
-        args.fixup_devel,
-        args.active_series_only,
-        args.skip_orig,
-        args.pullfile,
-        args.parentfile,
-        args.retries,
-        args.retry_backoffs,
-        args.skip_applied,
-        args.reimport,
->>>>>>> gitubuntu/importer.py
     )
diff --git a/gitubuntu/merge.py b/gitubuntu/merge.py
index b6098b7..cc16cc0 100644
--- a/gitubuntu/merge.py
+++ b/gitubuntu/merge.py
@@ -98,7 +98,6 @@ def parse_args(subparsers=None, base_subparsers=None):
 
 def cli_main(args):
     main(
-<<<<<<< gitubuntu/merge.py
         directory=args.directory,
         commitish=args.commitish,
         onto=args.onto,
@@ -107,17 +106,6 @@ def cli_main(args):
         bug=args.bug,
         release=args.release,
         subcommand=args.subsubcommand,
-=======
-        args.directory,
-        args.commitish,
-        args.onto,
-        args.force,
-        args.tag_only,
-        args.pullfile,
-        args.bug,
-        args.release,
-        args.subsubcommand,
->>>>>>> gitubuntu/merge.py
     )
 
 
@@ -327,7 +315,6 @@ def do_finish(repo, tag_prefix, commitish, merge_base_id, onto, release, bug, fo
             ancestor_check = True
         except subprocess.CalledProcessError as e:
             pass
-<<<<<<< gitubuntu/merge.py
 
     if not ancestor_check:
         if len(ancestors_checked) > 1:
@@ -474,142 +461,6 @@ def main(
         merge_base_id,
     )
 
-=======
-
-    if not ancestor_check:
-        if len(ancestors_checked) > 1:
-            msg = ' and '.join(ancestors_checked) + ' are not ancestors'
-        else:
-            msg = '%s is not an ancestor' % ancestors_checked.pop()
-        if force:
-            logging.error("%s of the HEAD commit, but --force passed.", msg)
-        else:
-            logging.error(
-                "%s of the HEAD commit. Did you run `git-ubuntu merge "
-                "start` first? (Pass -f to force the merge).",
-                msg
-            )
-            sys.exit(1)
-
-    # 1) git merge-changelogs old/ubuntu old/debian new/debian
-    do_merge_changelogs(repo, commitish, merge_base_id, onto)
-    # 2) git reconstruct-changelog <onto>
-    do_reconstruct_changelog(repo, onto, release, bug)
-    # TODO elide from each entry any lines outside of changelog-markers
-    # (add flag to specify it?)
-    # 3) update-maintainer
-    do_update_maintainer(repo)
-
-
-def main(
-    directory,
-    commitish,
-    onto,
-    force,
-    tag_only,
-    pullfile,
-    bug,
-    release,
-    subcommand,
-):
-    repo = GitUbuntuRepository(directory)
-    tag_prefix = ''
-    if bug:
-        tag_prefix = 'lp%s/' % bug
-
-    try:
-        commitish_obj = repo.get_commitish(commitish)
-    except KeyError:
-        logging.error(
-            "%s is not a defined object in this git repository.",
-            commitish
-        )
-        sys.exit(1)
-
-    commitish_version, _ = repo.get_changelog_versions_from_treeish(
-        str(commitish_obj.id),
-    )
-
-    try:
-        onto_obj = repo.get_commitish(onto)
-        onto_version, _ = repo.get_changelog_versions_from_treeish(onto)
-        if version_compare(commitish_version, onto_version) >= 0:
-            if force:
-                logging.info(
-                    "%s version (%s) is after %s version (%s), "
-                    "but --force passed.",
-                    commitish,
-                    commitish_version,
-                    onto,
-                    onto_version,
-                )
-            else:
-                logging.error(
-                    "%s version (%s) is after %s version (%s). "
-                    "Are you sure you want to merge? "
-                    "(Pass -f to force the merge).",
-                    commitish,
-                    commitish_version,
-                    onto,
-                    onto_version,
-                )
-                sys.exit(1)
-    except KeyError:
-        logging.info("%s is not a defined object in this git repository.", onto)
-        logging.info(
-            "Creating a local branch named %s tracking pkg/%s.",
-            onto,
-            onto,
-        )
-        try:
-            branch = repo.create_tracking_branch(
-                onto,
-                'pkg/%s' % onto,
-                force=force,
-            )
-        except:
-            logging.error(
-                "Failed to create local branch (%s). "
-                "Does it already exist (pass -f)?",
-                onto,
-            )
-            sys.exit(1)
-        onto_obj = repo.get_commitish(onto)
-
-    cp = run(['git', 'status', '--porcelain'])
-    if len(cp.stdout) > 0:
-        logging.error('Working tree must be clean to continue:')
-        logging.error(decode_binary(cp.stdout))
-        sys.exit(1)
-
-    merge_base_id = repo.raw_repo.merge_base(
-        onto_obj.id,
-        commitish_obj.id,
-    )
-    if merge_base_id is None:
-        logging.error(
-            "Unable to find a common ancestor for %s and %s.",
-            onto,
-            commitish,
-        )
-        sys.exit(1)
-
-    if merge_base_id == onto_obj.id:
-        logging.error(
-            "The common ancestor of %s and %s is "
-            "identical to %s. No merge is necessary.",
-            onto,
-            commitish,
-            onto,
-        )
-        sys.exit(1)
-
-    merge_base_id = str(merge_base_id)
-    merge_base_version, _ = repo.get_changelog_versions_from_treeish(
-        merge_base_id,
-    )
-
->>>>>>> gitubuntu/merge.py
     if subcommand == 'start':
         do_start(
             repo,
diff --git a/gitubuntu/remote.py b/gitubuntu/remote.py
index 41981ee..db42168 100644
--- a/gitubuntu/remote.py
+++ b/gitubuntu/remote.py
@@ -120,7 +120,6 @@ def main(
     directory=None,
     remote_name=None,
     no_fetch=False,
-<<<<<<< gitubuntu/remote.py
     proto=top_level_defaults.proto,
 ):
     """Entry point to remote subcommand
@@ -130,17 +129,6 @@ def main(
     @url: the string URL of the remote to add
     @lp_user: string user to authenticate to Launchpad as
     @proto: string protocol to use (one of 'http', 'https', 'git')
-=======
-    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')
->>>>>>> gitubuntu/remote.py
 
     If package is None, it will be derived from HEAD's debian/changelog.
 
@@ -152,12 +140,6 @@ def main(
     If directory is None, the current directory is used.
 
     If remote_name is None, the remote will be named @user.
-<<<<<<< gitubuntu/remote.py
-=======
-
-    If proto is None, the default value from gitubuntu.__main__ will be
-    used (top_level_defaults.proto).
->>>>>>> gitubuntu/remote.py
     """
     if directory is None:
         directory = os.path.abspath(os.getcwd())
diff --git a/gitubuntu/review.py b/gitubuntu/review.py
new file mode 100644
index 0000000..0acdbd3
--- /dev/null
+++ b/gitubuntu/review.py
@@ -0,0 +1,140 @@
+import argparse
+from contextlib import redirect_stdout
+import io
+import logging
+import os
+import shutil
+import sys
+import tempfile
+import urllib.parse
+import gitubuntu.clone
+import gitubuntu.lint
+from gitubuntu.__main__ import top_level_defaults
+from gitubuntu.git_repository import (
+    GitUbuntuRepository,
+    GitUbuntuRepositoryFetchError,
+)
+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 (EXPERIMENTAL)",
+        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(
+        "url",
+        type=str,
+        help="Full URL of Launchpad Merge Proposal to review",
+    )
+    parser.add_argument(
+        '--clone',
+        action='store_true',
+        help="Create a new local repository by cloning, rather than "
+             "using the repository at the current directory.",
+    )
+    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.clone,
+        args.url,
+        args.add_comment,
+    )
+
+def main(clone, url, add_comment):
+    """Entry point to review
+
+    Arguments:
+    @clone: boolean to indicate a new repository should be cloned.
+    @url: string URL of MP to review
+    @add_comment: boolean to indicate the MP should be updated with the
+    review results.
+    """
+    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/'):]
+    idx = target_url.index('~')
+    target_user = target_url[idx+1:target_url.index('/', idx)]
+    if target_user != 'usd-import-team':
+        logging.error(
+            "Performing arbitrary reviews is not yet supported (user %s "
+            "specified).",
+            target_user
+        )
+        sys.exit(1)
+
+    source_url = mp.source_git_repository.git_https_url
+    source_branch = mp.source_git_path[len('refs/heads/'):]
+    idx = source_url.index('~')
+    source_user = source_url[idx+1:source_url.index('/', idx)]
+    srcpkg = target_url.split('/')[-1]
+    if clone:
+        repo = gitubuntu.clone.main(package=srcpkg)
+    else:
+        repo = GitUbuntuRepository('.')
+        try:
+            repo.fetch_base_remotes()
+        except GitUbuntuRepositoryFetchError:
+            sys.exit(1)
+    os.chdir(repo.local_dir)
+    repo.add_remote(
+        pkgname=srcpkg,
+        repo_owner=source_user,
+        remote_name=source_user,
+    )
+    try:
+        repo.fetch_remote(remote_name=source_user)
+    except GitUbuntuRepositoryFetchError:
+        sys.exit(1)
+
+    logging.info(
+        "Linting merge of %s/%s into pkg/%s" % (
+            source_user,
+            source_branch,
+            target_branch
+        )
+    )
+    try:
+        f = io.StringIO()
+        with redirect_stdout(f):
+            gitubuntu.lint.do_lint(
+                repo=repo,
+                commitish='%s/%s' % (source_user, 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:
+            logging.info('git ubuntu lint result:')
+            print(lint_out, end='')
diff --git a/gitubuntu/submit.py b/gitubuntu/submit.py
index 0bf1319..fb080f3 100644
--- a/gitubuntu/submit.py
+++ b/gitubuntu/submit.py
@@ -75,7 +75,6 @@ def cli_main(args):
         user = None
 
     main(
-<<<<<<< gitubuntu/submit.py
         directory=args.directory,
         force=args.force,
         target_user=args.target_user,
@@ -85,23 +84,11 @@ def cli_main(args):
         user=user,
         branch=args.branch,
         proto=args.proto,
-=======
-        args.directory,
-        args.force,
-        user,
-        args.branch,
-        args.target_user,
-        args.target_branch,
-        args.no_push,
-        args.proto,
-        args.reviewer,
->>>>>>> gitubuntu/submit.py
     )
 
 def main(
     directory,
     force,
-<<<<<<< gitubuntu/submit.py
     target_user,
     target_branch,
     no_push,
@@ -130,16 +117,6 @@ def main(
 
     If branch is None, HEAD is used.
     """
-=======
-    user,
-    branch,
-    target_user,
-    target_branch,
-    no_push,
-    proto,
-    reviewers
-):
->>>>>>> gitubuntu/submit.py
     repo = GitUbuntuRepository(directory, user, proto)
 
     if branch:
diff --git a/gitubuntu/versioning.py b/gitubuntu/versioning.py
index c3187f5..3a5dc53 100644
--- a/gitubuntu/versioning.py
+++ b/gitubuntu/versioning.py
@@ -27,7 +27,7 @@ __all__ = [
 # see _decompose_version_string for explanation of members
 _VersionComps = namedtuple(
     "_VersionComps",
-    ["prefix_parts", "ubuntu_maj", "ubuntu_series", "ubuntu_min"]
+    ["prefix_parts", "ubuntu_maj", "ubuntu_series", "ubuntu_other", "ubuntu_min"]
 )
 
 def _decompose_version_string(version_string):
@@ -52,10 +52,19 @@ def _decompose_version_string(version_string):
     "1.0.2ubuntu3.17.04.4", 17.04 is the Ubuntu series string. In both
     "1.0-2ubuntu3.4" and "1.0-2", the Ubuntu series is not present.
 
+    ubuntu_other is any non-series information as a string between the
+    major and minor version numbers. For example, in the version strings
+    "1.0-2ubuntu3.4" and "1.0-2ubuntu3.17.04.4", the other Ubuntu
+    version string is not present. In both "1.0-2" and "1.0-2ubuntu3",
+    the other Ubuntu version number is not present. In
+    "1.2.2-0ubuntu13.1.23", "1" is the Ubuntu other version string.
+
     ubuntu_min is the "minor" Ubuntu version number as an integer, or None if
     not present. For example, in the version strings "1.0-2ubuntu3.4" and
     "1.0-2ubuntu3.17.04.4", 4 is the Ubuntu minor version number. In both
-    "1.0-2" and "1.0-2ubuntu3", the minor Ubuntu version number is not present.
+    "1.0-2" and "1.0-2ubuntu3", the minor Ubuntu version number is not
+    present. In "1.2.2-0ubuntu13.1.23", 23 is the Ubuntu minor version
+    number.
 
     For version strings not following these standard forms, the result is
     undefined except where specific test cases exist for them.
@@ -73,7 +82,7 @@ def _decompose_version_string(version_string):
         )
     elif 'build' in parts:
         return _VersionComps._make(
-            [parts[:parts.index('build')], None, None, None]
+            [parts[:parts.index('build')], None, None, None, None]
         )
     elif 'ubuntu' in parts:
         ubuntu_idx = parts.index('ubuntu')
@@ -85,6 +94,7 @@ def _decompose_version_string(version_string):
             # nothing after "Xubuntu", treated as 1
             ubuntu_maj = 1
         ubuntu_series = None
+        ubuntu_other = None
         if len(suffix_parts) < 2:
             ubuntu_min = None
         else:
@@ -95,15 +105,13 @@ def _decompose_version_string(version_string):
             if len(suffix_parts[1:-1]) > 2:
                 ubuntu_series = ''.join(suffix_parts[2:-2])
                 if not re.match(r'\d\d\.\d\d', ubuntu_series):
-                    raise ValueError("Not sure how to decompose %s: "
-                        "series %s not in expected format" %
-                        (version_string, ubuntu_series)
-                    )
+                    ubuntu_series = None
+                    ubuntu_other = ''.join(suffix_parts[2:-2])
         return _VersionComps._make(
-            [static_parts, ubuntu_maj, ubuntu_series, ubuntu_min]
+            [static_parts, ubuntu_maj, ubuntu_series, ubuntu_other, ubuntu_min]
         )
     else:
-        return _VersionComps._make([parts, None, None, None])
+        return _VersionComps._make([parts, None, None, None, None])
 
 
 def _bump_version_object(old_version_object, bump_function):
@@ -169,6 +177,8 @@ def _bump_sru_version_suffix_string(befores, series_string, afters,
     bumped_parts = ['ubuntu', str(new_maj), '.']
     if old_suffix_comps.ubuntu_series:
         bumped_parts.extend([old_suffix_comps.ubuntu_series, '.'])
+    elif old_suffix_comps.ubuntu_other:
+        bumped_parts.extend([old_suffix_comps.ubuntu_other, '.'])
     elif str(version) in [str(v) for v in befores + afters]:
         bumped_parts.extend([series_string, '.'])
     elif any(
@@ -251,12 +261,13 @@ def version_compare(a, b):
 
 
 @pytest.mark.parametrize('test_input, expected', [
-    ('2', _VersionComps._make([['2'], None, None, None])),
-    ('2ubuntu1', _VersionComps._make([['2'], 1, None, None])),
-    ('2ubuntu1.3', _VersionComps._make([['2'], 1, None, 3])),
-    ('2ubuntu0.16.04.3', _VersionComps._make([['2'], 0, '16.04', 3])),
-    ('2build1', _VersionComps._make([['2'], None, None, None])),
-    ('2build1.3', _VersionComps._make([['2'], None, None, None])),
+    ('2', _VersionComps._make([['2'], None, None, None, None])),
+    ('2ubuntu1', _VersionComps._make([['2'], 1, None, None, None])),
+    ('2ubuntu1.3', _VersionComps._make([['2'], 1, None, None, 3])),
+    ('2ubuntu0.16.04.3', _VersionComps._make([['2'], 0, '16.04', None, 3])),
+    ('2build1', _VersionComps._make([['2'], None, None, None, None])),
+    ('2build1.3', _VersionComps._make([['2'], None, None, None, None])),
+    ('0ubuntu13.1.22', _VersionComps._make([['0'], 13, None, '1', 22])),
 ])
 def test_decompose_version_string(test_input, expected):
     assert _decompose_version_string(test_input) == expected