Merge "Fix broken interactive mode to be usable"

git_upstream/commands/import.py

@@ -114,7 +114,9 @@ class ImportCommand(LogDedentMixin, GitUpstreamCommand):
         """Perform additional parsing of args"""
 
         if self.args.finish and not self.args.upstream_branch:
-            self.args.upstream_branch = self.args.import_branch
+            self.args.real_upstream_branch = self.args.import_branch
+        else:
+            self.args.real_upstream_branch = self.args.upstream_branch
 
     def _finish(self, import_upstream):
         self.log.notice("Merging import to requested branch '%s'",
@@ -126,7 +128,7 @@ class ImportCommand(LogDedentMixin, GitUpstreamCommand):
                     target branch: '%s'
                     upstream branch: '%s'
                     import branch: '%s'""",
-                self.args.branch, self.args.upstream_branch,
+                self.args.branch, self.args.real_upstream_branch,
                 import_upstream.import_branch)
             if self.args.branches:
                 for branch in self.args.branches:
@@ -140,14 +142,14 @@ class ImportCommand(LogDedentMixin, GitUpstreamCommand):
 
         import_upstream = ImportUpstream(
             branch=self.args.branch,
-            upstream=self.args.upstream_branch,
+            upstream=self.args.real_upstream_branch,
             import_branch=self.args.import_branch,
             extra_branches=self.args.branches)
 
         self.log.notice("Searching for previous import")
         strategy = ImportStrategiesFactory.create_strategy(
             self.args.strategy, branch=self.args.branch,
-            upstream=self.args.upstream_branch,
+            upstream=self.args.real_upstream_branch,
             search_refs=self.args.search_refs)
 
         if not strategy.previous_upstream:
@@ -179,7 +181,19 @@ class ImportCommand(LogDedentMixin, GitUpstreamCommand):
         import_upstream.create_import(force=self.args.force)
         self.log.notice("Successfully created import branch")
 
-        if not import_upstream.apply(strategy, self.args.interactive):
+        # build suitable command line for interactive mode
+        if self.args.merge:
+            cmdline = self.args.script_cmdline + [
+                self.name,
+                '--finish',
+                '--into=%s' % import_upstream.branch,
+                '--import-branch=%s' % import_upstream.import_branch,
+                import_upstream.upstream
+            ] + import_upstream.extra_branches
+        else:
+            cmdline = None
+
+        if not import_upstream.apply(strategy, self.args.interactive, cmdline):
             self.log.notice("Import cancelled")
             return False
 

git_upstream/lib/importupstream.py

@@ -48,10 +48,6 @@ class ImportUpstream(LogDedentMixin, GitMixin):
         # any computation
         super(ImportUpstream, self).__init__(*args, **kwargs)
 
-        # test that we can use this git repo
-        if self.is_detached():
-            raise ImportUpstreamError("In 'detached HEAD' state")
-
         if self.repo.bare:
             raise ImportUpstreamError("Cannot perform imports in bare repos")
 
@@ -141,6 +137,10 @@ class ImportUpstream(LogDedentMixin, GitMixin):
         automatically if checkout is true.
         """
 
+        # test that we can use this git repo
+        if self.is_detached():
+            raise ImportUpstreamError("In 'detached HEAD' state")
+
         if not commit:
             commit = self.upstream
 
@@ -264,7 +264,7 @@ class ImportUpstream(LogDedentMixin, GitMixin):
             # set root commit for next loop
             root = sequence[counter].hexsha
 
-    def apply(self, strategy, interactive=False):
+    def apply(self, strategy, interactive=False, resume_cmdline=None):
         """Apply list of commits given onto latest import of upstream"""
 
         commit_list = list(strategy.filtered_iter())
@@ -317,7 +317,8 @@ class ImportUpstream(LogDedentMixin, GitMixin):
             # reset head back to the tip of the changes to be rebased
             self._set_branch(self.import_branch, self.branch, force=True)
 
-        rebase = RebaseEditor(interactive, repo=self.repo)
+        # build the command line
+        rebase = RebaseEditor(resume_cmdline, interactive, repo=self.repo)
         if len(commit_list):
             first = commit_list[0]
 
@@ -366,6 +367,13 @@ class ImportUpstream(LogDedentMixin, GitMixin):
         performing suitable verification checks.
         """
         self.log.info("No verification checks enabled")
+        in_rebase = False
+        if self.is_detached():
+            # called via rebase exec
+            target_sha = self.git.rev_parse("HEAD")
+            in_rebase = True
+        else:
+            target_sha = self.import_branch
         self.git.checkout(self.branch)
         current_sha = self.git.rev_parse("HEAD")
 
@@ -385,7 +393,7 @@ class ImportUpstream(LogDedentMixin, GitMixin):
                 Merging import branch to HEAD and ignoring changes:
                     git merge -s ours --no-commit %s
                 """, self.import_branch)
-            self.git.merge('-s', 'ours', self.import_branch, no_commit=True)
+            self.git.merge('-s', 'ours', target_sha, no_commit=True)
             self.log.info(
                 """
                 Replacing tree contents with those from the import branch:
@@ -404,7 +412,9 @@ class ImportUpstream(LogDedentMixin, GitMixin):
                     self.git.rev_parse("%s^{tree}" % self.import_branch):
                 raise ImportUpstreamError(
                     "Resulting tree does not match import")
-        except (GitCommandError, ImportUpstreamError):
+            if in_rebase:
+                self.git.checkout(target_sha)
+        except (GitCommandError, ImportUpstreamError) as e:
             self.log.error(
                 """
                 Failed to finish import by merging branch:
@@ -412,6 +422,7 @@ class ImportUpstream(LogDedentMixin, GitMixin):
                 into and replacing the contents of:
                     '%s'
                 """, self.import_branch, self.branch)
+            self.log.error(str(e))
             self._set_branch(self.branch, current_sha, force=True)
             return False
         except Exception:

git_upstream/lib/rebaseeditor.py

@@ -33,11 +33,11 @@ REBASE_EDITOR_TODO = "git-upstream/git-rebase-todo"
 
 class RebaseEditor(GitMixin, LogDedentMixin):
 
-    def __init__(self, interactive=False, *args, **kwargs):
+    def __init__(self, finish_args, interactive=False, *args, **kwargs):
 
         self._interactive = interactive
 
-        super(RebaseEditor, self).__init__()
+        super(RebaseEditor, self).__init__(*args, **kwargs)
 
         self._editor = REBASE_EDITOR_SCRIPT
         # interactive switch here determines if the script that is given
@@ -48,6 +48,8 @@ class RebaseEditor(GitMixin, LogDedentMixin):
             self.log.debug("Enabling interactive mode for rebase")
             self._editor = "%s --interactive" % self.editor
 
+        self.finish_args = finish_args
+
     @property
     def editor(self):
         return self._editor
@@ -97,6 +99,48 @@ class RebaseEditor(GitMixin, LogDedentMixin):
 
         return todo_file
 
+    def _insert_exec_to_todo(self):
+        if not self.finish_args:
+            # no need to insert, as asked not to perform a finish/merge
+            return
+
+        todo_file = os.path.join(self.repo.git_dir, REBASE_EDITOR_TODO)
+        exec_line = "exec %s\n" % " ".join(self.finish_args)
+
+        insn_data = None
+        with codecs.open(todo_file, "r", "utf-8") as todo:
+            insn_data = todo.readlines()
+
+        # Cannot just append to file, as rebase appears to cut off
+        # after the second blank line in a row is encountered.
+        # Need to find the last instruction and insert afterwards,
+        # or if we find noop replace.
+        last = 0
+        for idx, line in enumerate(insn_data):
+            # comment line - ignore
+            if line.startswith("#"):
+                continue
+            # found noop - just replace
+            if line.rstrip() == "noop":
+                insn_data[idx] = exec_line
+                break
+            # not an empty line
+            if line.rstrip() != "":
+                last = idx
+        else:
+            # didn't break so need to insert after last instruction
+            insn_data.insert(last + 1, exec_line)
+
+        # replace contents to include exec
+        try:
+            todo = codecs.open(todo_file, "w", "utf-8")
+            todo.writelines(insn_data)
+            # ensure the filesystem has the correct contents
+            todo.stream.flush()
+            os.fsync(todo.stream.fileno())
+        finally:
+            todo.close()
+
     def _shorten(self, commit):
 
         if not commit:
@@ -106,31 +150,24 @@ class RebaseEditor(GitMixin, LogDedentMixin):
 
     def _set_editor(self, editor):
 
-        if self.git_sequence_editor:
+        env = os.environ.copy()
-            self._saveeditor = self.git_sequence_editor
+        # if git is new enough, we can edit the sequence without overriding
-            if self._interactive == 'debug':
+        # the editor, which allows rebase to call the correct editor if
-                os.environ['GIT_UPSTREAM_GIT_SEQUENCE_EDITOR'] = \
+        # reaches a 'reword' command before it has exited for the first time
-                    self._saveeditor
+        # otherwise the custom editor of git-upstream will executed with
-            os.environ['GIT_SEQUENCE_EDITOR'] = editor
+        # the path to a commit message as an argument and will need to be able
+        # to call the preferred user editor instead
+        if self.git.version_info >= (1, 7, 8):
+            env['GIT_SEQUENCE_EDITOR'] = editor
         else:
-            self._saveeditor = self.git_editor
+            env['GIT_UPSTREAM_GIT_EDITOR'] = self.git_editor
-            if self._interactive == 'debug':
+            env['GIT_EDITOR'] = editor
-                os.environ['GIT_UPSTREAM_GIT_EDITOR'] = self._saveeditor
+        return env
-            os.environ['GIT_EDITOR'] = editor
 
-    def _unset_editor(self):
+    def cleanup(self):
-
+        todo_file = os.path.join(self.repo.git_dir, REBASE_EDITOR_TODO)
-        for var in ['GIT_SEQUENCE_EDITOR', 'GIT_EDITOR']:
+        if os.path.exists(todo_file):
-            # GIT_UPSTREAM_* variations should only be set if script was in a
+            os.remove(todo_file)
-            # debug mode.
-            if os.environ.get('GIT_UPSTREAM_' + var, None):
-                del os.environ['GIT_UPSTREAM_' + var]
-            # Restore previous editor only if the environment var is set. This
-            # isn't perfect since we should probably unset the env var if it
-            # wasn't previously set, but this shouldn't cause any problems.
-            if os.environ.get(var, None):
-                os.environ[var] = self._saveeditor
-                break
 
     def run(self, commits, *args, **kwargs):
         """
@@ -144,37 +181,47 @@ class RebaseEditor(GitMixin, LogDedentMixin):
         todo_file = self._write_todo(commits, *args, **kwargs)
         if self._interactive:
             # spawn the editor
+            # It is not safe to redirect I/O channels as most editors will
+            # be expecting that I/O is from/to proper terminal. YMMV
             user_editor = self.git_sequence_editor or self.git_editor
             status = subprocess.call("%s %s" % (user_editor, todo_file),
                                      shell=True)
-            if status:
+            if status != 0:
                 return status, None, "Editor returned non-zero exit code"
 
         editor = "%s %s" % (self.editor, todo_file)
-        self._set_editor(editor)
+        environ = self._set_editor(editor)
 
-        try:
+        cmd = ['git', 'rebase', '--interactive']
-            if self._interactive == 'debug':
+        cmd.extend(self.git.transform_kwargs(**kwargs))
-                # In general it's not recommended to run rebase in direct
+        cmd.extend(args)
-                # interactive mode because it's not possible to capture the
+        mode = os.environ.get('TEST_GIT_UPSTREAM_REBASE_EDITOR', "")
-                # stdout/stderr, but sometimes it's useful to allow it for
+        if mode.lower() == "debug":
-                # debugging to check the final result.
+            # In general it's not recommended to run rebase in direct
-                #
+            # interactive mode because it's not possible to capture the
-                # It is not safe to redirect I/O channels as most editors will
+            # stdout/stderr, but sometimes it's useful to allow it for
-                # be expecting that I/O is from/to proper terminal. YMMV
+            # debugging to check the final result.
-                cmd = ['git', 'rebase', '--interactive']
+            try:
-                cmd.extend(self.git.transform_kwargs(**kwargs))
-                cmd.extend(args)
                 return subprocess.call(cmd), None, None
-            else:
+            finally:
-                return self.git.rebase(interactive=True, with_exceptions=False,
+                self.cleanup()
-                                       with_extended_output=True, *args,
+        elif mode == "1":
-                                       **kwargs)
+            # run in test mode to avoid replacing the existing process
-        finally:
+            # to keep the majority of tests simple and only require special
-            os.remove(todo_file)
+            # launching code for those tests written to check the rebase
-            # make sure to remove the environment tweaks added so as not to
+            # resume behaviour
-            # impact any subsequent use of git commands using editors
+            try:
-            self._unset_editor()
+                return 0, subprocess.check_output(
+                    cmd, stderr=subprocess.STDOUT, env=environ), None
+            except subprocess.CalledProcessError as e:
+                return e.returncode, e.output, None
+            finally:
+                self.cleanup()
+        else:
+            self._insert_exec_to_todo()
+
+            cmd.append(environ)
+            os.execlpe('git', *cmd)
 
     @property
     def git_sequence_editor(self):

git_upstream/main.py

@@ -22,6 +22,7 @@ Command-line tool for tracking upstream revisions
 
 import argparse
 import logging
+import os
 import sys
 
 import git
@@ -63,6 +64,15 @@ def build_parsers():
 
     subcommand_parsers = commands.get_subcommands(parser)
 
+    # calculate the correct path to allow the program be re-executed
+    program = sys.argv[0]
+    if os.path.split(program)[-1] != 'git-upstream':
+        script_cmdline = ['python', program]
+    else:
+        script_cmdline = [program]
+
+    parser.set_defaults(script_cmdline=script_cmdline)
+
     return subcommand_parsers, parser
 
 

git_upstream/rebase_editor.py

@@ -80,29 +80,36 @@ def main():
     args = parser.parse_args()
     VERBOSE = args.verbose
 
-    # don't attempt to use stdin to pass the information between the parent
+    # If called with a commit message file as the argument, skip this next
-    # process through 'git-rebase' and this script, as many editors will
+    # section, and try to spawn the user preferred editor. Should only be
-    # have problems if stdin is a pipe.
+    # needed if reword command is encountered by git-rebase before an edit
-    if VERBOSE:
+    # or conflict has occurred and git is older than 1.7.8, as otherwise
-        print("rebase-editor: Replacing contents of rebase instructions file")
+    # sequence.editor will be used instead , which will ensure the normal
-    rebase_replace_insn(args.ofile, open(args.ifile, 'r'))
+    # editor is called by rebase for the commit message, and this editor is
-
+    # limited to only modifying the instruction sequence.
-    # if interactive mode, attempt to exec the editor defined by the user
+    if os.path.basename(args.ofile) != "COMMIT_EDITMSG":
-    # for use with git
+        # don't attempt to use stdin to pass the information between the parent
-    if not args.interactive:
+        # process through 'git-rebase' and this script, as many editors will
+        # have problems if stdin is a pipe.
         if VERBOSE:
-            print("rebase-editor: Interactive mode not enabled")
+            print("rebase-editor: Replacing rebase instructions")
-        sys.exit(0)
+        rebase_replace_insn(args.ofile, open(args.ifile, 'r'))
+
+        # if interactive mode, attempt to exec the editor defined by the user
+        # for use with git
+        if not args.interactive:
+            if VERBOSE:
+                print("rebase-editor: Interactive mode not enabled")
+            sys.exit(0)
 
     # calling code should only override one of the two editor variables,
     # starting with the one with the highest precedence
     editor = None
-    env = os.environ
     for var in ['GIT_SEQUENCE_EDITOR', 'GIT_EDITOR']:
-        editor = env.get('GIT_UPSTREAM_' + var, None)
+        editor = os.environ.get('GIT_UPSTREAM_' + var, None)
         if editor:
-            del env['GIT_UPSTREAM_' + var]
+            del os.environ['GIT_UPSTREAM_' + var]
-            env[var] = editor
+            os.environ[var] = editor
             break
 
     if editor:
@@ -112,11 +119,10 @@ def main():
         sys.stdin.flush()
         sys.stdout.flush()
         sys.stderr.flush()
-        os.execvpe(editor, editor_args, env=env)
+        os.execvp(editor, editor_args)
 
     sys.stderr.write("rebase-editor: No git EDITOR variables defined in "
-                     "environment to call as requested by the "
+                     "environment to call as required.\n")
-                     "--interactive option.\n")
     sys.exit(2)
 
 if __name__ == '__main__':

git_upstream/tests/commands/import/interactive_scenarios/basic.yaml

@@ -0,0 +1,41 @@
+#
+# Copyright (c) 2016 Hewlett-Packard Enterprise Development Company, L.P.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+---
+- desc: |
+    Test that default behaviour and options work
+
+    Repository layout being checked (assumed already replayed)
+
+          C---D               local/master
+         /
+    A---B---E---F             upstream/master
+
+  tree:
+    - [A, []]
+    - [B, [A]]
+    - [C, [B]]
+    - [D, [C]]
+    - [E, [B]]
+    - [F, [E]]
+
+  branches:
+    head: [master, D]
+    upstream: [upstream/master, F]
+
+  parser_args: [-v, import, -i, upstream/master]
+
+  expect_rebased: [C, D]

git_upstream/tests/commands/import/interactive_scenarios/import_no_merge.yaml

@@ -0,0 +1,65 @@
+#
+# Copyright (c) 2016 Hewlett-Packard Enterprise Development Company, L.P.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+---
+- desc: |
+    Test the --no-merge option to the import command (interactive)
+
+    Given manual intervention to resolve conflicts and complete
+    the import, check that if originally given --no-merge, that
+    correctly reach the end of the rebase and skip performing the
+    final merge.
+
+    Repository layout being checked (assumed already replayed)
+
+          C---D                 local/master
+         /
+    A---B---E---F               custom/master
+
+
+    Test that result is as follows
+
+            C---D               local/master
+           /
+          /       C1---D1       import/F
+         /       /
+    A---B---E---F               custom/master
+
+
+
+  tree:
+    - [A, []]
+    - [B, [A]]
+    - [C, [B]]
+    - [D, [C]]
+    - [E, [B]]
+    - [F, [E]]
+    - [C1, [F]]
+    - [D1, [C1]]
+
+  branches:
+    head: [master, D]
+    upstream: [custom/master, F]
+
+  parser_args:
+    - -q
+    - import
+    - --no-merge
+    - --import-branch=import/F
+    - --into=master
+    - custom/master
+
+  check_merge: False

git_upstream/tests/commands/import/test_import.py

@@ -18,6 +18,7 @@
 import inspect
 import os
 
+import mock
 from testscenarios import TestWithScenarios
 from testtools.content import text_content
 from testtools.matchers import Contains
@@ -29,6 +30,8 @@ from git_upstream.tests.base import BaseTestCase
 from git_upstream.tests.base import get_scenarios
 
 
+@mock.patch.dict('os.environ',
+                 {'TEST_GIT_UPSTREAM_REBASE_EDITOR': '1'})
 class TestImportCommand(TestWithScenarios, BaseTestCase):
 
     commands, parser = main.build_parsers()
@@ -97,6 +100,10 @@ class TestImportCommand(TestWithScenarios, BaseTestCase):
         if extra_test_func:
             extra_test_func()
 
+    def _verify_basic(self):
+
+        self.assertThat(self.git.log(n=1), Contains("Merge branch 'import/"))
+
     def _verify_basic_additional_missed(self):
         """Additional verification that test produces a warning"""
 

git_upstream/tests/commands/import/test_interactive.py

@@ -0,0 +1,109 @@
+# Copyright (c) 2016 Hewlett-Packard Development Company, L.P.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Tests for the --interactive option to the  'import' command"""
+
+import os
+import subprocess
+
+import mock
+from testscenarios import TestWithScenarios
+from testtools.content import text_content
+from testtools.matchers import Contains
+from testtools.matchers import Equals
+from testtools.matchers import Not
+
+from git_upstream.lib.pygitcompat import Commit
+from git_upstream import main
+from git_upstream.tests.base import BaseTestCase
+from git_upstream.tests.base import get_scenarios
+
+
+@mock.patch.dict('os.environ', {'GIT_SEQUENCE_EDITOR': 'cat'})
+class TestImportInteractiveCommand(TestWithScenarios, BaseTestCase):
+
+    commands, parser = main.build_parsers()
+    scenarios = get_scenarios(os.path.join(os.path.dirname(__file__),
+                              "interactive_scenarios"))
+
+    def setUp(self):
+        # add description in case parent setup fails.
+        self.addDetail('description', text_content(self.desc))
+
+        script_cmdline = self.parser.get_default('script_cmdline')
+        script_cmdline[-1] = os.path.join(os.getcwd(), main.__file__)
+        self.parser.set_defaults(script_cmdline=script_cmdline)
+
+        # builds the tree to be tested
+        super(TestImportInteractiveCommand, self).setUp()
+
+    def test_interactive(self):
+        upstream_branch = self.branches['upstream'][0]
+        target_branch = self.branches['head'][0]
+
+        cmdline = self.parser.get_default('script_cmdline') + self.parser_args
+        try:
+            output = subprocess.check_output(cmdline, stderr=subprocess.STDOUT)
+        except subprocess.CalledProcessError as cpe:
+            self.addDetail('subprocess-output',
+                           text_content(cpe.output.decode('utf-8')))
+            raise
+        self.addDetail('subprocess-output',
+                       text_content(output.decode('utf-8')))
+
+        expected = getattr(self, 'expect_rebased', [])
+        if expected:
+            changes = list(Commit.iter_items(
+                self.repo, '%s..%s^2' % (upstream_branch, target_branch)))
+            self.assertThat(
+                len(changes), Equals(len(expected)),
+                "should only have seen %s changes, got: %s" %
+                (len(expected),
+                 ", ".join(["%s:%s" % (commit.hexsha,
+                                       commit.message.splitlines()[0])
+                           for commit in changes])))
+
+            # expected should be listed in order from oldest to newest, so
+            # reverse changes to match as it would be newest to oldest.
+            changes.reverse()
+            for commit, node in zip(changes, expected):
+                subject = commit.message.splitlines()[0]
+                node_subject = self.gittree.graph[node].message.splitlines()[0]
+                self.assertThat(subject, Equals(node_subject),
+                                "subject '%s' of commit '%s' does not match "
+                                "subject '%s' of node '%s'" % (
+                                    subject, commit.hexsha, node_subject,
+                                    node))
+        import_branch = [head for head in self.repo.heads
+                         if str(head).startswith("import") and
+                         not str(head).endswith("-base")]
+
+        self.assertThat(self.git.rev_parse(import_branch),
+                        Not(Equals(self.git.rev_parse(target_branch))),
+                        "Import branch and target should have identical "
+                        "contents, but not be the same")
+
+        # allow disabling of checking the merge commit contents
+        # as some tests won't result in an import
+        if getattr(self, 'check_merge', True):
+            commit_message = self.git.log(target_branch, n=1)
+            self.assertThat(commit_message,
+                            Contains("of '%s' into '%s'" % (upstream_branch,
+                                                            target_branch)))
+
+        # allow additional test specific verification methods below
+        extra_test_func = getattr(self, '_verify_%s' % self.name, None)
+        if extra_test_func:
+            extra_test_func()