lodato added a subscriber: lodato. lodato added a comment. 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 <https://reviews.llvm.org/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 ================ Comment at: cfe/trunk/tools/clang-format/git-clang-format:93 @@ -92,1 +92,3 @@ help='default commit to use if none is specified'), + p.add_argument('--single-commit', action='store_true', + help=('run clang-format on a single commit instead of against ' ---------------- 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~`. ================ Comment at: cfe/trunk/tools/clang-format/git-clang-format:323 @@ -312,1 +322,3 @@ +def create_tree_from_commit(commit, filenames): + """Create a new git tree with the given files from `commit`. ---------------- 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)`). Repository: rL LLVM https://reviews.llvm.org/D24319 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
