Page MenuHomePhabricator

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

Authored by ortogonal on Nov 6 2020, 11:00 PM.

Details

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

Diff Detail

Unit TestsFailed

TimeTest
410 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

ortogonal created this revision.Nov 6 2020, 11:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2020, 11:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ortogonal requested review of this revision.Nov 6 2020, 11:00 PM

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

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 added a project: Restricted Project.
MyDeveloperDay added a comment.EditedOct 21 2021, 1:06 AM

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?

MyDeveloperDay edited the summary of this revision. (Show Details)Oct 21 2021, 1:07 AM

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 accepted this revision.Oct 21 2021, 2:31 AM

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

This revision is now accepted and ready to land.Oct 21 2021, 2:31 AM

@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 <name@example.com>

Thanks for all your help (superfast as well)

Yes I need help commiting.

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

MyDeveloperDay added a comment.EditedOct 21 2021, 2:39 AM

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 requested changes to this revision.Oct 21 2021, 6:41 AM

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

Name: Mark Lodato
Email: lodato@google.com

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?

299

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

This revision now requires changes to proceed.Oct 21 2021, 6:41 AM
ortogonal updated this revision to Diff 381269.Oct 21 2021, 8:05 AM
ortogonal marked an inline comment as done.

Update description on function compute_diff

ortogonal added inline comments.Oct 21 2021, 8:14 AM
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!

@lodato are you blocking this review for a reason?

lodato accepted this revision.Oct 26 2021, 8:18 AM

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

This revision is now accepted and ready to land.Oct 26 2021, 8:18 AM

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