clang-format: Add an option to git-clang-format to diff between to commits

[lhchavez]

Summary

When building pre-upload hooks using git-clang-format, it is useful to limit the scope to a diff of two commits (instead of from a commit against the working tree) to allow for less false positives in dependent commits.

This change adds the option of specifying two git commits to git-clang-format when using the --diff flag, which uses a different strategy to diff (using git-diff-tree instead of git-diff-index), and runs clang-format against the second commit instead of the working directory.

There is a slight backwards-incompatibility introduced with this change: if a filename matches a branch name or other commit-ish, then git clang-format <commit> <file> will no longer work as expected; use git clang-format <commit> -- <file> instead.

[lodato]

Hi lhchavez,

This patch does not work as intended. If I understand correctly, you want to see if a given <commit> has any clang-format problems. However:

1. This patch only computes changed_lines from <commit> but then runs clang-format on the files in the working directory. Instead, you need to somehow update run_clang_format_and_save_to_tree() to operate on <commit>.

2. Unless --diff is given, this new mode writes clang-formatted contents of <commit> to the working directory, blowing away any changes since then. The simple solution is to require --diff whenever this new mode is used, since it's not obvious what should be written to the working directory.

See also D15465, which had similar problems.

Here is an example showing the problem. The last command should have said that aa207 was bad, but since the current working directory is fine, it shows no diff.

$ git log -p --reverse
commit a2765ba80f03f02aaa0cefbfe705ae844c219065
Author: ...
Date:   2016-09-12 16:29:53 -0400

    initial commit; x and y bad

diff --git a/foo.cc b/foo.cc
new file mode 100644
index 0000000..08422a0
--- /dev/null
+++ b/foo.cc
@@ -0,0 +1,4 @@
+void foo() {
+  int x=1;
+  int y=2;
+}

commit aa207f7991d26d1ee6826bf272f8eefffdf31b31
Author: ...
Date:   2016-09-12 16:30:19 -0400

    modify x, still bad

diff --git a/foo.cc b/foo.cc
index 08422a0..dd75f40 100644
--- a/foo.cc
+++ b/foo.cc
@@ -1,4 +1,4 @@
 void foo() {
-  int x=1;
+  int x=3;
   int y=2;
 }

commit 72858d5d2b13ed9dcf6a3328fef43a81ee3f01fc
Author: ...
Date:   2016-09-12 16:30:58 -0400

    modify both lines, still bad

diff --git a/foo.cc b/foo.cc
index dd75f40..3eff9ca 100644
--- a/foo.cc
+++ b/foo.cc
@@ -1,4 +1,4 @@
 void foo() {
-  int x=3;
-  int y=2;
+  int x=30;
+  int y=20;
 }

commit 9e6bfa1b9582b1423efc6d2339e269636fa0718b
Author: ...
Date:   2016-09-12 16:32:21 -0400

    clang-format

diff --git a/foo.cc b/foo.cc
index 3eff9ca..ac4d00d 100644
--- a/foo.cc
+++ b/foo.cc
@@ -1,4 +1,4 @@
 void foo() {
-  int x=30;
-  int y=20;
+  int x = 30;
+  int y = 20;
 }
$ ~/tmp/git-clang-format --single-commit aa207
clang-format did not modify any files

cfe/trunk/tools/clang-format/git-clang-format

93 ↗

(On Diff #70607)

nit: I find this flag confusing since it does not follow any git convention. Instead, I suggest making the interface similar to git diff: if two <commit>s are given, take the diff of those two trees to find the list of changed lines, then run clang-format on the second commit.

For example:

• git clang-format --diff HEAD HEAD~ would tell you if HEAD was properly clang-formatted or not.
• git clang-format --diff 8bf003 ae9172 would tell you if any of the lines in ae9172 that differ from 8bf003 are not properly clang-formatted.
  
  operate in this new mode only if two <commit>s are given. Then the interface would be, for example, git clang-format abcd1234 abcd1234~.

323 ↗

(On Diff #70607)

Unless I'm mistaken, this function is unnecessary. There is no need to filter out files in the tree that do not match the pattern, since the filtering happens in compute_diff() (cmd.extend(files)).

[lhchavez]

I'll post a no-op change with arcanist to fix the paths (I shouldn't have tried to manually upload the diff), and then another one to fix the patch so that the script actually does what it advertises and add the comment explaining the reason for create_tree_from_commit().

I'll tackle the interface change in the change following that one, after some more experimentation.

cfe/trunk/tools/clang-format/git-clang-format

93 ↗

(On Diff #70607)

The thing I liked about using git-diff-tree is that you can pass a single commit and even if it's a merge-commit, it does the right thing. I'll try that idea out and allow more than two commits and see how it behaves.

323 ↗

(On Diff #70607)

The reason I added this is to prevent the generation of much larger temporary indices for large projects (like Chromium) vs. git-clang-format in normal mode. I can add a comment explaining that.

[lhchavez]

No-op update. Using arcanist to fix the diff and the paths.

[lhchavez]

Fix the script so it does what it advertises.

[lhchavez]

Using lodato's proposed interface. This patch:

• Accepts an arbitrary number of commits as arguments. Validation will be done in main(), such that two commits are valid only when running in --diff mode.
• Allows diffing two commits against each other in addition to diffing against the working directory (still using git diff-tree). The previous version didn't really work with merge-commits anyways.
• Runs clang-format against the file in the working directory (if a single commit is passed), or against the second commit.

[lhchavez]

cfe/trunk/tools/clang-format/git-clang-format

93 ↗

(On Diff #70607)

Nevermind, git diff-tree didn't work with merge-commits if a single commit is passed. Changed to your proposal.

[lhchavez]

cfe/trunk/tools/clang-format/git-clang-format

323 ↗

(On Diff #70607)

Wait, I understand what you mean now. I can just extract the tree from the commit and use that as diff base. Changed it to a simpler git show command.

[lhchavez]

Got rid of create_tree_from_commit

[lhchavez]

Gentle ping. Is there anything else that needs addressing? Did I miss anything?

[lodato]

Sorry for the delay - I haven't had a chance to review. I'll be sure to review by tomorrow. Thanks for the updates!

[lodato]

Could you add a note to the commit description to say that there is a backwards incompatibility: if a filename matches a branch name or other commit-ish, then git clang-format <commit> <file> will no longer work as expected; use git clang-format <commit> -- <file> instead.

tools/clang-format/git-clang-format

35

add a second [<commit>]

38–43

suggestion:

If zero or one commits are given, <old text>.

If two commits are given (requires --diff), run clang-format on all lines in the second <commit> that differ from the first <commit>.

130

This error message is a bit confusing. I think it would be clearer to do:

if len(commits) > 1:
  if not opts.diff:
    die('--diff is required when two commits are given')
elif len(commits) > 2:
  die('at most two ...

201

nit: Python style is to put the whole thing on a single line. Maybe remove the two ...s or the [--]?

335

I don't think this function is necessary. All git commands that take a tree actually take a "tree-ish", which is a tree, commit, tag, or branch. I just tried removing this function and it appeared to work.

(If it ends up this function is necessary, I think the proper thing to do is git rev-parse <commit>^{tree} since git-show is a porcelain command.)

382

Runs on the file in `revision` if not None, or on the file in the working directory if `revision` is None.

394

Should use cat-file instead of show - the former is plumbing while the latter is porcelain (see man git). That is, git cat-file blob %s:%s.

463–468

Add --diff-filter=M. Without this, the command prints all unmodified files as deleted since old_tree has all the files in the case of multi-commit mode, but new_tree only has modified files. I suggest adding a comment explaining this.

To test, try running git clang-format --diff HEAD~30 HEAD on the LLVM repo. You'll see that without the diff filter, you get a 100MB+ diff! :)

476–477

Might as well add --diff-filter=M here too

[lhchavez]

Addressed lovato's comments

[lhchavez]

Addressed all comments. PTAAL.

[lodato]

Nice feature. Thanks for the patch!

tools/clang-format/git-clang-format

 import collections
 import contextlib
 import errno
 import os
 import re
 import subprocess
 import sys
 
-usage = 'git clang-format [OPTIONS] [<commit>] [--] [<file>...]'
+usage = 'git clang-format [OPTIONS] [<commit>] [<commit>] [--] [<file>...]'
 
 desc = '''
-Run clang-format on all lines that differ between the working directory
-and <commit>, which defaults to HEAD.  Changes are only applied to the working
-directory.
+If zero or one commits are given, run clang-format on all lines that differ
+between the working directory and <commit>, which defaults to HEAD.  Changes are
+only applied to the working directory.
+
+If two commits are given (requires --diff), run clang-format on all lines in the
+second <commit> that differ from the first <commit>.
 
 The following git-config settings set the default of the corresponding option:
   clangFormat.binary
   clangFormat.commit
   clangFormat.extension
   clangFormat.style
 '''
 
p.add_argument('args', nargs='*', metavar='<commit>',
                  help='revision from which to compute the diff')
   p.add_argument('ignored', nargs='*', metavar='<file>...',
                  help='if specified, only consider differences in these files')
   opts = p.parse_args(argv)
 
   opts.verbose -= opts.quiet
   del opts.quiet
 
-  commit, files = interpret_args(opts.args, dash_dash, opts.commit)
-  changed_lines = compute_diff_and_extract_lines(commit, files)
+  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')
+  else:
+    if len(commits) > 2:
+      die('at most two commits allowed; %d given' % len(commits))
+  changed_lines = compute_diff_and_extract_lines(commits, files)
   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 '   ', filename
     if changed_lines:
       print 'Running clang-format on the following files:'
       for filename in changed_lines:
         print '   ', 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)
+  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)
   if opts.verbose >= 1:
     print 'old tree:', old_tree
     print 'new tree:', 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)
if entry:
       name, value = entry.split('\n', 1)
       if name in non_string_options:
         value = run('git', 'config', non_string_options[name], name)
       out[name] = value
   return out
 
 
 def interpret_args(args, dash_dash, default_commit):
-  """Interpret `args` as "[commit] [--] [files...]" and return (commit, files).
+  """Interpret `args` as "[commits] [--] [files]" and return (commits, files).
 
   It is assumed that "--" and everything that follows has been removed from
   args and placed in `dash_dash`.
 
-  If "--" is present (i.e., `dash_dash` is non-empty), the argument to its
-  left (if present) is taken as commit.  Otherwise, the first argument is
-  checked if it is a commit or a file.  If commit is not given,
-  `default_commit` is used."""
+  If "--" is present (i.e., `dash_dash` is non-empty), the arguments to its
+  left (if present) are taken as commits.  Otherwise, the arguments are checked
+  from left to right if they are commits or files.  If commits are not given,
+  a list with `default_commit` is used."""
   if dash_dash:
     if len(args) == 0:
-      commit = default_commit
-    elif len(args) > 1:
-      die('at most one commit allowed; %d given' % len(args))
+      commits = [default_commit]
     else:
-      commit = args[0]
+      commits = args
+    for commit in commits:
       object_type = get_object_type(commit)
       if object_type not in ('commit', 'tag'):
         if object_type is None:
           die("'%s' is not a commit" % commit)
         else:
           die("'%s' is a %s, but a commit was expected" % (commit, object_type))
     files = dash_dash[1:]
   elif args:
-    if disambiguate_revision(args[0]):
-      commit = args[0]
-      files = args[1:]
-    else:
-      commit = default_commit
-      files = args
+    commits = []
+    while args:
+      if not disambiguate_revision(args[0]):
+        break
+      commits.append(args.pop(0))
+    if not commits:
+      commits = [default_commit]
   else:
-    commit = default_commit
+    commits = [default_commit]
     files = []
-  return commit, files
+  return commits, files
 
 
 def disambiguate_revision(value):
   """Returns True if `value` is a revision, False if it is a file, or dies."""
   # If `value` is ambiguous (neither a commit nor a file), the following
   # command will die with an appropriate error message.
   run('git', 'rev-parse', value, verbose=False)
   object_type = get_object_type(value)
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 stdout.strip()
 
 
-def compute_diff_and_extract_lines(commit, files):
+def compute_diff_and_extract_lines(commits, files):
   """Calls compute_diff() followed by extract_lines()."""
-  diff_process = compute_diff(commit, files)
+  diff_process = compute_diff(commits, files)
   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(commit, files):
-  """Return a subprocess object producing the diff from `commit`.
+def compute_diff(commits, files):
+  """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 `commit`, filtered on `files`
-  (if non-empty).  Zero context lines are used in the patch."""
-  cmd = ['git', 'diff-index', '-p', '-U0', commit, '--']
+  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.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 cd_to_toplevel():
   """Change to the top level of the git repository."""
   toplevel = run('git', 'rev-parse', '--show-toplevel')
   os.chdir(toplevel)
 
 
 def create_tree_from_workdir(filenames):
   """Create a new git tree with the given files from the working directory.
 
   Returns the object ID (SHA-1) of the created tree."""
   return create_tree(filenames, '--stdin')
 
 
-def run_clang_format_and_save_to_tree(changed_lines, binary='clang-format',
-                                      style=None):
+def run_clang_format_and_save_to_tree(changed_lines, revision=None,
+                                      binary='clang-format', style=None):
   """Run clang-format on each file and save the result to a git tree.
 
   Returns the object ID (SHA-1) of the created tree."""
   def index_info_generator():
     for filename, line_ranges in changed_lines.iteritems():
       mode = oct(os.stat(filename).st_mode)
-      blob_id = clang_format_to_blob(filename, line_ranges, binary=binary,
+      blob_id = clang_format_to_blob(filename, line_ranges,
+                                     revision=revision,
+                                     binary=binary,
                                      style=style)
       yield '%s %s\t%s' % (mode, blob_id, filename)
   return create_tree(index_info_generator(), '--index-info')
 
 
 def create_tree(input_lines, mode):
   """Create a tree object from the given input.
 
for line in input_lines:
       p.stdin.write('%s\0' % line)
     p.stdin.close()
     if p.wait() != 0:
       die('`%s` failed' % ' '.join(cmd))
     tree_id = run('git', 'write-tree')
     return tree_id
 
 
-def clang_format_to_blob(filename, line_ranges, binary='clang-format',
-                         style=None):
+def clang_format_to_blob(filename, line_ranges, revision=None,
+                         binary='clang-format', style=None):
   """Run clang-format on the given file and save the result to a git blob.
 
+  Runs on the file in `revision` if not None, or on the file in the working
+  directory if `revision` is None.
+
   Returns the object ID (SHA-1) of the created blob."""
-  clang_format_cmd = [binary, filename]
+  clang_format_cmd = [binary]
   if style:
     clang_format_cmd.extend(['-style='+style])
   clang_format_cmd.extend([
       '-lines=%s:%s' % (start_line, start_line+line_count-1)
       for start_line, line_count in line_ranges])
+  if revision:
+    clang_format_cmd.extend(['-assume-filename='+filename])
+    git_show_cmd = ['git', 'cat-file', 'blob', '%s:%s' % (revision, filename)]
+    git_show = subprocess.Popen(git_show_cmd, stdin=subprocess.PIPE,
+                                stdout=subprocess.PIPE)
+    git_show.stdin.close()
+    clang_format_stdin = git_show.stdout
+  else:
+    clang_format_cmd.extend([filename])
+    git_show = None
+    clang_format_stdin = subprocess.PIPE
   try:
-    clang_format = subprocess.Popen(clang_format_cmd, stdin=subprocess.PIPE,
+    clang_format = subprocess.Popen(clang_format_cmd, stdin=clang_format_stdin,
                                     stdout=subprocess.PIPE)
+    if clang_format_stdin == subprocess.PIPE:
+      clang_format_stdin = clang_format.stdin
   except OSError as e:
     if e.errno == errno.ENOENT:
       die('cannot find executable "%s"' % binary)
     else:
       raise
-  clang_format.stdin.close()
+  clang_format_stdin.close()
   hash_object_cmd = ['git', 'hash-object', '-w', '--path='+filename, '--stdin']
   hash_object = subprocess.Popen(hash_object_cmd, stdin=clang_format.stdout,
                                  stdout=subprocess.PIPE)
   clang_format.stdout.close()
   stdout = hash_object.communicate()[0]
   if hash_object.returncode != 0:
     die('`%s` failed' % ' '.join(hash_object_cmd))
   if clang_format.wait() != 0:
     die('`%s` failed' % ' '.join(clang_format_cmd))
+  if git_show and git_show.wait() != 0:
+    die('`%s` failed' % ' '.join(git_show_cmd))
   return stdout.rstrip('\r\n')
 
 
 @contextlib.contextmanager
 def temporary_index_file(tree=None):
   """Context manager for setting GIT_INDEX_FILE to a temporary file and deleting
   the file afterward."""
   index_path = create_temporary_index(tree)
def create_temporary_index(tree=None):
   return path
 
 
 def print_diff(old_tree, new_tree):
   """Print the diff between the two trees to stdout."""
   # We use the porcelain 'diff' and not plumbing 'diff-tree' because the output
   # is expected to be viewed by the user, and only the former does nice things
   # like color and pagination.
-  subprocess.check_call(['git', '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.
 
   Bails if there are local changes in those files and not `force`.  If
   `patch_mode`, runs `git checkout --patch` to select hunks interactively."""
-  changed_files = run('git', 'diff-tree', '-r', '-z', '--name-only', old_tree,
+  changed_files = run('git', 'diff-tree', '--diff-filter=M', '-r', '-z',
+                      '--name-only', old_tree,
                       new_tree).rstrip('\0').split('\0')
   if not force:
     unstaged_files = run('git', 'diff-files', '--name-status', *changed_files)
     if unstaged_files:
       print >>sys.stderr, ('The following files would be modified but '
                            'have unstaged changes:')
       print >>sys.stderr, unstaged_files
       print >>sys.stderr, 'Please commit, stage, or stash them first.'