[PATCH] D15465: [git-clang-format]: New option to perform formatting against staged changes only

[Mark Lodato via cfe-commits]

lodato added a subscriber: lodato.
lodato requested changes to this revision.
lodato added a reviewer: lodato.
lodato added a comment.
This revision now requires changes to proceed.

This does not work properly because it calls `clang-format` on the files in the working directory, even if `--staged` is given.  To fix, you need to somehow pass in the version of the files in the index into `clang-format`.  To do that, I think you'd want to pass in the blob via stdin and add `-assume-filename=...` to the command line.

Example:

  $ mkdir tmp
  $ cd tmp
  $ git init
  $ cat > foo.cc <<EOF
  int main() {
    int x = 1;
    return 0;
  }
  EOF
  $ git add foo.cc
  $ git commit -m 'initial commit'
  $ sed -i -e 's/1/2/g' foo.cc
  $ git add foo.cc
  $ rm foo.cc
  $ cat > foo.cc <<EOF
  int main() {
    printf("hello\n");
    printf("goodbye\n");
    return 0;
  }
  EOF

Now we have the situation where the stage and the working directory differ, but both are clang-format-compatible and should produce no changes.

  $ git diff --cached
  diff --git c/foo.cc i/foo.cc
  index cb7e0b0..0a5833c 100644
  --- c/foo.cc
  +++ i/foo.cc
  @@ -1,4 +1,4 @@
   int main() {
  -  int x = 1;
  +  int x = 2;
     return 0;
   }
  $ git diff
  diff --git i/foo.cc w/foo.cc
  index 0a5833c..b821f3e 100644
  --- i/foo.cc
  +++ w/foo.cc
  @@ -1,4 +1,5 @@
   int main() {
  -  int x = 2;
  +  printf("hello\n");
  +  printf("goodbye\n");
     return 0;
   }

However, your script produces the following, which is clearly incorrect:

  $ git-clang-format --staged --diff
  diff --git a/foo.cc b/foo.cc
  index 0a5833c..b821f3e 100644
  --- a/foo.cc
  +++ b/foo.cc
  @@ -1,4 +1,5 @@
   int main() {
  -  int x = 2;
  +  printf("hello\n");
  +  printf("goodbye\n");
     return 0;
   }


================
Comment at: git-clang-format:104
@@ -103,1 +103,3 @@
                  help='select hunks interactively')
+  p.add_argument('-s', '--staged', action='store_true',
+                 help='consider only staged lines')
----------------
Please move this down after `-q` so it stays sorted.

Also I would drop the `-s` since the this will be an uncommon option.

Finally, could you add `aliases=['cached']` to mirror the behavior of `git diff`?

================
Comment at: git-clang-format:124
@@ -121,3 +123,3 @@
   del opts.quiet
 
   commit, files = interpret_args(opts.args, dash_dash, opts.commit)
----------------
This will work without `--diff` (otherwise it will try to apply changes in the index to the working directory, which doesn't make sense), so could you please add a check that `--staged` requires `--diff`?

================
Comment at: git-clang-format:250
@@ -244,3 +249,3 @@
 
-def compute_diff_and_extract_lines(commit, files):
+def compute_diff_and_extract_lines(commit, files, cached=False):
   """Calls compute_diff() followed by extract_lines()."""
----------------
I suggest sticking with the name `staged` throughout the code.

================
Comment at: git-clang-format:328
@@ -319,1 +327,3 @@
 
+def run_git_ls_files_and_save_to_tree(changed_lines):
+  """Run git ls-files and save the result to a git tree.
----------------
There is already a git command that does this, `git write-tree`, so you can replace this entire function with a call to `run('git', 'write-tree')`.


Repository:
  rL LLVM

http://reviews.llvm.org/D15465