Re-land "git-cl: Fix format --dry-run not working with --full."

Re-land fixes format always saying the folder was dirty.

The --full option would skip setting the return value of the format
command when used with --dry-run. This in turn would cause the
presubmit check to always pass when --full was enabled in a repo by
default.

Was discovered in the ANGLE repository.

Bug: angleproject:4241
Change-Id: I986646336d1857af2826863ef4becbf2b9ee3049
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1993910
Commit-Queue: Jamie Madill <jmadill@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>

git_cl.py

@@ -5117,6 +5117,65 @@ def BuildGitDiffCmd(diff_type, upstream_commit, args, allow_prefix=False):
   return diff_cmd
 
 
+def _RunClangFormatDiff(opts, clang_diff_files, top_dir, upstream_commit):
+  """Runs clang-format-diff and sets a return value if necessary."""
+
+  if not clang_diff_files:
+    return 0
+
+  # Set to 2 to signal to CheckPatchFormatted() that this patch isn't
+  # formatted. This is used to block during the presubmit.
+  return_value = 0
+
+  # Locate the clang-format binary in the checkout
+  try:
+    clang_format_tool = clang_format.FindClangFormatToolInChromiumTree()
+  except clang_format.NotFoundError as e:
+    DieWithError(e)
+
+  if opts.full or settings.GetFormatFullByDefault():
+    cmd = [clang_format_tool]
+    if not opts.dry_run and not opts.diff:
+      cmd.append('-i')
+    if opts.dry_run:
+      for diff_file in clang_diff_files:
+        with open(diff_file, 'r') as myfile:
+          code = myfile.read().replace('\r\n', '\n')
+          stdout = RunCommand(cmd + [diff_file], cwd=top_dir)
+          stdout = stdout.replace('\r\n', '\n')
+          if opts.diff:
+            sys.stdout.write(stdout)
+          if code != stdout:
+            return_value = 2
+    else:
+      stdout = RunCommand(cmd + clang_diff_files, cwd=top_dir)
+      if opts.diff:
+        sys.stdout.write(stdout)
+  else:
+    env = os.environ.copy()
+    env['PATH'] = str(os.path.dirname(clang_format_tool))
+    try:
+      script = clang_format.FindClangFormatScriptInChromiumTree(
+          'clang-format-diff.py')
+    except clang_format.NotFoundError as e:
+      DieWithError(e)
+
+    cmd = [sys.executable, script, '-p0']
+    if not opts.dry_run and not opts.diff:
+      cmd.append('-i')
+
+    diff_cmd = BuildGitDiffCmd('-U0', upstream_commit, clang_diff_files)
+    diff_output = RunGit(diff_cmd)
+
+    stdout = RunCommand(cmd, stdin=diff_output, cwd=top_dir, env=env)
+    if opts.diff:
+      sys.stdout.write(stdout)
+    if opts.dry_run and len(stdout) > 0:
+      return_value = 2
+
+  return return_value
+
+
 def MatchingFileType(file_name, extensions):
   """Returns True if the file name ends with one of the given extensions."""
   return bool([ext for ext in extensions if file_name.lower().endswith(ext)])
@@ -5213,45 +5272,8 @@ def CMDformat(parser, args):
   top_dir = os.path.normpath(
       RunGit(["rev-parse", "--show-toplevel"]).rstrip('\n'))
 
-  # Set to 2 to signal to CheckPatchFormatted() that this patch isn't
-  # formatted. This is used to block during the presubmit.
-  return_value = 0
-
-  if clang_diff_files:
-    # Locate the clang-format binary in the checkout
-    try:
-      clang_format_tool = clang_format.FindClangFormatToolInChromiumTree()
-    except clang_format.NotFoundError as e:
-      DieWithError(e)
-
-    if opts.full or settings.GetFormatFullByDefault():
-      cmd = [clang_format_tool]
-      if not opts.dry_run and not opts.diff:
-        cmd.append('-i')
-      stdout = RunCommand(cmd + clang_diff_files, cwd=top_dir)
-      if opts.diff:
-        sys.stdout.write(stdout)
-    else:
-      env = os.environ.copy()
-      env['PATH'] = str(os.path.dirname(clang_format_tool))
-      try:
-        script = clang_format.FindClangFormatScriptInChromiumTree(
-            'clang-format-diff.py')
-      except clang_format.NotFoundError as e:
-        DieWithError(e)
-
-      cmd = [sys.executable, script, '-p0']
-      if not opts.dry_run and not opts.diff:
-        cmd.append('-i')
-
-      diff_cmd = BuildGitDiffCmd('-U0', upstream_commit, clang_diff_files)
-      diff_output = RunGit(diff_cmd)
-
-      stdout = RunCommand(cmd, stdin=diff_output, cwd=top_dir, env=env)
-      if opts.diff:
-        sys.stdout.write(stdout)
-      if opts.dry_run and len(stdout) > 0:
-        return_value = 2
+  return_value = _RunClangFormatDiff(opts, clang_diff_files, top_dir,
+                                     upstream_commit)
 
   # Similar code to above, but using yapf on .py files rather than clang-format
   # on C/C++ files
@@ -5336,7 +5358,7 @@ def CMDformat(parser, args):
       stdout = RunCommand(command, cwd=top_dir)
       if opts.dry_run and stdout:
         return_value = 2
-    except dart_format.NotFoundError as e:
+    except dart_format.NotFoundError:
       print('Warning: Unable to check Dart code formatting. Dart SDK not '
             'found in this checkout. Files in other languages are still '
             'formatted.')

tests/git_cl_test.py

@@ -32,6 +32,7 @@ import metrics
 # We have to disable monitoring before importing git_cl.
 metrics.DISABLE_METRICS_COLLECTION = True
 
+import clang_format
 import gerrit_util
 import git_cl
 import git_common
@@ -3441,20 +3442,62 @@ class CMDFormatTestCase(TestCase):
 
   def setUp(self):
     super(CMDFormatTestCase, self).setUp()
+    mock.patch('git_cl.RunCommand').start()
+    mock.patch('clang_format.FindClangFormatToolInChromiumTree').start()
+    mock.patch('clang_format.FindClangFormatScriptInChromiumTree').start()
+    mock.patch('git_cl.settings').start()
     self._top_dir = tempfile.mkdtemp()
+    self.addCleanup(mock.patch.stopall)
 
   def tearDown(self):
     shutil.rmtree(self._top_dir)
     super(CMDFormatTestCase, self).tearDown()
 
+  def _make_temp_file(self, fname, contents):
+    with open(os.path.join(self._top_dir, fname), 'w') as tf:
+      tf.write('\n'.join(contents))
+
   def _make_yapfignore(self, contents):
-    with open(os.path.join(self._top_dir, '.yapfignore'), 'w') as yapfignore:
-      yapfignore.write('\n'.join(contents))
+    self._make_temp_file('.yapfignore', contents)
 
   def _check_yapf_filtering(self, files, expected):
     self.assertEqual(expected, git_cl._FilterYapfIgnoredFiles(
         files, git_cl._GetYapfIgnorePatterns(self._top_dir)))
 
+  def testClangFormatDiffFull(self):
+    self._make_temp_file('test.cc', ['// test'])
+    git_cl.settings.GetFormatFullByDefault.return_value = False
+    diff_file = [os.path.join(self._top_dir, 'test.cc')]
+    mock_opts = mock.Mock(full=True, dry_run=True, diff=False)
+
+    # Diff
+    git_cl.RunCommand.return_value = '  // test'
+    return_value = git_cl._RunClangFormatDiff(mock_opts, diff_file,
+                                              self._top_dir, 'HEAD')
+    self.assertEqual(2, return_value)
+
+    # No diff
+    git_cl.RunCommand.return_value = '// test'
+    return_value = git_cl._RunClangFormatDiff(mock_opts, diff_file,
+                                              self._top_dir, 'HEAD')
+    self.assertEqual(0, return_value)
+
+  def testClangFormatDiff(self):
+    git_cl.settings.GetFormatFullByDefault.return_value = False
+    mock_opts = mock.Mock(full=False, dry_run=True, diff=False)
+
+    # Diff
+    git_cl.RunCommand.return_value = 'error'
+    return_value = git_cl._RunClangFormatDiff(mock_opts, ['.'], self._top_dir,
+                                              'HEAD')
+    self.assertEqual(2, return_value)
+
+    # No diff
+    git_cl.RunCommand.return_value = ''
+    return_value = git_cl._RunClangFormatDiff(mock_opts, ['.'], self._top_dir,
+                                              'HEAD')
+    self.assertEqual(0, return_value)
+
   def testYapfignoreExplicit(self):
     self._make_yapfignore(['foo/bar.py', 'foo/bar/baz.py'])
     files = [