[PATCH] D26287: clang-format: Use git-ls-tree to get file mode in diff mode

[Mark Lodato via cfe-commits]

lodato accepted this revision.
lodato added a comment.
This revision is now accepted and ready to land.

Thanks for the fix!


https://reviews.llvm.org/D26287



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mark Lodato via cfe-commits]

lodato accepted this revision.
lodato added a reviewer: lodato.
lodato added a comment.
This revision is now accepted and ready to land.

Nice feature. Thanks for the patch!


https://reviews.llvm.org/D24319



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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  ` will no longer work as expected; use `git 
clang-format  -- ` instead.



Comment at: tools/clang-format/git-clang-format:35
@@ -34,3 +34,3 @@
 
 usage = 'git clang-format [OPTIONS] [] [--] [...]'
 

add a second `[]`


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 , 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
+ (which defaults to HEAD), or all lines that changed in a specific

suggestion:

> If zero or one commits are given, .
> 
> If two commits are given (requires --diff), run clang-format on all lines in 
> the second  that differ from the first .



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 ^{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

Re: [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.

Sorry for the delay - I haven't had a chance to review. I'll be sure to review 
by tomorrow. Thanks for the updates!


https://reviews.llvm.org/D24319



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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 `` has any clang-format problems. However:

1. This patch only computes `changed_lines` from `` 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 ``.

2. Unless `--diff` is given, this new mode writes clang-formatted contents of 
`` 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 , 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 000..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 
``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 ``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

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

[Mark Lodato via cfe-commits]

lodato added inline comments.


Comment at: git-clang-format:124
@@ -121,3 +123,3 @@
   del opts.quiet
 
   commit, files = interpret_args(opts.args, dash_dash, opts.commit)

lodato wrote:
> 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`?
Oops, will //not// work.


Repository:
  rL LLVM

http://reviews.llvm.org/D15465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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 < foo.cc