Fix broken interactive mode to be usable

Switch rebase editor class to exec rebase so that it takes over the
console to allow the user to continue/skip/abort as they would
normally. Update the instruction set for rebase to re-call
git-upstream as a final step for user convenience to finish the import.

When the EDITOR or GIT_EDITOR is defined to change what editor is used
to edit the sequence of instructions to git-rebase, git will recall the
same editor in those variables if one of the instructions is 'reword',
'edit', or 'squash' in order to amend the commit message. Change the
rebaseeditor script to be able to handle being called with a
COMMIT_EDITMSG file when this occurs.

This allows the 'edit', 'reword', 'squash' & 'fixup' commands to git
rebase to function as expected by the user irrespective of whether a
conflict has been encountered first or not.

Still results in git rebase outputting additional information after
the import has completed, but avoids the main issue of the interactive
mode being unusable due to ignoring the result of the git-rebase call
unless it exits with an error, which only occurs for conflicts.

Change-Id: Ic125c85b868d816ef13fac6f503092ac3142b4de
Closes-Bug: #1402032
This commit is contained in:
Darragh Bailey 2016-02-04 12:12:40 +00:00
parent 307637d8b6
commit c195b69c6b
9 changed files with 390 additions and 80 deletions

View File

@ -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

View File

@ -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:

View File

@ -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:
self._saveeditor = self.git_sequence_editor
if self._interactive == 'debug':
os.environ['GIT_UPSTREAM_GIT_SEQUENCE_EDITOR'] = \
self._saveeditor
os.environ['GIT_SEQUENCE_EDITOR'] = editor
env = os.environ.copy()
# if git is new enough, we can edit the sequence without overriding
# the editor, which allows rebase to call the correct editor if
# reaches a 'reword' command before it has exited for the first time
# otherwise the custom editor of git-upstream will executed with
# 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
if self._interactive == 'debug':
os.environ['GIT_UPSTREAM_GIT_EDITOR'] = self._saveeditor
os.environ['GIT_EDITOR'] = editor
env['GIT_UPSTREAM_GIT_EDITOR'] = self.git_editor
env['GIT_EDITOR'] = editor
return env
def _unset_editor(self):
for var in ['GIT_SEQUENCE_EDITOR', 'GIT_EDITOR']:
# GIT_UPSTREAM_* variations should only be set if script was in a
# 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 cleanup(self):
todo_file = os.path.join(self.repo.git_dir, REBASE_EDITOR_TODO)
if os.path.exists(todo_file):
os.remove(todo_file)
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:
if self._interactive == 'debug':
# In general it's not recommended to run rebase in direct
# interactive mode because it's not possible to capture the
# stdout/stderr, but sometimes it's useful to allow it for
# debugging to check the final result.
#
# It is not safe to redirect I/O channels as most editors will
# be expecting that I/O is from/to proper terminal. YMMV
cmd = ['git', 'rebase', '--interactive']
cmd.extend(self.git.transform_kwargs(**kwargs))
cmd.extend(args)
cmd = ['git', 'rebase', '--interactive']
cmd.extend(self.git.transform_kwargs(**kwargs))
cmd.extend(args)
mode = os.environ.get('TEST_GIT_UPSTREAM_REBASE_EDITOR', "")
if mode.lower() == "debug":
# In general it's not recommended to run rebase in direct
# interactive mode because it's not possible to capture the
# stdout/stderr, but sometimes it's useful to allow it for
# debugging to check the final result.
try:
return subprocess.call(cmd), None, None
else:
return self.git.rebase(interactive=True, with_exceptions=False,
with_extended_output=True, *args,
**kwargs)
finally:
os.remove(todo_file)
# make sure to remove the environment tweaks added so as not to
# impact any subsequent use of git commands using editors
self._unset_editor()
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
try:
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):

View File

@ -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

View File

@ -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
# process through 'git-rebase' and this script, as many editors will
# have problems if stdin is a pipe.
if VERBOSE:
print("rebase-editor: Replacing contents of rebase instructions file")
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 called with a commit message file as the argument, skip this next
# section, and try to spawn the user preferred editor. Should only be
# needed if reword command is encountered by git-rebase before an edit
# or conflict has occurred and git is older than 1.7.8, as otherwise
# sequence.editor will be used instead , which will ensure the normal
# editor is called by rebase for the commit message, and this editor is
# limited to only modifying the instruction sequence.
if os.path.basename(args.ofile) != "COMMIT_EDITMSG":
# don't attempt to use stdin to pass the information between the parent
# 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")
sys.exit(0)
print("rebase-editor: Replacing rebase instructions")
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]
env[var] = editor
del os.environ['GIT_UPSTREAM_' + var]
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 "
"--interactive option.\n")
"environment to call as required.\n")
sys.exit(2)
if __name__ == '__main__':

View File

@ -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]

View File

@ -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

View File

@ -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"""

View File

@ -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()