git-clang-format: Add --cached option to format index

[owenpan]

Summary

Add --cached option to git-clang-format which behaves analogously to the use of --cached for other git subcommands, by causing the operation to work against the index state rather than the working directory state.

This can be particularly useful for hook scripts which need to check or change the formatting of the index state before commit.

[kevinoid]

Fixed handling of case when no formatting changes are made but old_tree != new_tree in apply_changes.

[MyDeveloperDay]

Is this revision still relavant, I find it annoying that I have to run git add again on any files that are already added but have been formatted.

If you think this is useful and can rebase it perhaps we can go around the review cycle again with a fresh set of eyes

[owenpan]

We have --cached now.

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

tools/clang-format/git-clang-format

default_extensions = ','.join([
       ])
 
   p = argparse.ArgumentParser(
     usage=usage, formatter_class=argparse.RawDescriptionHelpFormatter,
     description=desc)
   p.add_argument('--binary',
                  default=config.get('clangformat.binary', 'clang-format'),
                  help='path to clang-format'),
+  p.add_argument('--cached', action='store_true',
+                 help='format index instead of working directory'),
   p.add_argument('--commit',
                  default=config.get('clangformat.commit', 'HEAD'),
                  help='default commit to use if none is specified'),
   p.add_argument('--diff', action='store_true',
                  help='print a diff instead of applying the changes')
   p.add_argument('--extensions',
                  default=config.get('clangformat.extensions',
                                     default_extensions),
def main():
 
   opts.verbose -= opts.quiet
   del opts.quiet
 
   commits, files = interpret_args(opts.args, dash_dash, opts.commit)
   if len(commits) > 1:
     if not opts.diff:
       die('--diff is required when two commits are given')
+    if opts.cached:
+      die('--cached is not applicable when two commits are given')
   else:
     if len(commits) > 2:
       die('at most two commits allowed; %d given' % len(commits))
-  changed_lines = compute_diff_and_extract_lines(commits, files)
+  changed_lines = compute_diff_and_extract_lines(commits, files, opts.cached)
   if opts.verbose >= 1:
     ignored_files = set(changed_lines)
   filter_by_extension(changed_lines, opts.extensions.lower().split(','))
   if opts.verbose >= 1:
     ignored_files.difference_update(changed_lines)
     if ignored_files:
       print('Ignoring changes in the following files (wrong extension):')
       for filename in ignored_files:
         print('    %s' % filename)
     if changed_lines:
       print('Running clang-format on the following files:')
       for filename in changed_lines:
         print('    %s' % filename)
   if not changed_lines:
     print('no modified files to format')
     return
   # The computed diff outputs absolute paths, so we must cd before accessing
   # those files.
   cd_to_toplevel()
   if len(commits) > 1:
     old_tree = commits[1]
-    new_tree = run_clang_format_and_save_to_tree(changed_lines,
-                                                 revision=commits[1],
-                                                 binary=opts.binary,
-                                                 style=opts.style)
+    fmt_tree = commits[1]
+  elif opts.cached:
+    old_tree = run('git', 'write-tree')
+    fmt_tree = old_tree
   else:
     old_tree = create_tree_from_workdir(changed_lines)
-    new_tree = run_clang_format_and_save_to_tree(changed_lines,
-                                                 binary=opts.binary,
-                                                 style=opts.style)
+    fmt_tree = None
+                                               revision=fmt_tree,
   if opts.verbose >= 1:
     print('old tree: %s' % old_tree)
     print('new tree: %s' % new_tree)
   if old_tree == new_tree:
     if opts.verbose >= 0:
       print('clang-format did not modify any files')
   elif opts.diff:
     print_diff(old_tree, new_tree)
   else:
     changed_files = apply_changes(old_tree, new_tree, force=opts.force,
-                                  patch_mode=opts.patch)
+                                  patch_mode=opts.patch, cached=opts.cached)
     if (opts.verbose >= 0 and not opts.patch) or opts.verbose >= 1:
       print('changed files:')
       for filename in changed_files:
         print('    %s' % filename)
 
 
 def load_git_config(non_string_options=None):
   """Return the git configuration as a dictionary.
def get_object_type(value):
   cmd = ['git', 'cat-file', '-t', value]
   p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
   stdout, stderr = p.communicate()
   if p.returncode != 0:
     return None
   return convert_string(stdout.strip())
 
 
-def compute_diff_and_extract_lines(commits, files):
+def compute_diff_and_extract_lines(commits, files, cached=False):
   """Calls compute_diff() followed by extract_lines()."""
-  diff_process = compute_diff(commits, files)
+  diff_process = compute_diff(commits, files, cached)
   changed_lines = extract_lines(diff_process.stdout)
   diff_process.stdout.close()
   diff_process.wait()
   if diff_process.returncode != 0:
     # Assume error was already printed to stderr.
     sys.exit(2)
   return changed_lines
 
 
-def compute_diff(commits, files):
+def compute_diff(commits, files, cached=False):
   """Return a subprocess object producing the diff from `commits`.
 
   The return value's `stdin` file object will produce a patch with the
   differences between the working directory and the first commit if a single
   one was specified, or the difference between both specified commits, filtered
   on `files` (if non-empty).  Zero context lines are used in the patch."""
   git_tool = 'diff-index'
   if len(commits) > 1:
     git_tool = 'diff-tree'
-  cmd = ['git', git_tool, '-p', '-U0'] + commits + ['--']
+  cmd = ['git', git_tool, '-p', '-U0']
+  if cached:
+    cmd.append('--cached')
+  cmd.extend(commits)
+  cmd.append('--')
   cmd.extend(files)
   p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
   p.stdin.close()
   return p
 
 
 def extract_lines(patch_file):
   """Extract the changed lines in `patch_file`.
def print_diff(old_tree, new_tree):
   #
   # We also only print modified files since `new_tree` only contains the files
   # that were modified, so unmodified files would show as deleted without the
   # filter.
   subprocess.check_call(['git', 'diff', '--diff-filter=M', old_tree, new_tree,
                          '--'])
 
 
-def apply_changes(old_tree, new_tree, force=False, patch_mode=False):
-  """Apply the changes in `new_tree` to the working directory.
+def apply_changes(old_tree, new_tree, force=False, patch_mode=False,
+                  cached=False):
+  """Apply the changes in `new_tree` to the working directory or index.
 
   Bails if there are local changes in those files and not `force`.  If
-  `patch_mode`, runs `git checkout --patch` to select hunks interactively."""
+  `patch_mode`, adds `--patch` option to select hunks interactively."""
   changed_files = run('git', 'diff-tree', '--diff-filter=M', '-r', '-z',
                       '--name-only', old_tree,
                       new_tree).rstrip('\0').split('\0')
+  if changed_files == ['']:
+      # Early exit if no files have changes.  Can occur when
+      # old_tree != new_tree because new_tree only contains formatted files.
+      return []
   if not force:
-    unstaged_files = run('git', 'diff-files', '--name-status', *changed_files)
-    if unstaged_files:
+    if cached:
+      localmod_files = run('git', 'diff-index', '--cached', '--name-status',
+                           'HEAD', '--', *changed_files)
+    else:
+      localmod_files = run('git', 'diff-files', '--name-status', *changed_files)
+    if localmod_files:
       print('The following files would be modified but '
-                'have unstaged changes:', file=sys.stderr)
-      print(unstaged_files, file=sys.stderr)
+                'have existing changes:', file=sys.stderr)
+      print(localmod_files, file=sys.stderr)
+      if cached:
+        print('Please commit or reset them first.', file=sys.stderr)
+      else:
         print('Please commit, stage, or stash them first.', file=sys.stderr)
       sys.exit(2)
+  if cached:
+    cmd = ['git', 'reset']
     if patch_mode:
+      cmd.append('--patch')
+    cmd.append(new_tree)
+    cmd.append('--')
+    cmd.extend(changed_files)
+    subprocess.check_call(cmd)
+  elif patch_mode:
     # In patch mode, we could just as well create an index from the new tree
     # and checkout from that, but then the user will be presented with a
     # message saying "Discard ... from worktree".  Instead, we use the old
     # tree as the index and checkout from new_tree, which gives the slightly
     # better message, "Apply ... to index and worktree".  This is not quite
     # right, since it won't be applied to the user's index, but oh well.
     with temporary_index_file(old_tree):
       subprocess.check_call(['git', 'checkout', '--patch', new_tree])