From a24debcbf8ca835cb6a9170065b5702a3963fa54 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Mon, 19 Sep 2016 18:44:23 +0100 Subject: [PATCH] Ensure correct mode of git-rebase executed When not using interactive mode, ensure that git rebase is called as a subprocess to ensure git-upstream can switch the final branch to the merged result after completion of the import. Only where interactive mode is enabled should the branch remain on the import branch after running to completion and leave it to the user to switch to the target branch, as otherwise git-rebase will throw an error. Change-Id: I633a371775b5a177c76529a49b44fb841d9c1332 Related-Bug: #1625876 --- git_upstream/lib/rebaseeditor.py | 31 ++++++++++++++++--- .../tests/commands/import/test_import.py | 9 ++++-- .../tests/commands/import/test_interactive.py | 10 ++++-- 3 files changed, 40 insertions(+), 10 deletions(-) diff --git a/git_upstream/lib/rebaseeditor.py b/git_upstream/lib/rebaseeditor.py index ffef11e..44b0fab 100644 --- a/git_upstream/lib/rebaseeditor.py +++ b/git_upstream/lib/rebaseeditor.py @@ -205,11 +205,32 @@ class RebaseEditor(GitMixin, LogDedentMixin): return subprocess.call(cmd), None, None finally: self.cleanup() - elif mode == "1": - # run in test mode to avoid replacing the existing process - # to keep the majority of tests simple and only require special - # launching code for those tests written to check the rebase - # resume behaviour + elif not self._interactive: + # If in non-interactive mode use subprocess instead of exec + # + # This ensures that if no conflicts occur, that the calling + # git-upstream process will be able to switch the current + # branch after the git-rebase subprocess exits. This is not + # possible when using exec to have git-rebase replace the + # existing process. Since git-rebase performs checks once + # it is completed running the instructions (todo file), + # changing the current branch checked out in the git + # repository via the final instruction (calling + # `git-upstream import --finish ...`) results in git-rebase + # exiting with an exception. + # + # For interactive mode it is impossible to perform a rebase + # via subprocess and have it correctly attach an editor to + # the console for users to edit/reword commits. The + # consequence of using exec to support interactive usage + # prevents correctly switching final branch to anything other + # than the branch that git-rebase was started on (which will + # be the import branch). + # + # As interactive mode involves user intervention it seems a + # reasonable compromise to require manual switch of branches + # after being finished until such time that an alternative + # solution can be found. try: return 0, subprocess.check_output( cmd, stderr=subprocess.STDOUT, env=environ), None diff --git a/git_upstream/tests/commands/import/test_import.py b/git_upstream/tests/commands/import/test_import.py index 6366414..f317c48 100644 --- a/git_upstream/tests/commands/import/test_import.py +++ b/git_upstream/tests/commands/import/test_import.py @@ -18,11 +18,11 @@ import inspect import os -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 @@ -30,8 +30,6 @@ 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): scenarios = get_scenarios(os.path.join(os.path.dirname(__file__), @@ -56,6 +54,11 @@ class TestImportCommand(TestWithScenarios, BaseTestCase): self.assertThat(args.cmd.run(args), Equals(True), "import command failed to complete successfully") + # assuming non-interactive we should *NOT* see the following message + # appear in the logged output. + self.assertThat(self.logger.output, + Not(Contains("Successfully rebased and updated"))) + # perform sanity checks on results self._check_tree_state() diff --git a/git_upstream/tests/commands/import/test_interactive.py b/git_upstream/tests/commands/import/test_interactive.py index f044fde..d79994c 100644 --- a/git_upstream/tests/commands/import/test_interactive.py +++ b/git_upstream/tests/commands/import/test_interactive.py @@ -55,13 +55,14 @@ class TestImportInteractiveCommand(TestWithScenarios, BaseTestCase): cmdline = self.parser.get_default('script_cmdline') + self.parser_args try: - output = subprocess.check_output(cmdline, stderr=subprocess.STDOUT) + self.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'))) + text_content(self.output.decode('utf-8'))) expected = getattr(self, 'expect_rebased', []) if expected: @@ -107,3 +108,8 @@ class TestImportInteractiveCommand(TestWithScenarios, BaseTestCase): extra_test_func = getattr(self, '_verify_%s' % self.name, None) if extra_test_func: extra_test_func() + + def _verify_basic(self): + self.assertThat( + self.output.decode('utf-8'), + Contains("Successfully rebased and updated refs/heads/import/"))