[PATCH] D24319: clang-format: Add an option to git-clang-format to diff between to commits

[Mark Lodato via cfe-commits]

lodato added a comment.

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.


================
Comment at: tools/clang-format/git-clang-format:35
@@ -34,3 +34,3 @@
 
 usage = 'git clang-format [OPTIONS] [<commit>] [--] [<file>...]'
 
----------------
add a second `[<commit>]`

================
Comment at: tools/clang-format/git-clang-format:38
@@ -37,4 +37,3 @@
 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.
+Run clang-format on all lines that differ between the working directory and
+<commit> (which defaults to HEAD), or all lines that changed in a specific
----------------
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>.


================
Comment at: tools/clang-format/git-clang-format:127
@@ +126,3 @@
+    if len(commits) > 1:
+      die('at most one commit allowed; %d given' % len(commits))
+  else:
----------------
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 ...
```

================
Comment at: tools/clang-format/git-clang-format:198
@@ -184,2 +197,3 @@
 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).
----------------
nit: Python style is to put the whole thing on a single line.  Maybe remove the two `...`s or the `[--]`?

================
Comment at: tools/clang-format/git-clang-format:333
@@ -313,1 +332,3 @@
 
+def get_tree_from_commit(commit):
+  """Returns the object ID (SHA-1) of the tree associated with `commit`"""
----------------
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.)

================
Comment at: tools/clang-format/git-clang-format:385
@@ -358,1 +384,3 @@
 
+  If `revision` is empty, clang-format will be run on the file in the working
+  directory.
----------------
```
Runs on the file in `revision` if not None, or on the file in the working directory if `revision` is None.
```

================
Comment at: tools/clang-format/git-clang-format:397
@@ +396,3 @@
+    clang_format_cmd.extend(['-assume-filename='+filename])
+    git_show_cmd = ['git', 'show', '%s:%s' % (revision, filename)]
+    git_show = subprocess.Popen(git_show_cmd, stdin=subprocess.PIPE,
----------------
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`.

================
Comment at: tools/clang-format/git-clang-format:466
@@ -421,3 +465,3 @@
   # like color and pagination.
   subprocess.check_call(['git', 'diff', old_tree, new_tree, '--'])
 
----------------
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! :)

================
Comment at: tools/clang-format/git-clang-format:474
@@ -429,3 +473,3 @@
   `patch_mode`, runs `git checkout --patch` to select hunks interactively."""
   changed_files = run('git', 'diff-tree', '-r', '-z', '--name-only', old_tree,
                       new_tree).rstrip('\0').split('\0')
----------------
Might as well add `--diff-filter=M` here too


https://reviews.llvm.org/D24319