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

[ortogonal]

Summary

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: git-clang-format: Add new --staged option. which is a fork of D15465: [git-clang-format]: New option to perform formatting against staged changes only

This is tested with:

mkdir /tmp/gcfs
cd /tmp/gcfs

git init

cat <<EOF > foo.cc
int main() {
  int x =  1;
  return 0;
}
EOF

git add foo.cc
git commit -m 'first commit'
rm foo.cc

cat <<EOF > 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;
 }

[• alex1701c]

Any update on this topic? I really like this feature.

[ortogonal]

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.

[MyDeveloperDay]

FYI I doubt you'll get a response from the original clang-format code owners as they have moved on. But I can take a look

Before giving this the ok, I'd like to understand a little more.

if I make a (bad) formatting change in a file which is not already staged and run git clang-format it will say

$ git clang-format
The following files would be modified but have unstaged changes:
M       clang/lib/Format/Format.cpp
Please commit, stage, or stash them first.

If I make a formatting change to a file that is untracked in the current directory, then it will NOT format those files

I have to do git add Format.cpp for it to format my file

$ git clang-format
changed files:
    clang/lib/Format/Format.cpp

i.e. I seem to ALWAYS have to stage files in order to have git clang-format do anything

So I'm kind of unclear as to what this is doing and what --staged will do differently? (is it to simply ignore the The following files would be modified but have unstaged changes: warning?

Did I misunderstand?

[ortogonal]

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 <file>..." to unstage)
        modified:   main.cpp

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." 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?

[MyDeveloperDay]

Much, thank you for seeing this over the line,

This LGTM, lets wait a bit in case any of the others have a comment.

I assume you need help committing this? if so we need your name and your email address so we can commit it on your behalf

[MyDeveloperDay]

@lodato if you are still around (I see no changes since 2017), we may be able to add you as a "Co-author" I'm not sure of the policy, (again I would need your name and email address)

Co-authored-by: name <[e-mail]>

[ortogonal]

Thanks for all your help (superfast as well)

Yes I need help commiting.

My name: Erik Larsson
My email: [e-mail]

[MyDeveloperDay]

No problem, I hope your ok if @lodato gets back to us, I think we should probably try and credit them at least a little.

Like I said let us wait a bit for people to have a chance to take a look (I'm not in the US hence why I've seen it already),

I doubt there will be a problem as @klimek approved @lodato work (I'm not sure why it didn't get landed), but sometimes it just needs someone to be persistent, for that I thank you.

[lodato]

Thanks for pushing this through, @ortogonal! The change LGTM, except the two minor issues listed.

Name: Mark Lodato
Email: [e-mail]

clang/tools/clang-format/git-clang-format

140

Does there need to be an equivalent check that --staged requires --diff? Could you test to make sure that works as expected?

298–299

nit: probably more clear to say "working directory (or stage if staged)" or something like that.

[ortogonal]

Update description on function compute_diff

[ortogonal]

clang/tools/clang-format/git-clang-format

140

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!

[MyDeveloperDay]

@lodato are you blocking this review for a reason?

[lodato]

Oops, I'm sorry. I meant to test it but ran out of time and then forgot to click approve. My apologies.

[ortogonal]

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

clang/tools/clang-format/git-clang-format

 import subprocess
 import sys
 
-usage = 'git clang-format [OPTIONS] [<commit>] [<commit>] [--] [<file>...]'
+usage = ('git clang-format [OPTIONS] [<commit>] [<commit>|--staged] '
+         '[--] [<file>...]')
 
 desc = '''
 If zero or one commits are given, run clang-format on all lines that differ
 between the working directory and <commit>, 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 <commit> that differ from the first <commit>.

                  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'),

 
   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(','))

   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()

   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()