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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
