From 809dbf780a8a955b9bb624c7c22cc0e2e6cdda6d Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 11 Dec 2017 10:57:46 -0800 Subject: [PATCH 1/4] Document how to use git-clang-format as a pre-commit hook. Add a simple wrapper script that adapts it for this purpose. --- README.md | 44 +++++++++++++++++++++ bin/check-clang-format.js | 82 +++++++++++++++++++++++++++++++++++++++ package.json | 7 +++- 3 files changed, 132 insertions(+), 1 deletion(-) create mode 100644 bin/check-clang-format.js diff --git a/README.md b/README.md index f167ad2..1854634 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,50 @@ If your platform isn't yet supported, you can create the native binary from the latest upstream clang sources, make sure it is stripped and optimized (should be about 1.4MB as of mid-2015) and send a pull request to add it. +## Checking formatting + +Ensuring that changes to your code are properly formatted is an important part +of your development workflow. We recommend using a git pre-commit hook. You can +configure this as follows: + +1. add a `precommit` script to your package.json file: + + ```js + "scripts": { + "precommit": "check-clang-format" + } + ``` + + By default, the user gets an error instructing them to run + `./node_modules/.bin/git-clang-format`. You may find it more ergonomic to set + up a package.json script, eg. + `"scripts": { "format": "git-clang-format" }` + + In this case, add a second argument to the "precommit" script, giving the + error you'd like to print for users, eg. + + `"precommit": "check-clang-format \"yarn format\""` + +2. Add a devDependency on the `husky` package, which will add a + `.git/hooks/pre-commit` script, which in turn triggers the `precommit` + package.json script to run whenever someone adds a commit in this repository: + + ```sh + $ yarn add -D husky + ``` + + or + + ```sh + npm install --save-dev husky + ``` + +> Why do this in a pre-commit hook? For one thing, it's faster to run + `clang-format` only on the changed files, especially as the repository grows. + Also, this lets you upgrade `clang-format` or change your settings without + needing to re-format the entire repository, while still enforcing that later + changes follow the new style. + ## Globbing files $ clang-format --glob=folder/**/*.js diff --git a/bin/check-clang-format.js b/bin/check-clang-format.js new file mode 100644 index 0000000..11cc480 --- /dev/null +++ b/bin/check-clang-format.js @@ -0,0 +1,82 @@ +#!/usr/bin/env node +/** + * @fileoverview This script provides the glue to run git-clang-format as a pre- + * commit hook. + * + * It checks that your git configuration is correct, and wraps the + * git-clang-format program by simply exiting with non-zero exit code if any + * changes are not already formatted correctly. + * + * To install it, see the instructions in the README.md. + */ + +const spawn = require('child_process').spawnSync; + +function checkGitConfig() { + const spawn_opts = {encoding: 'utf-8', stdio: ['pipe', 'pipe', 'inherit']}; + const binary = + spawn('git', ['config', '--get', 'clangFormat.binary'], spawn_opts) + .stdout.trim(); + const style = + spawn('git', ['config', '--get', 'clangFormat.style'], spawn_opts) + .stdout.trim(); + var gitConfigWrong = false; + + if (binary !== 'node_modules/.bin/clang-format') { + console.error(` + ERROR: Found git config --get clangFormat.binary = "${binary}" + This can result in running a different version of clang-format than your + co-workers, leading to inconsistent formatting.`); + gitConfigWrong = true; + } + if (style !== 'file') { + console.error(` + ERROR: Found git config --get clangFormat.style = "${style}" + The style should be set so that the settings in .clang-format are used.`); + gitConfigWrong = true; + } + if (gitConfigWrong) { + console.error(` + ERROR: You need to configure git-clang-format: + $ git config clangFormat.binary node_modules/.bin/clang-format + $ git config clangFormat.style file`); + return 2; + } +} + +function main(args) { + try { + var clangFormatPath = require.resolve('clang-format'); + var configCheck = checkGitConfig(); + if (configCheck !== 0) return configCheck; + } catch (e) { + // When running the git-clang-format on ourselves, it's located in a + // different place + var clangFormatPath = '.'; + // And we don't run the configCheck, because the clang-format binary is also + // in a different place + } + + const gitClangFormatPath = + require('path').relative(clangFormatPath, 'bin/git-clang-format'); + const result = spawn(gitClangFormatPath, ['--diff'], {encoding: 'utf-8'}); + + if (result.error) { + console.error('Error running git-clang-format:', result.error); + return 2; + } + + const clangFormatOutput = result.stdout.trim(); + if (clangFormatOutput !== ('no modified files to format') && + clangFormatOutput !== ('clang-format did not modify any files')) { + console.error(clangFormatOutput); + const fixCmd = args[0] || './node_modules/.bin/git-clang-format'; + console.error(` + ERROR: please run ${fixCmd} to format changes in your commit`); + return 1; + } +} + +if (require.main === module) { + process.exitCode = main(process.argv.slice(2)); +} diff --git a/package.json b/package.json index 8d89fc3..783b340 100644 --- a/package.json +++ b/package.json @@ -9,9 +9,11 @@ "main": "index.js", "bin": { "clang-format": "index.js", - "git-clang-format": "bin/git-clang-format" + "git-clang-format": "bin/git-clang-format", + "check-clang-format": "bin/check-clang-format.js" }, "scripts": { + "precommit": "node bin/check-clang-format.js \"bin/git-clang-format\"", "test": "./test.sh" }, "contributors": [ @@ -24,5 +26,8 @@ "async": "^1.5.2", "glob": "^7.0.0", "resolve": "^1.1.6" + }, + "devDependencies": { + "husky": "^0.14.3" } } From a288ad2bfd951132b84ac60c0f03df073df60e5c Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Mon, 11 Dec 2017 12:49:35 -0800 Subject: [PATCH 2/4] fix issues found when running check-clang-format from another repo --- bin/check-clang-format.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/bin/check-clang-format.js b/bin/check-clang-format.js index 11cc480..f6f2408 100644 --- a/bin/check-clang-format.js +++ b/bin/check-clang-format.js @@ -11,6 +11,7 @@ */ const spawn = require('child_process').spawnSync; +const path = require('path'); function checkGitConfig() { const spawn_opts = {encoding: 'utf-8', stdio: ['pipe', 'pipe', 'inherit']}; @@ -42,12 +43,14 @@ function checkGitConfig() { $ git config clangFormat.style file`); return 2; } + return 0; } function main(args) { try { - var clangFormatPath = require.resolve('clang-format'); + var clangFormatPath = path.dirname(require.resolve('clang-format')); var configCheck = checkGitConfig(); + if (configCheck !== 0) return configCheck; } catch (e) { // When running the git-clang-format on ourselves, it's located in a @@ -57,8 +60,7 @@ function main(args) { // in a different place } - const gitClangFormatPath = - require('path').relative(clangFormatPath, 'bin/git-clang-format'); + const gitClangFormatPath = path.join(clangFormatPath, 'bin/git-clang-format'); const result = spawn(gitClangFormatPath, ['--diff'], {encoding: 'utf-8'}); if (result.error) { From 8b61a660bc381176095130cc588f2f436072370d Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Thu, 21 Dec 2017 13:31:37 -0800 Subject: [PATCH 3/4] Release 1.0.41 (re-release 1.0.41 with check-clang-format script) --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 783b340..98358ab 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "clang-format", - "version": "1.0.41", + "version": "1.0.41-b", "description": "node wrapper around clang-format", "repository": { "type": "git", From b8c7df0b7078e49e68f75a4f649fae9f052e5307 Mon Sep 17 00:00:00 2001 From: Alex Eagle Date: Fri, 30 Mar 2018 15:03:31 -0700 Subject: [PATCH 4/4] Port bin/git-clang-format from master (#57) --- bin/git-clang-format | 247 ++++++++++++++++++++++++++++++------------- package.json | 2 +- 2 files changed, 172 insertions(+), 77 deletions(-) diff --git a/bin/git-clang-format b/bin/git-clang-format index 0c45762..0b21039 100755 --- a/bin/git-clang-format +++ b/bin/git-clang-format @@ -20,9 +20,10 @@ clang-format on the changes in current files or a specific commit. For further details, run: git clang-format -h -Requires Python 2.7 +Requires Python 2.7 or Python 3 """ +from __future__ import print_function import argparse import collections import contextlib @@ -32,12 +33,15 @@ import re import subprocess import sys -usage = 'git clang-format [OPTIONS] [] [--] [...]' +usage = 'git clang-format [OPTIONS] [] [] [--] [...]' 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. +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. + +If two commits are given (requires --diff), run clang-format on all lines in the +second that differ from the first . The following git-config settings set the default of the corresponding option: clangFormat.binary @@ -75,8 +79,10 @@ def main(): 'm', # ObjC 'mm', # ObjC++ 'cc', 'cp', 'cpp', 'c++', 'cxx', 'hpp', # C++ + 'cu', # CUDA # Other languages that clang-format supports 'proto', 'protodevel', # Protocol Buffers + 'java', # Java 'js', # JavaScript 'ts', # TypeScript ]) @@ -120,46 +126,59 @@ def main(): opts.verbose -= opts.quiet del opts.quiet - commit, files = interpret_args(opts.args, dash_dash, opts.commit) - changed_lines = compute_diff_and_extract_lines(commit, files) + commits, files = interpret_args(opts.args, dash_dash, opts.commit) + if len(commits) > 1: + 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) if opts.verbose >= 1: ignored_files = set(changed_lines) filter_by_extension(changed_lines, opts.extensions.lower().split(',')) if opts.verbose >= 1: ignored_files.difference_update(changed_lines) if ignored_files: - print 'Ignoring changes in the following files (wrong extension):' + print('Ignoring changes in the following files (wrong extension):') for filename in ignored_files: - print ' ', filename + print(' %s' % filename) if changed_lines: - print 'Running clang-format on the following files:' + print('Running clang-format on the following files:') for filename in changed_lines: - print ' ', filename + print(' %s' % filename) if not changed_lines: - print 'no modified files to format' + print('no modified files to format') return # The computed diff outputs absolute paths, so we must cd before accessing # those files. cd_to_toplevel() - old_tree = create_tree_from_workdir(changed_lines) - new_tree = run_clang_format_and_save_to_tree(changed_lines, - binary=opts.binary, - style=opts.style) + if len(commits) > 1: + old_tree = commits[1] + new_tree = run_clang_format_and_save_to_tree(changed_lines, + revision=commits[1], + binary=opts.binary, + style=opts.style) + else: + old_tree = create_tree_from_workdir(changed_lines) + new_tree = run_clang_format_and_save_to_tree(changed_lines, + binary=opts.binary, + style=opts.style) if opts.verbose >= 1: - print 'old tree:', old_tree - print 'new tree:', new_tree + print('old tree: %s' % old_tree) + print('new tree: %s' % new_tree) if old_tree == new_tree: if opts.verbose >= 0: - print 'clang-format did not modify any files' + print('clang-format did not modify any files') elif opts.diff: print_diff(old_tree, new_tree) else: changed_files = apply_changes(old_tree, new_tree, force=opts.force, patch_mode=opts.patch) if (opts.verbose >= 0 and not opts.patch) or opts.verbose >= 1: - print 'changed files:' + print('changed files:') for filename in changed_files: - print ' ', filename + print(' %s' % filename) def load_git_config(non_string_options=None): @@ -181,40 +200,41 @@ def load_git_config(non_string_options=None): 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). It is assumed that "--" and everything that follows has been removed from args and placed in `dash_dash`. - If "--" is present (i.e., `dash_dash` is non-empty), the argument to its - left (if present) is taken as commit. Otherwise, the first argument is - checked if it is a commit or a file. If commit is not given, - `default_commit` is used.""" + If "--" is present (i.e., `dash_dash` is non-empty), the arguments to its + left (if present) are taken as commits. Otherwise, the arguments are checked + from left to right if they are commits or files. If commits are not given, + a list with `default_commit` is used.""" if dash_dash: if len(args) == 0: - commit = default_commit - elif len(args) > 1: - die('at most one commit allowed; %d given' % len(args)) + commits = [default_commit] else: - commit = args[0] - object_type = get_object_type(commit) - if object_type not in ('commit', 'tag'): - if object_type is None: - die("'%s' is not a commit" % commit) - else: - die("'%s' is a %s, but a commit was expected" % (commit, object_type)) + commits = args + for commit in commits: + object_type = get_object_type(commit) + if object_type not in ('commit', 'tag'): + if object_type is None: + die("'%s' is not a commit" % commit) + else: + die("'%s' is a %s, but a commit was expected" % (commit, object_type)) files = dash_dash[1:] elif args: - if disambiguate_revision(args[0]): - commit = args[0] - files = args[1:] - else: - commit = default_commit - files = args + commits = [] + while args: + if not disambiguate_revision(args[0]): + break + commits.append(args.pop(0)) + if not commits: + commits = [default_commit] + files = args else: - commit = default_commit + commits = [default_commit] files = [] - return commit, files + return commits, files def disambiguate_revision(value): @@ -239,12 +259,12 @@ def get_object_type(value): stdout, stderr = p.communicate() if p.returncode != 0: return None - return stdout.strip() + return convert_string(stdout.strip()) -def compute_diff_and_extract_lines(commit, files): +def compute_diff_and_extract_lines(commits, files): """Calls compute_diff() followed by extract_lines().""" - diff_process = compute_diff(commit, files) + diff_process = compute_diff(commits, files) changed_lines = extract_lines(diff_process.stdout) diff_process.stdout.close() diff_process.wait() @@ -254,13 +274,17 @@ def compute_diff_and_extract_lines(commit, files): return changed_lines -def compute_diff(commit, files): - """Return a subprocess object producing the diff from `commit`. +def compute_diff(commits, files): + """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 `commit`, filtered on `files` - (if non-empty). Zero context lines are used in the patch.""" - cmd = ['git', 'diff-index', '-p', '-U0', commit, '--'] + 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.""" + git_tool = 'diff-index' + if len(commits) > 1: + git_tool = 'diff-tree' + cmd = ['git', git_tool, '-p', '-U0'] + commits + ['--'] cmd.extend(files) p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE) p.stdin.close() @@ -278,6 +302,7 @@ def extract_lines(patch_file): list of line `Range`s.""" matches = {} for line in patch_file: + line = convert_string(line) match = re.search(r'^\+\+\+\ [^/]+/(.*)', line) if match: filename = match.group(1).rstrip('\r\n') @@ -298,8 +323,10 @@ def filter_by_extension(dictionary, allowed_extensions): `allowed_extensions` must be a collection of lowercase file extensions, excluding the period.""" allowed_extensions = frozenset(allowed_extensions) - for filename in dictionary.keys(): + for filename in list(dictionary.keys()): base_ext = filename.rsplit('.', 1) + if len(base_ext) == 1 and '' in allowed_extensions: + continue if len(base_ext) == 1 or base_ext[1].lower() not in allowed_extensions: del dictionary[filename] @@ -317,15 +344,34 @@ def create_tree_from_workdir(filenames): return create_tree(filenames, '--stdin') -def run_clang_format_and_save_to_tree(changed_lines, binary='clang-format', - style=None): +def run_clang_format_and_save_to_tree(changed_lines, revision=None, + binary='clang-format', style=None): """Run clang-format on each file and save the result to a git tree. Returns the object ID (SHA-1) of the created tree.""" + def iteritems(container): + try: + return container.iteritems() # Python 2 + except AttributeError: + return container.items() # Python 3 def index_info_generator(): - for filename, line_ranges in changed_lines.iteritems(): - mode = oct(os.stat(filename).st_mode) - blob_id = clang_format_to_blob(filename, line_ranges, binary=binary, + for filename, line_ranges in iteritems(changed_lines): + if revision: + git_metadata_cmd = ['git', 'ls-tree', + '%s:%s' % (revision, os.path.dirname(filename)), + os.path.basename(filename)] + git_metadata = subprocess.Popen(git_metadata_cmd, stdin=subprocess.PIPE, + stdout=subprocess.PIPE) + stdout = git_metadata.communicate()[0] + mode = oct(int(stdout.split()[0], 8)) + else: + mode = oct(os.stat(filename).st_mode) + # Adjust python3 octal format so that it matches what git expects + if mode.startswith('0o'): + mode = '0' + mode[2:] + blob_id = clang_format_to_blob(filename, line_ranges, + revision=revision, + binary=binary, style=style) yield '%s %s\t%s' % (mode, blob_id, filename) return create_tree(index_info_generator(), '--index-info') @@ -343,7 +389,7 @@ def create_tree(input_lines, mode): with temporary_index_file(): p = subprocess.Popen(cmd, stdin=subprocess.PIPE) for line in input_lines: - p.stdin.write('%s\0' % line) + p.stdin.write(to_bytes('%s\0' % line)) p.stdin.close() if p.wait() != 0: die('`%s` failed' % ' '.join(cmd)) @@ -351,26 +397,42 @@ def create_tree(input_lines, mode): return tree_id -def clang_format_to_blob(filename, line_ranges, binary='clang-format', - style=None): +def clang_format_to_blob(filename, line_ranges, revision=None, + binary='clang-format', style=None): """Run clang-format on the given file and save the result to a git blob. + Runs on the file in `revision` if not None, or on the file in the working + directory if `revision` is None. + Returns the object ID (SHA-1) of the created blob.""" - clang_format_cmd = [binary, filename] + clang_format_cmd = [binary] if style: clang_format_cmd.extend(['-style='+style]) clang_format_cmd.extend([ '-lines=%s:%s' % (start_line, start_line+line_count-1) for start_line, line_count in line_ranges]) + if revision: + clang_format_cmd.extend(['-assume-filename='+filename]) + git_show_cmd = ['git', 'cat-file', 'blob', '%s:%s' % (revision, filename)] + git_show = subprocess.Popen(git_show_cmd, stdin=subprocess.PIPE, + stdout=subprocess.PIPE) + git_show.stdin.close() + clang_format_stdin = git_show.stdout + else: + clang_format_cmd.extend([filename]) + git_show = None + clang_format_stdin = subprocess.PIPE try: - clang_format = subprocess.Popen(clang_format_cmd, stdin=subprocess.PIPE, + clang_format = subprocess.Popen(clang_format_cmd, stdin=clang_format_stdin, stdout=subprocess.PIPE) + if clang_format_stdin == subprocess.PIPE: + clang_format_stdin = clang_format.stdin except OSError as e: if e.errno == errno.ENOENT: die('cannot find executable "%s"' % binary) else: raise - clang_format.stdin.close() + clang_format_stdin.close() hash_object_cmd = ['git', 'hash-object', '-w', '--path='+filename, '--stdin'] hash_object = subprocess.Popen(hash_object_cmd, stdin=clang_format.stdout, stdout=subprocess.PIPE) @@ -380,7 +442,9 @@ def clang_format_to_blob(filename, line_ranges, binary='clang-format', die('`%s` failed' % ' '.join(hash_object_cmd)) if clang_format.wait() != 0: die('`%s` failed' % ' '.join(clang_format_cmd)) - return stdout.rstrip('\r\n') + if git_show and git_show.wait() != 0: + die('`%s` failed' % ' '.join(git_show_cmd)) + return convert_string(stdout).rstrip('\r\n') @contextlib.contextmanager @@ -418,7 +482,12 @@ def print_diff(old_tree, new_tree): # We use the porcelain 'diff' and not plumbing 'diff-tree' because the output # is expected to be viewed by the user, and only the former does nice things # like color and pagination. - subprocess.check_call(['git', 'diff', old_tree, new_tree, '--']) + # + # We also only print modified files since `new_tree` only contains the files + # that were modified, so unmodified files would show as deleted without the + # filter. + subprocess.check_call(['git', 'diff', '--diff-filter=M', old_tree, new_tree, + '--']) def apply_changes(old_tree, new_tree, force=False, patch_mode=False): @@ -426,15 +495,16 @@ def apply_changes(old_tree, new_tree, force=False, patch_mode=False): Bails if there are local changes in those files and not `force`. If `patch_mode`, runs `git checkout --patch` to select hunks interactively.""" - changed_files = run('git', 'diff-tree', '-r', '-z', '--name-only', old_tree, + changed_files = run('git', 'diff-tree', '--diff-filter=M', '-r', '-z', + '--name-only', old_tree, new_tree).rstrip('\0').split('\0') if not force: unstaged_files = run('git', 'diff-files', '--name-status', *changed_files) if unstaged_files: - print >>sys.stderr, ('The following files would be modified but ' - 'have unstaged changes:') - print >>sys.stderr, unstaged_files - print >>sys.stderr, 'Please commit, stage, or stash them first.' + print('The following files would be modified but ' + 'have unstaged changes:', file=sys.stderr) + print(unstaged_files, file=sys.stderr) + print('Please commit, stage, or stash them first.', file=sys.stderr) sys.exit(2) if patch_mode: # In patch mode, we could just as well create an index from the new tree @@ -461,25 +531,50 @@ def run(*args, **kwargs): p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE) stdout, stderr = p.communicate(input=stdin) + + stdout = convert_string(stdout) + stderr = convert_string(stderr) + if p.returncode == 0: if stderr: if verbose: - print >>sys.stderr, '`%s` printed to stderr:' % ' '.join(args) - print >>sys.stderr, stderr.rstrip() + print('`%s` printed to stderr:' % ' '.join(args), file=sys.stderr) + print(stderr.rstrip(), file=sys.stderr) if strip: stdout = stdout.rstrip('\r\n') return stdout if verbose: - print >>sys.stderr, '`%s` returned %s' % (' '.join(args), p.returncode) + print('`%s` returned %s' % (' '.join(args), p.returncode), file=sys.stderr) if stderr: - print >>sys.stderr, stderr.rstrip() + print(stderr.rstrip(), file=sys.stderr) sys.exit(2) def die(message): - print >>sys.stderr, 'error:', message + print('error:', message, file=sys.stderr) sys.exit(2) +def to_bytes(str_input): + # Encode to UTF-8 to get binary data. + if isinstance(str_input, bytes): + return str_input + return str_input.encode('utf-8') + + +def to_string(bytes_input): + if isinstance(bytes_input, str): + return bytes_input + return bytes_input.encode('utf-8') + + +def convert_string(bytes_input): + try: + return to_string(bytes_input.decode('utf-8')) + except AttributeError: # 'str' object has no attribute 'decode'. + return str(bytes_input) + except UnicodeError: + return str(bytes_input) + if __name__ == '__main__': main() diff --git a/package.json b/package.json index 98358ab..580cf58 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "clang-format", - "version": "1.0.41-b", + "version": "1.0.41-c", "description": "node wrapper around clang-format", "repository": { "type": "git",