[PATCH] D90996: [clang-format] Add --staged/--cached option to git-clang-format

[Erik Larsson via Phabricator via cfe-commits]

ortogonal created this revision.
ortogonal added reviewers: klimek, Alexander-Shukaev, djasper.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
ortogonal requested review of this revision.

When running git-clang-format in a pre-commit hook it's very useful to be able 
to tell git-clang-format to only look at the --staged/--cached files and not 
the working directory.

Note this patch is a rebase/fork from D41147  
which is a fork of D15465 

This is tested with:

  mkdir /tmp/gcfs
  cd /tmp/gcfs
  
  git init
  
  cat < foo.cc
  int main() {
int x =  1;
return 0;
  }
  EOF
  
  git add foo.cc
  git commit -m 'first commit'
  rm foo.cc
  
  cat < foo.cc
  int main() {
int x =  1;
int y =  2;
int z =  3;
return 0;
  }
  EOF
  
  git add foo.cc
  git commit -m 'second commit'
  sed -i -e 's/1;/10;/' foo.cc
  git add foo.cc
  sed -i -e 's/10;/1;/' foo.cc
  sed -i -e 's/3;/30;/' foo.cc
  
  
  $ git-clang-format --diff
  diff --git a/foo.cc b/foo.cc
  index cb2dbc9..2e1b0e1 100644
  --- a/foo.cc
  +++ b/foo.cc
  @@ -1,6 +1,6 @@
   int main() {
 int x =  1;
 int y =  2;
  -  int z =  30;
  +  int z = 30;
 return 0;
   }
  
  
  $ git-clang-format --diff --staged
  diff --git a/foo.cc b/foo.cc
  index 3ea8c6c..7da0033 100644
  --- a/foo.cc
  +++ b/foo.cc
  @@ -1,5 +1,5 @@
   int main() {
  -  int x =  10;
  +  int x = 10;
 int y =  2;
 int z =  3;
 return 0;
  
  
  $ git-clang-format --diff HEAD~ HEAD
  diff --git a/foo.cc b/foo.cc
  index 7bfdb83..ce6f65e 100644
  --- a/foo.cc
  +++ b/foo.cc
  @@ -1,6 +1,6 @@
   int main() {
 int x =  1;
  -  int y =  2;
  -  int z =  3;
  +  int y = 2;
  +  int z = 3;
 return 0;
   }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90996

Files:
  clang/tools/clang-format/git-clang-format


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -32,12 +32,13 @@
 import subprocess
 import sys
 
-usage = 'git clang-format [OPTIONS] [] [] [--] [...]'
+usage = ('git clang-format [OPTIONS] [] [|--staged] '
+ '[--] [...]')
 
 desc = '''
 If zero or one commits are given, 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.
+only applied to the working directory, or in the stage/index.
 
 If two commits are given (requires --diff), run clang-format on all lines in 
the
 second  that differ from the first .
@@ -109,6 +110,8 @@
  help='select hunks interactively')
   p.add_argument('-q', '--quiet', action='count', default=0,
  help='print less information')
+  p.add_argument('--staged', '--cached', action='store_true',
+ help='format lines in the stage instead of the working dir')
   p.add_argument('--style',
  default=config.get('clangformat.style', None),
  help='passed to clang-format'),
@@ -128,12 +131,14 @@
 
   commits, files = interpret_args(opts.args, dash_dash, opts.commit)
   if len(commits) > 1:
+if opts.staged:
+  die('--staged is not allowed when two commits are given')
 if not opts.diff:
   die('--diff is required when two commits are given')
   else:
 if len(commits) > 2:
   die('at most two commits allowed; %d given' % len(commits))
-  changed_lines = compute_diff_and_extract_lines(commits, files)
+  changed_lines = compute_diff_and_extract_lines(commits, files, opts.staged)
   if opts.verbose >= 1:
 ignored_files = set(changed_lines)
   filter_by_extension(changed_lines, opts.extensions.lower().split(','))
@@ -268,9 +273,9 @@
   return convert_string(stdout.strip())
 
 
-def compute_diff_and_extract_lines(commits, files):
+def compute_diff_and_extract_lines(commits, files, staged):
   """Calls compute_diff() followed by extract_lines()."""
-  diff_process = compute_diff(commits, files)
+  diff_process = compute_diff(commits, files, staged)
   changed_lines = extract_lines(diff_process.stdout)
   diff_process.stdout.close()
   diff_process.wait()
@@ -280,17 +285,20 @@
   return changed_lines
 
 
-def compute_diff(commits, files):
+def compute_diff(commits, files, staged):
   """Return a subprocess object producing the diff from `commits`.
 
   The return value's `stdin` file object will produce a patch with the
-  differences between the working directory and the first commit if a single
+  differences between the working/cached directory and the first commit if a 
single
   one was specified, or the difference between both specified commits, filtered
   on `files` (if non-empty).  Zero context lines are used in the patch."""
   git_tool = 'diff-index'
+  extra_args = []
   if len(commits) > 1:
 git_tool = 'diff-tree'
-  cmd = ['git', 

[PATCH] D90996: [clang-format] Add --staged/--cached option to git-clang-format

[Erik Larsson via Phabricator via cfe-commits]

ortogonal added a comment.

@MyDeveloperDay Is everything okay with this? Can you help out with the next 
step in the process?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90996/new/

https://reviews.llvm.org/D90996

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

[PATCH] D90996: [clang-format] Add --staged/--cached option to git-clang-format

[Erik Larsson via Phabricator via cfe-commits]

ortogonal updated this revision to Diff 381150.
ortogonal added a comment.

Just refactor it to latest main. I hope this will re-trigger a new build so I 
can investigate if there are still tests that fail.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90996/new/

https://reviews.llvm.org/D90996

Files:
  clang/tools/clang-format/git-clang-format


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -32,12 +32,13 @@
 import subprocess
 import sys
 
-usage = 'git clang-format [OPTIONS] [] [] [--] [...]'
+usage = ('git clang-format [OPTIONS] [] [|--staged] '
+ '[--] [...]')
 
 desc = '''
 If zero or one commits are given, 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.
+only applied to the working directory, or in the stage/index.
 
 If two commits are given (requires --diff), run clang-format on all lines in 
the
 second  that differ from the first .
@@ -112,6 +113,8 @@
  help='select hunks interactively')
   p.add_argument('-q', '--quiet', action='count', default=0,
  help='print less information')
+  p.add_argument('--staged', '--cached', action='store_true',
+ help='format lines in the stage instead of the working dir')
   p.add_argument('--style',
  default=config.get('clangformat.style', None),
  help='passed to clang-format'),
@@ -131,12 +134,14 @@
 
   commits, files = interpret_args(opts.args, dash_dash, opts.commit)
   if len(commits) > 1:
+if opts.staged:
+  die('--staged is not allowed when two commits are given')
 if not opts.diff:
   die('--diff is required when two commits are given')
   else:
 if len(commits) > 2:
   die('at most two commits allowed; %d given' % len(commits))
-  changed_lines = compute_diff_and_extract_lines(commits, files)
+  changed_lines = compute_diff_and_extract_lines(commits, files, opts.staged)
   if opts.verbose >= 1:
 ignored_files = set(changed_lines)
   filter_by_extension(changed_lines, opts.extensions.lower().split(','))
@@ -275,9 +280,9 @@
   return convert_string(stdout.strip())
 
 
-def compute_diff_and_extract_lines(commits, files):
+def compute_diff_and_extract_lines(commits, files, staged):
   """Calls compute_diff() followed by extract_lines()."""
-  diff_process = compute_diff(commits, files)
+  diff_process = compute_diff(commits, files, staged)
   changed_lines = extract_lines(diff_process.stdout)
   diff_process.stdout.close()
   diff_process.wait()
@@ -287,17 +292,20 @@
   return changed_lines
 
 
-def compute_diff(commits, files):
+def compute_diff(commits, files, staged):
   """Return a subprocess object producing the diff from `commits`.
 
   The return value's `stdin` file object will produce a patch with the
-  differences between the working directory and the first commit if a single
+  differences between the working/cached directory and the first commit if a 
single
   one was specified, or the difference between both specified commits, filtered
   on `files` (if non-empty).  Zero context lines are used in the patch."""
   git_tool = 'diff-index'
+  extra_args = []
   if len(commits) > 1:
 git_tool = 'diff-tree'
-  cmd = ['git', git_tool, '-p', '-U0'] + commits + ['--']
+  elif staged:
+extra_args += ['--cached']
+  cmd = ['git', git_tool, '-p', '-U0'] + extra_args + commits + ['--']
   cmd.extend(files)
   p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
   p.stdin.close()


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -32,12 +32,13 @@
 import subprocess
 import sys
 
-usage = 'git clang-format [OPTIONS] [] [] [--] [...]'
+usage = ('git clang-format [OPTIONS] [] [|--staged] '
+ '[--] [...]')
 
 desc = '''
 If zero or one commits are given, 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.
+only applied to the working directory, or in the stage/index.
 
 If two commits are given (requires --diff), run clang-format on all lines in the
 second  that differ from the first .
@@ -112,6 +113,8 @@
  help='select hunks interactively')
   p.add_argument('-q', '--quiet', action='count', default=0,
  help='print less information')
+  p.add_argument('--staged', '--cached', action='store_true',
+ help='format lines in the stage instead of the working dir')
   p.add_argument('--style',
  default=config.get('clangformat.style', None),
  help='pass

[PATCH] D90996: [clang-format] Add --staged/--cached option to git-clang-format

[Erik Larsson via Phabricator via cfe-commits]

ortogonal added a comment.

Thanks for looking into this!

The feature `--staged` adds the possibility to ONLY run clang format on what is 
staged.

Lets say you have main.cpp where you have done two changes. You stage one of 
them using:

  $ git add -p main.cpp

You now have a state were one of the changes is staged for commit but not the 
other.

  $ git status
  On branch master
  Changes to be committed:
(use "git restore --staged ..." to unstage)
  modified:   main.cpp
  
  Changes not staged for commit:
(use "git add ..." to update what will be committed)
(use "git restore ..." to discard changes in working directory)
  modified:   main.cpp

If you now run `git-clang-format` it will format all changes in main.cpp, but 
at this stage adding `--staged` to git-clang-format lets you only format what 
is staged in that file.

I use this together with a git pre-commit-hook to make sure that all my code 
changes are formatted correctly before I commit. But without the `--staged` 
option I end up in situations were my pre-commit-hook fails because I have part 
of a files staged and a formatting error in the part of the file that is not 
staged.

Does this make the need for `--staged` clearer?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90996/new/

https://reviews.llvm.org/D90996

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

[PATCH] D90996: [clang-format] Add --staged/--cached option to git-clang-format

[Erik Larsson via Phabricator via cfe-commits]

ortogonal added a comment.

Thanks for all your help (superfast as well)

Yes I need help commiting.

My name: Erik Larsson
My email: karl.erik.lars...@gmail.com


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90996/new/

https://reviews.llvm.org/D90996

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

[PATCH] D90996: [clang-format] Add --staged/--cached option to git-clang-format

[Erik Larsson via Phabricator via cfe-commits]

ortogonal updated this revision to Diff 381269.
ortogonal added a comment.

Update description on function `compute_diff`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90996/new/

https://reviews.llvm.org/D90996

Files:
  clang/tools/clang-format/git-clang-format


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -32,12 +32,13 @@
 import subprocess
 import sys
 
-usage = 'git clang-format [OPTIONS] [] [] [--] [...]'
+usage = ('git clang-format [OPTIONS] [] [|--staged] '
+ '[--] [...]')
 
 desc = '''
 If zero or one commits are given, 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.
+only applied to the working directory, or in the stage/index.
 
 If two commits are given (requires --diff), run clang-format on all lines in 
the
 second  that differ from the first .
@@ -112,6 +113,8 @@
  help='select hunks interactively')
   p.add_argument('-q', '--quiet', action='count', default=0,
  help='print less information')
+  p.add_argument('--staged', '--cached', action='store_true',
+ help='format lines in the stage instead of the working dir')
   p.add_argument('--style',
  default=config.get('clangformat.style', None),
  help='passed to clang-format'),
@@ -131,12 +134,14 @@
 
   commits, files = interpret_args(opts.args, dash_dash, opts.commit)
   if len(commits) > 1:
+if opts.staged:
+  die('--staged is not allowed when two commits are given')
 if not opts.diff:
   die('--diff is required when two commits are given')
   else:
 if len(commits) > 2:
   die('at most two commits allowed; %d given' % len(commits))
-  changed_lines = compute_diff_and_extract_lines(commits, files)
+  changed_lines = compute_diff_and_extract_lines(commits, files, opts.staged)
   if opts.verbose >= 1:
 ignored_files = set(changed_lines)
   filter_by_extension(changed_lines, opts.extensions.lower().split(','))
@@ -275,9 +280,9 @@
   return convert_string(stdout.strip())
 
 
-def compute_diff_and_extract_lines(commits, files):
+def compute_diff_and_extract_lines(commits, files, staged):
   """Calls compute_diff() followed by extract_lines()."""
-  diff_process = compute_diff(commits, files)
+  diff_process = compute_diff(commits, files, staged)
   changed_lines = extract_lines(diff_process.stdout)
   diff_process.stdout.close()
   diff_process.wait()
@@ -287,17 +292,21 @@
   return changed_lines
 
 
-def compute_diff(commits, files):
+def compute_diff(commits, files, staged):
   """Return a subprocess object producing the diff from `commits`.
 
   The return value's `stdin` file object will produce a patch with the
-  differences between the working directory and the first commit if a single
-  one was specified, or the difference between both specified commits, filtered
-  on `files` (if non-empty).  Zero context lines are used in the patch."""
+  differences between the working directory (or stage if --staged is used) and
+  the first commit if a single one was specified, or the difference between
+  both specified commits, filtered on `files` (if non-empty).
+  Zero context lines are used in the patch."""
   git_tool = 'diff-index'
+  extra_args = []
   if len(commits) > 1:
 git_tool = 'diff-tree'
-  cmd = ['git', git_tool, '-p', '-U0'] + commits + ['--']
+  elif staged:
+extra_args += ['--cached']
+  cmd = ['git', git_tool, '-p', '-U0'] + extra_args + commits + ['--']
   cmd.extend(files)
   p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
   p.stdin.close()


Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -32,12 +32,13 @@
 import subprocess
 import sys
 
-usage = 'git clang-format [OPTIONS] [] [] [--] [...]'
+usage = ('git clang-format [OPTIONS] [] [|--staged] '
+ '[--] [...]')
 
 desc = '''
 If zero or one commits are given, 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.
+only applied to the working directory, or in the stage/index.
 
 If two commits are given (requires --diff), run clang-format on all lines in the
 second  that differ from the first .
@@ -112,6 +113,8 @@
  help='select hunks interactively')
   p.add_argument('-q', '--quiet', action='count', default=0,
  help='print less information')
+  p.add_argument('--staged', '--cached', action='store_true',
+ help='format lines in the stage instead of the working dir')
   p.add_argument('--styl

[PATCH] D90996: [clang-format] Add --staged/--cached option to git-clang-format

[Erik Larsson via Phabricator via cfe-commits]

ortogonal added inline comments.



Comment at: clang/tools/clang-format/git-clang-format:140
 if not opts.diff:
   die('--diff is required when two commits are given')
   else:

lodato wrote:
> Does there need to be an equivalent check that --staged requires --diff? 
> Could you test to make sure that works as expected?
Sorry, new to this system. I wrote a reply to this, but it seemed to get lost 
when uploading. Let me try again :)

The check that you can't use `--staged` with two commits is enough. For example 
it you do:
```
git clang-format --diff --staged 53f64aa089c5fd335b1337cab7eaa99d072a25fc 
273e318e1ff0fae58ec7ed248ee3c9c73da8c00e
error: --staged is not allowed when two commits are given
```

But you are allowed to run just `git clang-format --staged`. It you do:
```
$ git add foo.cc
$ git clang-format --staged
changed files:
foo.cc
```
This will run clang-format on the staged changes on foo.cc.

It this what you meant or did I misunderstand you.

Thanks for your review!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90996/new/

https://reviews.llvm.org/D90996

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