diff --git a/git_cl.py b/git_cl.py index d4486e5b1..b94be71c8 100755 --- a/git_cl.py +++ b/git_cl.py @@ -2583,8 +2583,9 @@ def CMDpatch(parser, args): def PatchIssue(issue_arg, reject, nocommit, directory, auth_config): - # 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. + # PatchIssue should never be called with a dirty tree. It is up to the + # caller to check this, but just in case we assert here since the + # consequences of the caller not checking this could be dire. assert(not git_common.is_dirty_git_tree('apply')) if type(issue_arg) is int or issue_arg.isdigit(): @@ -2635,8 +2636,8 @@ def PatchIssue(issue_arg, reject, nocommit, directory, auth_config): subprocess2.check_call(cmd, env=GetNoGitPagerEnv(), stdin=patch_data, stdout=subprocess2.VOID) except subprocess2.CalledProcessError: - RunGit(['reset', '--hard']) - DieWithError('Failed to apply the patch') + print 'Failed to apply the patch' + return 1 # If we had an issue, commit the current state and register the issue. if not nocommit: @@ -2975,6 +2976,7 @@ def CMDdiff(parser, args): # Patch in the latest changes from rietveld. rtn = PatchIssue(issue, False, False, None, auth_config) if rtn != 0: + RunGit(['reset', '--hard']) return rtn # Switch back to starting branch and diff against the temporary diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index c1eebebf7..f63358116 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -110,7 +110,11 @@ class TestGitCl(TestCase): self.assertTrue( self.calls, '@%d Expected: Actual: %r' % (self._calls_done, args)) - expected_args, result = self.calls.pop(0) + top = self.calls.pop(0) + if len(top) > 2 and top[2]: + raise top[2] + expected_args, result = top + # Also logs otherwise it could get caught in a try/finally and be hard to # diagnose. if expected_args != args: @@ -854,6 +858,47 @@ class TestGitCl(TestCase): git_cl.GetTargetRef('origin', 'refs/remotes/origin/master', None, 'prefix/')) + def test_patch_when_dirty(self): + # Patch when local tree is dirty + self.mock(git_common, 'is_dirty_git_tree', lambda x: True) + self.assertNotEqual(git_cl.main(['patch', '123456']), 0) + + def test_diff_when_dirty(self): + # Do 'git cl diff' when local tree is dirty + self.mock(git_common, 'is_dirty_git_tree', lambda x: True) + self.assertNotEqual(git_cl.main(['diff']), 0) + + def _patch_common(self): + self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda x: '60001') + self.mock(git_cl.Changelist, 'GetPatchSetDiff', lambda *args: None) + self.mock(git_cl.Changelist, 'SetIssue', lambda *args: None) + self.mock(git_cl.Changelist, 'SetPatchset', lambda *args: None) + self.mock(git_cl, 'IsGitVersionAtLeast', lambda *args: True) + + self.calls = [ + ((['git', 'config', 'rietveld.autoupdate'],), ''), + ((['git', 'config', 'rietveld.server'],), 'codereview.example.com'), + ((['git', 'rev-parse', '--show-cdup'],), ''), + ((['sed', '-e', 's|^--- a/|--- |; s|^+++ b/|+++ |'],), ''), + ] + + def test_patch_successful(self): + self._patch_common() + self.calls += [ + ((['git', 'apply', '--index', '-p0', '--3way'],), ''), + ((['git', 'commit', '-m', + 'patch from issue 123456 at patchset 60001 ' + + '(http://crrev.com/123456#ps60001)'],), ''), + ] + self.assertEqual(git_cl.main(['patch', '123456']), 0) + + def test_patch_conflict(self): + self._patch_common() + self.calls += [ + ((['git', 'apply', '--index', '-p0', '--3way'],), '', + subprocess2.CalledProcessError(1, '', '', '', '')), + ] + self.assertNotEqual(git_cl.main(['patch', '123456']), 0) if __name__ == '__main__': git_cl.logging.basicConfig(