[PATCH] D24319: clang-format: Add a flag to limit git-clang-format's diff to a single commit

[Mark Lodato via cfe-commits]

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