Use git-clang-format as Arcanist linter

[starsid]

Summary

D49116 was using clang-format-diff because at the time of its writing,
it needed to handle the subversion repo as well.

[mehdi_amini]

Thanks for the quick follow-up! I patched it locally and it seems to work.

(Let me know if you need me to land this)

[starsid]

In D77363#1958855, @mehdi_amini wrote:

(Let me know if you need me to land this)

Thank you! Please land this. I don't have write permissions to the repo.

[vitalybuka]

utils/arcanist/clang-format.sh

44

https://github.com/llvm/llvm-project/commit/1aee1ae5326cf1f6d03b4b7adbdcc2b499f044fc

[vitalybuka]

This does not work for me

arc patch D77374 # Applies two patches D77373 D77374
arc diff --trace # fails

# It works with just one D77373 in the branch, but fails with both patches.
# .git/index.lock does not exist before and after "arc diff"

 ARGV  /usr/local/google/home/vitalybuka/src/clang.git/arc/arcanist/bin/arc diff --trace
 ARGV  /usr/local/google/home/vitalybuka/src/clang.git/arc/arcanist/scripts/arcanist.php diff --trace
 LOAD  Loaded "arcanist" from "/usr/local/google/home/vitalybuka/src/clang.git/arc/arcanist/src".
Config: Reading user configuration file "/usr/local/google/home/vitalybuka/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "/usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/.arcconfig".
Working Copy: Path "/usr/local/google/home/vitalybuka/src/llvm.git/llvm-project" is part of `git` working copy "/usr/local/google/home/vitalybuka/src/llvm.git/llvm-project".
Working Copy: Project root is at "/usr/local/google/home/vitalybuka/src/llvm.git/llvm-project".
Config: Did not find local configuration at "/usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/.git/arc/config".
>>> [1] (+0) <http> https://reviews.llvm.org/api/user.whoami
<<< [1] (+489) <http> 489,887 us
>>> [2] (+490) <conduit> user.whoami() <bytes = 117>
<<< [2] (+490) <conduit> 89 us
>>> [3] (+494) <exec> $ git --version
<<< [3] (+503) <exec> 9,371 us
>>> [4] (+503) <exec> $ git status --porcelain=2 -z
<<< [4] (+813) <exec> 309,811 us
>>> [5] (+814) <event> diff.didCollectChanges <listeners = 0>
<<< [5] (+814) <event> 85 us
>>> [6] (+815) <exec> $ git rev-parse --verify HEAD^
<<< [6] (+826) <exec> 11,203 us
>>> [7] (+827) <exec> $ git cat-file -t 'HEAD^'
<<< [7] (+837) <exec> 10,037 us
>>> [8] (+837) <exec> $ git rev-parse HEAD
<<< [8] (+848) <exec> 10,249 us
>>> [9] (+848) <exec> $ git log --first-parent --format=medium 'HEAD^'..bd0884a4955e8ffea1aa0c63e6145c9eedd22286
<<< [9] (+859) <exec> 11,240 us
>>> [10] (+863) <http> https://reviews.llvm.org/api/differential.query
<<< [10] (+1,123) <http> 259,589 us
>>> [11] (+1,123) <conduit> differential.query() <bytes = 239>
<<< [11] (+1,123) <conduit> 67 us
>>> [12] (+1,123) <http> https://reviews.llvm.org/api/differential.query
<<< [12] (+1,381) <http> 257,272 us
>>> [13] (+1,381) <conduit> differential.query() <bytes = 149>
<<< [13] (+1,381) <conduit> 70 us
>>> [14] (+1,381) <http> https://reviews.llvm.org/api/differential.getcommitmessage
<<< [14] (+1,655) <http> 274,502 us
>>> [15] (+1,656) <conduit> differential.getcommitmessage() <bytes = 172>
<<< [15] (+1,656) <conduit> 66 us
>>> [16] (+1,656) <http> https://reviews.llvm.org/api/differential.parsecommitmessage
<<< [16] (+1,914) <http> 258,117 us
>>> [17] (+1,914) <conduit> differential.parsecommitmessage() <bytes = 441>
<<< [17] (+1,914) <conduit> 67 us
>>> [18] (+1,914) <event> diff.didBuildMessage <listeners = 0>
<<< [18] (+1,914) <event> 60 us
>>> [19] (+1,915) <exec> $ git rev-parse --git-dir
<<< [19] (+1,926) <exec> 11,059 us
Linting...
>>> [20] (+5,540) <exec> $ git rev-parse HEAD
<<< [20] (+5,551) <exec> 11,047 us
>>> [21] (+5,551) <exec> $ git merge-base 'HEAD^' bd0884a4955e8ffea1aa0c63e6145c9eedd22286
<<< [21] (+5,562) <exec> 10,637 us
>>> [22] (+5,562) <exec> $ git diff --no-ext-diff --no-textconv --submodule=short --raw 7aadfc1a190cdfe32a9b04fc55cfc25164a903d2 HEAD --
<<< [22] (+5,575) <exec> 13,128 us
>>> [23] (+5,576) <exec> $ git -c 'diff.suppressBlankEmpty=false' diff --no-ext-diff --no-textconv --submodule=short --no-color --src-prefix=a/ --dst-prefix=b/ -U32767 -M -C 7aadfc1a190cdfe32a9b04fc55cfc25164a903d2 --
<<< [23] (+5,661) <exec> 85,034 us
Examining paths for linter 'clang-format'.
Found 2 matching paths for linter 'clang-format'.
>>> [24] (+5,700) <lint> Script and Regex <paths = 2>
>>> [25] (+5,700) <exec> $ utils/arcanist/clang-format.sh clang/lib/CodeGen/CGExpr.cpp
>>> [26] (+5,701) <exec> $ utils/arcanist/clang-format.sh clang/test/CodeGen/bounds-checking.cpp
<<< [26] (+6,544) <exec> 842,917 us
<<< [24] (+6,718) <lint> 1,018,294 us

[2020-04-03 08:53:16] EXCEPTION: (PhutilAggregateException) Some linters failed:
    - CommandException: Command failed with error #2!
      COMMAND
      utils/arcanist/clang-format.sh clang/test/CodeGen/bounds-checking.cpp
      
      STDOUT
      (empty)
      
      STDERR
      `git read-tree --index-output=.git/clang-format-index --empty` returned 128
      fatal: Unable to create '/usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/.git/index.lock': File exists.
      
      Another git process seems to be running in this repository, e.g.
      an editor opened by 'git commit'. Please make sure all processes
      are terminated then try again. If it still fails, a git process
      may have crashed in this repository earlier:
      remove the file manually to continue.
       at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:264]
arcanist(head=master, ref.master=368aec16a1ee)
  #0 <#2> ExecFuture::resolvex() called at [<arcanist>/src/lint/linter/ArcanistScriptAndRegexLinter.php:195]
  #1 <#2> ArcanistScriptAndRegexLinter::willLintPaths(array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:575]
  #2 <#2> ArcanistLintEngine::executeLinterOnPaths(ArcanistScriptAndRegexLinter, array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:536]
  #3 <#2> ArcanistLintEngine::executeLintersOnChunk(array, array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:474]
  #4 <#2> ArcanistLintEngine::executeLinters(array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:206]
  #5 ArcanistLintEngine::run() called at [<arcanist>/src/workflow/ArcanistLintWorkflow.php:219]
  #6 ArcanistLintWorkflow::run() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1277]
  #7 ArcanistDiffWorkflow::runLint() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1239]
  #8 ArcanistDiffWorkflow::runLintUnit() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:441]
  #9 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:411]

[fhahn]

In D77363#1959149, @vitalybuka wrote:

This does not work for me

+1

I am seeing a similar error when trying to upload a patch during an interactive rebase. The lock file does not exist before or after running arc.

Linting...
>>> [20] (+7,528) <exec> $ git rev-parse 'HEAD'
<<< [20] (+7,537) <exec> 8,955 us
>>> [21] (+7,537) <exec> $ git merge-base '9b56cc9361a471a3ce57da4c98f60e377cc43026' '64e1c0d3bb54e39b13fc902d5fa0bdd78c5bf458'
<<< [21] (+7,551) <exec> 13,073 us
>>> [22] (+7,551) <exec> $ git diff --no-ext-diff --no-textconv --submodule=short --raw '9b56cc9361a471a3ce57da4c98f60e377cc43026' HEAD --
<<< [22] (+7,567) <exec> 15,900 us
>>> [23] (+7,567) <exec> $ git -c 'diff.suppressBlankEmpty=false' diff --no-ext-diff --no-textconv --submodule=short --no-color --src-prefix=a/ --dst-prefix=b/ -U32767 -M -C '9b56cc9361a471a3ce57da4c98f60e377cc43026' --
<<< [23] (+8,230) <exec> 662,852 us
Examining paths for linter 'clang-format'.
Found 3 matching paths for linter 'clang-format'.
>>> [24] (+8,303) <lint> Script and Regex <paths = 3>
>>> [25] (+8,303) <exec> $ utils/arcanist/clang-format.sh 'clang/include/clang/AST/Type.h'
>>> [26] (+8,304) <exec> $ utils/arcanist/clang-format.sh 'clang/lib/Sema/SemaType.cpp'
>>> [27] (+8,305) <exec> $ utils/arcanist/clang-format.sh 'clang/test/SemaCXX/vector.cpp'
<<< [25] (+15,132) <exec> 6,829,495 us
<<< [27] (+16,807) <exec> 8,502,181 us
<<< [24] (+16,807) <lint> 8,504,390 us

[2020-04-03 10:15:48] EXCEPTION: (PhutilAggregateException) Some linters failed:
    - CommandException: Command failed with error #2!
      COMMAND
      utils/arcanist/clang-format.sh 'clang/include/clang/AST/Type.h'

      STDOUT
      (empty)

      STDERR
      fatal: Unable to create 'projects/llvm-project/.git/clang-format-index.lock': File exists.

      Another git process seems to be running in this repository, e.g.
      an editor opened by 'git commit'. Please make sure all processes
      are terminated then try again. If it still fails, a git process
      may have crashed in this repository earlier:
      remove the file manually to continue.
      error: `git update-index --add -z --stdin` failed
       at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:274]
arcanist(head=master, ref.master=cc850163f30c), phutil(head=master, ref.master=39ed96cd818a)
  #0 <#2> ExecFuture::resolvex() called at [<arcanist>/src/lint/linter/ArcanistScriptAndRegexLinter.php:195]
  #1 <#2> ArcanistScriptAndRegexLinter::willLintPaths(array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:585]
  #2 <#2> ArcanistLintEngine::executeLinterOnPaths(ArcanistScriptAndRegexLinter, array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:546]
  #3 <#2> ArcanistLintEngine::executeLintersOnChunk(array, array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:484]
  #4 <#2> ArcanistLintEngine::executeLinters(array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:216]
  #5 ArcanistLintEngine::run() called at [<arcanist>/src/workflow/ArcanistLintWorkflow.php:337]
  #6 ArcanistLintWorkflow::run() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1250]
  #7 ArcanistDiffWorkflow::runLint() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1211]
  #8 ArcanistDiffWorkflow::runLintUnit() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:483]
  #9 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]

[kadircet]

Reverted in 3e1b8db309375551f4883f5068ec3536e1812b95 as it was breaking arc diff workflow for multiple people.

[scott.linder]

I missed a bit of the discussion in the previous review, but why would we want/need to use the git-clang-format wrapper? We aren't exposing any of the git-related features and we just have a fixed command-line anyway.

@mehdi_amini how did you install clang format? It seems like our CMake doesn't actually install a clang-format-diff executable, only a clang-format-diff.py executable, but it gets packaged on e.g. Debian/Ubuntu as clang-format-diff. I didn't think to check what LLVM does vs. what my distribution does when reviewing, but since it differs we may have to look for several different possible installations?

[mehdi_amini]

@mehdi_amini how did you install clang format? It seems like our CMake doesn't actually install a clang-format-diff executable, only a clang-format-diff.py executable, but it gets packaged on e.g. Debian/Ubuntu as clang-format-diff. I didn't think to check what LLVM does vs. what my distribution does when reviewing, but since it differs we may have to look for several different possible installations?

Here is what I have on Ubuntu:

$ dpkg -L clang-format
[...]
/usr/bin/clang-format
/usr/bin/git-clang-format
[...]
/usr/lib/clang-format
/usr/lib/clang-format/clang-format-diff.py
/usr/lib/clang-format/clang-format.el
/usr/lib/clang-format/clang-format.py

Note that there isn't clang-format-diff and the .py is not in an executable path.

[mehdi_amini]

I proposed in the original patch to call directly to clang-format-diff.py in-tree instead of looking for an installed version, would you do this instead?

[scott.linder]

In D77363#1960200, @mehdi_amini wrote:

I proposed in the original patch to call directly to clang-format-diff.py in-tree instead of looking for an installed version, would you do this instead?

That doesn't seem unreasonable, but we would also need to ensure clang-format is present as the clang-format-diff* script just calls out to it.

If the only guarantee we have across all the packaged versions is that an executable called clang-format is in the PATH, then I agree that changing the script to hash clang-format and then invoke the in-tree clang-format-diff.py is probably the best we can do.

[scott.linder]

I took a stab at take 3 at https://reviews.llvm.org/D77428

utils/arcanist/clang-format.sh

 #!/bin/bash
 
 set -euo pipefail
 
 # "script-and-regex.regex": "/^(?P<severity>.*?)\n(?P<message>.*?)\n(?P<line>\\d),(?P<char>\\d)(\n(?P<original>.*?)>>>>\n(?P<replacement>.*?)<<<<?)$/s",
 
 # Arcanist linter that invokes clang-format.
 # stdout from this script is parsed into a regex and used by Arcanist.
 # https://secure.phabricator.com/book/phabricator/article/arcanist_lint_script_and_regex/
 
 # To skip running all linters when creating/updating a diff, use `arc diff --nolint`.
 
-if ! hash clang-format-diff >/dev/null; then
+if ! hash git-clang-format >/dev/null; then
   # advice severity level is completely non-disruptive.
   # switch to warning or error if you want to prompt the user.
   echo "advice"
-  echo "clang-format-diff not found in user's PATH; not linting file."
+  echo "git-clang-format not found in user's PATH; not linting file."
   echo "===="
   exit 0
 fi
 
 src_file="${1}"
 original_file="$(mktemp)"
 formatted_file="$(mktemp)"
 readonly src_file
 readonly original_file
 readonly formatted_file
 cp -p "${src_file}" "${original_file}"
 cp -p "${src_file}" "${formatted_file}"
 
 cleanup() {
   rc=$?
   rm "${formatted_file}" "${original_file}"
   exit ${rc}
 }
 trap 'cleanup' INT HUP QUIT TERM EXIT
 
 # Arcanist can filter out lint messages for unchanged lines, but for that, we
 # need to generate line by line lint messages. Instead, we generate one lint
-# message on line 1, char 1 with file content edited using clang-format-diff.
+# message on line 1, char 1 with file content edited using git-clang-format.
 if git rev-parse --git-dir >/dev/null; then
   arc_base_commit=$(arc which --show-base)
   # An alternative is to use git-clang-format.
-  git diff -U0 --no-color "${arc_base_commit}"| clang-format-diff -style LLVM -i -p1
+  >&2 git-clang-format --quiet --force --style LLVM "${arc_base_commit}"
 else
-  svn diff --diff-cmd=diff -x -U0 "${src_file}" | clang-format-diff -style LLVM -i
+  >&2 echo "repo is expected to be a git directory"
 fi
 
 cp -p "${src_file}" "${formatted_file}"
 cp -p "${original_file}" "${src_file}"
 if ! diff -q "${src_file}" "${formatted_file}" > /dev/null ; then
   echo "autofix"
   echo "clang-format suggested style edits found:"
   echo "1,1"  # line,char of start of replacement.
   cat "${src_file}"
   echo ">>>>"
   cat "${formatted_file}"
   echo "<<<<"
 fi