From 71437c0ba5ea0065c50d3b607c7de1ed790a25f0 Mon Sep 17 00:00:00 2001 From: "sbc@chromium.org" Date: Thu, 9 Apr 2015 19:29:40 +0000 Subject: [PATCH] git-squash-branch: handle empty squashes Error out of the current tree is dirty (previously the dirty content would be incorporated silently into the newly squashed branch!). Review URL: https://codereview.chromium.org/1064933004 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@294744 0039d316-1c4b-4281-b951-d872f2087c98 --- git_cl.py | 26 ++++++-------------------- git_common.py | 25 +++++++++++++++++++++++++ git_squash_branch.py | 6 ++++-- testing_support/git_test_utils.py | 1 - tests/git_cl_test.py | 8 +------- tests/git_common_test.py | 17 ++++++++++++++++- 6 files changed, 52 insertions(+), 31 deletions(-) diff --git a/git_cl.py b/git_cl.py index 4eee3f152..65c7fc79f 100755 --- a/git_cl.py +++ b/git_cl.py @@ -201,20 +201,6 @@ def add_git_similarity(parser): parser.parse_args = Parse -def is_dirty_git_tree(cmd): - # Make sure index is up-to-date before running diff-index. - RunGit(['update-index', '--refresh', '-q'], error_ok=True) - dirty = RunGit(['diff-index', '--name-status', 'HEAD']) - if dirty: - print 'Cannot %s with a dirty tree. You must commit locally first.' % cmd - print 'Uncommitted files: (git diff-index --name-status HEAD)' - print dirty[:4096] - if len(dirty) > 4096: - print '... (run "git diff-index --name-status HEAD" to see full output).' - return True - return False - - def MatchSvnGlob(url, base_url, glob_spec, allow_wildcards): """Return the corresponding git ref if |base_url| together with |glob_spec| matches the full |url|. @@ -1655,7 +1641,7 @@ def CMDpresubmit(parser, args): help='Run checks even if tree is dirty') (options, args) = parser.parse_args(args) - if not options.force and is_dirty_git_tree('presubmit'): + if not options.force and git_common.is_dirty_git_tree('presubmit'): print 'use --force to check even if tree is dirty.' return 1 @@ -2032,7 +2018,7 @@ def CMDupload(parser, args): add_git_similarity(parser) (options, args) = parser.parse_args(args) - if is_dirty_git_tree('upload'): + if git_common.is_dirty_git_tree('upload'): return 1 options.reviewers = cleanup_list(options.reviewers) @@ -2162,7 +2148,7 @@ def SendUpstream(parser, args, cmd): base_branch = args[0] base_has_submodules = IsSubmoduleMergeCommit(base_branch) - if is_dirty_git_tree(cmd): + if git_common.is_dirty_git_tree(cmd): return 1 # This rev-list syntax means "show all commits not in my branch that @@ -2538,7 +2524,7 @@ def CMDpatch(parser, args): issue_arg = args[0] # We don't want uncommitted changes mixed up with the patch. - if is_dirty_git_tree('patch'): + if git_common.is_dirty_git_tree('patch'): return 1 # TODO(maruel): Use apply_issue.py @@ -2558,7 +2544,7 @@ def CMDpatch(parser, args): def PatchIssue(issue_arg, reject, nocommit, directory): # There's a "reset --hard" when failing to apply the patch. In order # not to destroy users' data, make sure the tree is not dirty here. - assert(not is_dirty_git_tree('apply')) + assert(not git_common.is_dirty_git_tree('apply')) if type(issue_arg) is int or issue_arg.isdigit(): # Input is an issue id. Figure out the URL. @@ -2921,7 +2907,7 @@ def CMDdiff(parser, args): # Staged changes would be committed along with the patch from last # upload, hence counted toward the "last upload" side in the final # diff output, and this is not what we want. - if is_dirty_git_tree('diff'): + if git_common.is_dirty_git_tree('diff'): return 1 cl = Changelist() diff --git a/git_common.py b/git_common.py index a18f59f16..6b21050b2 100644 --- a/git_common.py +++ b/git_common.py @@ -602,6 +602,24 @@ def set_config(option, value, scope='local'): run('config', '--' + scope, option, value) +def get_dirty_files(): + # Make sure index is up-to-date before running diff-index. + run_with_retcode('update-index', '--refresh', '-q') + return run('diff-index', '--name-status', 'HEAD') + + +def is_dirty_git_tree(cmd): + dirty = get_dirty_files() + if dirty: + print 'Cannot %s with a dirty tree. You must commit locally first.' % cmd + print 'Uncommitted files: (git diff-index --name-status HEAD)' + print dirty[:4096] + if len(dirty) > 4096: # pragma: no cover + print '... (run "git diff-index --name-status HEAD" to see full output).' + return True + return False + + def squash_current_branch(header=None, merge_base=None): header = header or 'git squash commit.' merge_base = merge_base or get_or_create_merge_base(current_branch()) @@ -610,7 +628,14 @@ def squash_current_branch(header=None, merge_base=None): log_msg += '\n' log_msg += run('log', '--reverse', '--format=%H%n%B', '%s..HEAD' % merge_base) run('reset', '--soft', merge_base) + + if not get_dirty_files(): + # Sometimes the squash can result in the same tree, meaning that there is + # nothing to commit at this point. + print 'Nothing to commit; squashed branch is empty' + return False run('commit', '-a', '-F', '-', indata=log_msg) + return True def tags(*args): diff --git a/git_squash_branch.py b/git_squash_branch.py index 83acfa7f0..33366d28b 100755 --- a/git_squash_branch.py +++ b/git_squash_branch.py @@ -6,7 +6,7 @@ import argparse import sys -from git_common import squash_current_branch +import git_common def main(args): parser = argparse.ArgumentParser() @@ -14,7 +14,9 @@ def main(args): '-m', '--message', metavar='', default='git squash commit.', help='Use the given as the first line of the commit message.') opts = parser.parse_args(args) - squash_current_branch(opts.message) + if git_common.is_dirty_git_tree('squash-branch'): + return 1 + git_common.squash_current_branch(opts.message) return 0 diff --git a/testing_support/git_test_utils.py b/testing_support/git_test_utils.py index 10e54f51b..fbe7c73c3 100644 --- a/testing_support/git_test_utils.py +++ b/testing_support/git_test_utils.py @@ -349,7 +349,6 @@ class GitRepo(object): env['GIT_%s' % singleton] = str(val) return env - def git(self, *args, **kwargs): """Runs a git command specified by |args| in this repo.""" assert self.repo_path is not None diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 31eb97221..71e8c60cc 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -75,7 +75,7 @@ class TestGitCl(TestCase): self.mock(subprocess2, 'check_call', self._mocked_call) self.mock(subprocess2, 'check_output', self._mocked_call) self.mock(subprocess2, 'communicate', self._mocked_call) - self.mock(subprocess2, 'Popen', self._mocked_call) + self.mock(git_common, 'is_dirty_git_tree', lambda x: False) self.mock(git_common, 'get_or_create_merge_base', lambda *a: ( self._mocked_call(['get_or_create_merge_base']+list(a)))) @@ -156,8 +156,6 @@ class TestGitCl(TestCase): similarity_call, ((['git', 'symbolic-ref', 'HEAD'],), 'master'), find_copies_call, - ((['git', 'update-index', '--refresh', '-q'],), ''), - ((['git', 'diff-index', '--name-status', 'HEAD'],), ''), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', 'branch.master.merge'],), 'master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), @@ -280,8 +278,6 @@ class TestGitCl(TestCase): ((['git', 'rev-list', '--merges', '--grep=^SVN changes up to revision [0-9]*$', 'refs/remotes/origin/master^!'],), ''), - ((['git', 'update-index', '--refresh', '-q'],), ''), - ((['git', 'diff-index', '--name-status', 'HEAD'],), ''), ((['git', 'rev-list', '^refs/heads/working', 'refs/remotes/origin/master'],), ''), @@ -544,8 +540,6 @@ class TestGitCl(TestCase): ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', '--int', '--get', 'branch.master.git-find-copies'],), ''), - ((['git', 'update-index', '--refresh', '-q'],), ''), - ((['git', 'diff-index', '--name-status', 'HEAD'],), ''), ((['git', 'symbolic-ref', 'HEAD'],), 'master'), ((['git', 'config', 'branch.master.merge'],), 'master'), ((['git', 'config', 'branch.master.remote'],), 'origin'), diff --git a/tests/git_common_test.py b/tests/git_common_test.py index ee44eb390..029c823c1 100755 --- a/tests/git_common_test.py +++ b/tests/git_common_test.py @@ -561,10 +561,17 @@ class GitMutableStructuredTest(git_test_utils.GitRepoReadWriteTestBase, ('root_A', 'root_X'), ]) + def testIsGitTreeDirty(self): + self.assertEquals(False, self.repo.run(self.gc.is_dirty_git_tree, 'foo')) + self.repo.open('test.file', 'w').write('test data') + self.repo.git('add', 'test.file') + self.assertEquals(True, self.repo.run(self.gc.is_dirty_git_tree, 'foo')) + def testSquashBranch(self): self.repo.git('checkout', 'branch_K') - self.repo.run(self.gc.squash_current_branch, 'cool message') + self.assertEquals(True, self.repo.run(self.gc.squash_current_branch, + 'cool message')) lines = ['cool message', ''] for l in 'HIJK': @@ -580,6 +587,14 @@ class GitMutableStructuredTest(git_test_utils.GitRepoReadWriteTestBase, 'K' ) + def testSquashBranchEmpty(self): + self.repo.git('checkout', 'branch_K') + self.repo.git('checkout', 'branch_G', '.') + self.repo.git('commit', '-m', 'revert all changes no branch') + # Should return False since the quash would result in an empty commit + stdout = self.repo.capture_stdio(self.gc.squash_current_branch)[0] + self.assertEquals(stdout, 'Nothing to commit; squashed branch is empty\n') + def testRebase(self): self.assertSchema(""" A B C D E F G