diff --git a/script/githooks/README.md b/script/githooks/README.md new file mode 100644 index 000000000000..8323cbfaa17e --- /dev/null +++ b/script/githooks/README.md @@ -0,0 +1,30 @@ +# Git Hooks + +This directory contains Git hooks for the `flutter/packages` repository. + +## Installation + +To install the Git hooks, run the following commands from the root of the repository: + +```bash +# Fetch dependencies for the githooks package +dart pub get -C script/githooks + +# Run the installation script +dart script/githooks/bin/install_hooks.dart +``` + +## Available Hooks + +### pre-commit + +The `pre-commit` hook runs automatically when you run `git commit` and performs the following checks on any staged changes: + +1. **Formatting**: It runs `dart run script/tool/bin/flutter_plugin_tools.dart format --run-on-staged-packages` to verify that all staged files in the targeted packages are correctly formatted. +2. **Static Analysis**: If formatting passes, it runs `dart run script/tool/bin/flutter_plugin_tools.dart analyze --run-on-staged-packages --dart` to run static analysis on the staged packages. + +If either check fails, it aborts the commit. To bypass the hook (for a WIP commit, for example), you can use the `--no-verify` flag: + +```bash +git commit -m "WIP" --no-verify +``` diff --git a/script/githooks/bin/install_hooks.dart b/script/githooks/bin/install_hooks.dart new file mode 100644 index 000000000000..385c926a170a --- /dev/null +++ b/script/githooks/bin/install_hooks.dart @@ -0,0 +1,34 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// ignore_for_file: avoid_print + +import 'dart:io'; +import 'package:path/path.dart' as p; + +void main() async { + Directory repoRoot = Directory.current; + while (repoRoot.path != repoRoot.parent.path && + !(Directory(p.join(repoRoot.path, '.git')).existsSync() || + File(p.join(repoRoot.path, '.git')).existsSync())) { + repoRoot = repoRoot.parent; + } + if (!(Directory(p.join(repoRoot.path, '.git')).existsSync() || + File(p.join(repoRoot.path, '.git')).existsSync())) { + print('Installation failed because .git directory could not be found.'); + exit(1); + } + + final ProcessResult result = await Process.run('git', [ + 'config', + 'core.hooksPath', + 'script/githooks', + ], workingDirectory: repoRoot.path); + if (result.exitCode == 0) { + print('Git hooks installed successfully!'); + } else { + print('Failed to install Git hooks: ${result.stderr}'); + exit(1); + } +} diff --git a/script/githooks/bin/main.dart b/script/githooks/bin/main.dart new file mode 100644 index 000000000000..7d734dd82b52 --- /dev/null +++ b/script/githooks/bin/main.dart @@ -0,0 +1,11 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' as io; + +import 'package:githooks/githooks.dart'; + +Future main(List args) async { + io.exitCode = await run(args); +} diff --git a/script/githooks/lib/githooks.dart b/script/githooks/lib/githooks.dart new file mode 100644 index 000000000000..59b3e6fe3fff --- /dev/null +++ b/script/githooks/lib/githooks.dart @@ -0,0 +1,16 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:args/command_runner.dart'; + +import 'src/pre_commit_command.dart'; + +/// Runs the githooks command line utility. +Future run(List args) async { + final runner = CommandRunner('githooks', 'Git hooks for flutter/packages') + ..addCommand(PreCommitCommand()); + + final bool success = await runner.run(args) ?? false; + return success ? 0 : 1; +} diff --git a/script/githooks/lib/src/pre_commit_command.dart b/script/githooks/lib/src/pre_commit_command.dart new file mode 100644 index 000000000000..cd606f1f1c48 --- /dev/null +++ b/script/githooks/lib/src/pre_commit_command.dart @@ -0,0 +1,180 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// ignore_for_file: avoid_print + +import 'dart:io'; +import 'package:args/command_runner.dart'; +import 'package:path/path.dart' as p; + +/// The command that implements the pre-commit githook. +class PreCommitCommand extends Command { + /// Creates a [PreCommitCommand]. + PreCommitCommand({ + Future Function( + String executable, + List arguments, { + String? workingDirectory, + })? + processRunner, + }) : processRunner = processRunner ?? Process.run; + + /// The process runner injected for testing. + final Future Function( + String executable, + List arguments, { + String? workingDirectory, + }) + processRunner; + + @override + final String name = 'pre-commit'; + + @override + final String description = + 'Runs formatting and static analysis checks on staged changes before a "git commit"'; + + /// Runs a pre-commit check for correct formatting and static analysis. + /// + /// It runs the plugin tool format and analyze commands on all packages that have staged changes. + /// If any of the commands fail, it will return false; otherwise, it will return true. + @override + Future run() async { + final Directory? repoRoot = await _findRepoRoot(); + if (repoRoot == null) { + print('Could not find git repository.'); + return false; + } + + // Skips check if there are no staged package changes because this + // makes a ~5s difference in runtime. + // TODO(camsim99): Investigate why the plugins tool adds overhead: + // https://github.com/flutter/flutter/issues/188870. + final bool? hasStaged = await _hasStagedPackages(repoRoot); + if (hasStaged == null) { + return false; + } + if (!hasStaged) { + print('No staged package changes to check.'); + return true; + } + + final String toolScript = p.join( + repoRoot.path, + 'script', + 'tool', + 'bin', + 'flutter_plugin_tools.dart', + ); + + print('Running pre-commit format and static analysis checks for staged changes...'); + + // Check format. + final bool formatPassed = await _executeCheckFormatting(repoRoot, toolScript); + if (!formatPassed) { + return false; + } + + return _executeCheckStaticAnalysis(repoRoot, toolScript); + } + + /// Finds the repository root directory using git. + /// + /// Returns null if git fails or if the directory is not a git repository. + Future _findRepoRoot() async { + final ProcessResult rootResult = await processRunner('git', [ + 'rev-parse', + '--show-toplevel', + ], workingDirectory: Directory.current.path); + + if (rootResult.exitCode != 0) { + return null; + } + return Directory((rootResult.stdout as String).trim()); + } + + /// Checks if there are any staged package changes in the repository. + /// + /// Returns true if at least one staged file is located within a package directory + /// (under packages/ or third_party/packages/), false if there are none, or null + /// if the git command fails. + Future _hasStagedPackages(Directory repoRoot) async { + final ProcessResult diffResult = await processRunner('git', [ + 'diff', + '--cached', + '-z', + '--name-only', + ], workingDirectory: repoRoot.path); + + if (diffResult.exitCode != 0) { + print('Failed to check staged changes.'); + if (diffResult.stderr.toString().isNotEmpty) { + print(diffResult.stderr); + } + // If we cannot determine the diff, abort pre-commit check by returning null. + return null; + } + + final stdoutStr = diffResult.stdout as String; + if (stdoutStr.isEmpty) { + return false; + } + + final List lines = stdoutStr.split('\u0000') + ..removeWhere((String element) => element.isEmpty); + return lines.any( + (String path) => path.startsWith('packages/') || path.startsWith('third_party/packages/'), + ); + } + + /// Runs the formatting check on staged files. + /// + /// Returns true if all staged files are correctly formatted or false otherwise. + Future _executeCheckFormatting(Directory repoRoot, String toolScript) async { + final ProcessResult formatResult = await processRunner('dart', [ + 'run', + toolScript, + 'format', + '--run-on-staged-packages', + '--fail-on-change', + ], workingDirectory: repoRoot.path); + + if (formatResult.exitCode != 0) { + print(''' +Formatting check failed. +To fix formatting automatically, run: + dart run script/tool/bin/flutter_plugin_tools.dart format --run-on-staged-packages +To bypass this check, commit with --no-verify.'''); + return false; + } + + print('Formatting looks good!'); + return true; + } + + /// Runs the static analysis check on staged files. + /// + /// Returns true if all staged files pass analysis or false otherwise. + Future _executeCheckStaticAnalysis(Directory repoRoot, String toolScript) async { + final ProcessResult analyzeResult = await processRunner('dart', [ + 'run', + toolScript, + 'analyze', + '--run-on-staged-packages', + '--dart', + ], workingDirectory: repoRoot.path); + + if (analyzeResult.exitCode != 0) { + print(''' +Static analysis check failed. +To view and fix analysis errors, run: + dart run script/tool/bin/flutter_plugin_tools.dart analyze --run-on-staged-packages --dart +To bypass this check, commit with --no-verify.'''); + return false; + } + + print('Static analysis looks good!'); + return true; + } +} diff --git a/script/githooks/pre-commit b/script/githooks/pre-commit new file mode 100755 index 000000000000..809f62c219df --- /dev/null +++ b/script/githooks/pre-commit @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +set -e + +HOOKS_DIR="$(dirname "$0")" +exec dart "$HOOKS_DIR/bin/main.dart" pre-commit "$@" diff --git a/script/githooks/pubspec.yaml b/script/githooks/pubspec.yaml new file mode 100644 index 000000000000..71a0e69c1d42 --- /dev/null +++ b/script/githooks/pubspec.yaml @@ -0,0 +1,13 @@ +name: githooks +description: Git hooks for the flutter/packages repository. For more information on the hooks and how to install them, see README.md +publish_to: none + +environment: + sdk: ^3.10.0-0 + +dependencies: + args: any + path: any + +dev_dependencies: + test: any diff --git a/script/githooks/test/pre_commit_command_test.dart b/script/githooks/test/pre_commit_command_test.dart new file mode 100644 index 000000000000..5da81df28b3b --- /dev/null +++ b/script/githooks/test/pre_commit_command_test.dart @@ -0,0 +1,209 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io'; + +import 'package:githooks/src/pre_commit_command.dart'; +import 'package:test/test.dart'; + +void main() { + const repoRoot = '/mock/repo/root'; + const toolScript = '$repoRoot/script/tool/bin/flutter_plugin_tools.dart'; + + group('pre-commit hook', () { + test('passes when both format and analyze succeed', () async { + final List> executedArguments = []; + final command = PreCommitCommand( + processRunner: + (String executable, List arguments, {String? workingDirectory}) async { + executedArguments.add(arguments); + if (executable == 'git') { + if (arguments.contains('rev-parse')) { + return ProcessResult(0, 0, '$repoRoot\n', ''); + } + if (arguments.contains('diff')) { + return ProcessResult(0, 0, 'packages/a_plugin/lib/a.dart\u0000', ''); + } + } + return ProcessResult(0, 0, 'Success', ''); + }, + ); + + final bool result = await command.run(); + expect(result, isTrue); + + // Verify the exact arguments passed to format and analyze + expect( + executedArguments, + anyElement( + equals(['run', toolScript, 'format', '--run-on-staged-packages', '--fail-on-change']), + ), + ); + expect( + executedArguments, + anyElement(equals(['run', toolScript, 'analyze', '--run-on-staged-packages', '--dart'])), + ); + }); + + test('fails when formatting fails', () async { + final List> executedArguments = []; + final command = PreCommitCommand( + processRunner: + (String executable, List arguments, {String? workingDirectory}) async { + executedArguments.add(arguments); + if (executable == 'git') { + if (arguments.contains('rev-parse')) { + return ProcessResult(0, 0, '$repoRoot\n', ''); + } + if (arguments.contains('diff')) { + return ProcessResult(0, 0, 'packages/a_plugin/lib/a.dart\u0000', ''); + } + } + if (arguments.contains('format')) { + return ProcessResult(0, 1, 'bad_file.dart', ''); + } + return ProcessResult(0, 0, 'Success', ''); + }, + ); + + final bool result = await command.run(); + expect(result, isFalse); + + expect( + executedArguments, + anyElement( + equals(['run', toolScript, 'format', '--run-on-staged-packages', '--fail-on-change']), + ), + ); + // Verify that static analysis was skipped because formatting failed + expect(executedArguments.any((args) => args.contains('analyze')), isFalse); + }); + + test('fails when analysis fails', () async { + final List> executedArguments = []; + final command = PreCommitCommand( + processRunner: + (String executable, List arguments, {String? workingDirectory}) async { + executedArguments.add(arguments); + if (executable == 'git') { + if (arguments.contains('rev-parse')) { + return ProcessResult(0, 0, '$repoRoot\n', ''); + } + if (arguments.contains('diff')) { + return ProcessResult(0, 0, 'packages/a_plugin/lib/a.dart\u0000', ''); + } + } + if (arguments.contains('analyze')) { + return ProcessResult(0, 1, 'error in file.dart', ''); + } + return ProcessResult(0, 0, 'Success', ''); + }, + ); + + final bool result = await command.run(); + expect(result, isFalse); + + expect( + executedArguments, + anyElement(equals(['run', toolScript, 'analyze', '--run-on-staged-packages', '--dart'])), + ); + }); + + test('exits early with success if there are no staged packages', () async { + final List> executedArguments = []; + final command = PreCommitCommand( + processRunner: + (String executable, List arguments, {String? workingDirectory}) async { + executedArguments.add(arguments); + if (executable == 'git') { + if (arguments.contains('rev-parse')) { + return ProcessResult(0, 0, '$repoRoot\n', ''); + } + if (arguments.contains('diff')) { + return ProcessResult( + 0, + 0, + 'script/githooks/test/pre_commit_command_test.dart\u0000', + '', + ); + } + } + return ProcessResult(0, 0, 'Success', ''); + }, + ); + + final bool result = await command.run(); + expect(result, isTrue); + + // Verify that no format or analyze commands were run + expect( + executedArguments.any((args) => args.contains('format') || args.contains('analyze')), + isFalse, + ); + }); + + test('exits early with success if there are no staged changes at all', () async { + final List> executedArguments = []; + final command = PreCommitCommand( + processRunner: + (String executable, List arguments, {String? workingDirectory}) async { + executedArguments.add(arguments); + if (executable == 'git') { + if (arguments.contains('rev-parse')) { + return ProcessResult(0, 0, '$repoRoot\n', ''); + } + if (arguments.contains('diff')) { + return ProcessResult(0, 0, '', ''); + } + } + return ProcessResult(0, 0, 'Success', ''); + }, + ); + + final bool result = await command.run(); + expect(result, isTrue); + + // Verify that no format or analyze commands were run + expect( + executedArguments.any((args) => args.contains('format') || args.contains('analyze')), + isFalse, + ); + }); + + test('fails when checking staged packages fails', () async { + final command = PreCommitCommand( + processRunner: + (String executable, List arguments, {String? workingDirectory}) async { + if (executable == 'git') { + if (arguments.contains('rev-parse')) { + return ProcessResult(0, 0, '$repoRoot\n', ''); + } + if (arguments.contains('diff')) { + return ProcessResult(0, 1, '', 'Git diff failed'); + } + } + return ProcessResult(0, 0, 'Success', ''); + }, + ); + + final bool result = await command.run(); + expect(result, isFalse); + }); + + test('fails when git root cannot be found', () async { + final command = PreCommitCommand( + processRunner: + (String executable, List arguments, {String? workingDirectory}) async { + if (executable == 'git' && arguments.contains('rev-parse')) { + return ProcessResult(0, 1, '', 'Not a git repository'); + } + return ProcessResult(0, 0, 'Success', ''); + }, + ); + + final bool result = await command.run(); + expect(result, isFalse); + }); + }); +} diff --git a/script/tool/lib/src/common/git_version_finder.dart b/script/tool/lib/src/common/git_version_finder.dart index c033e255729d..07a91c1eb806 100644 --- a/script/tool/lib/src/common/git_version_finder.dart +++ b/script/tool/lib/src/common/git_version_finder.dart @@ -35,17 +35,43 @@ class GitVersionFinder { final String baseSha = await getBaseSha(); final io.ProcessResult changedFilesCommand = await baseGitDir.runCommand([ 'diff', + '-z', '--name-only', baseSha, if (!includeUncommitted) 'HEAD', ]); - final changedFilesStdout = changedFilesCommand.stdout.toString(); - if (changedFilesStdout.isEmpty) { + return _splitDiffOutputs(changedFilesCommand.stdout.toString()); + } + + /// Get a list of all the staged files. + Future> getStagedFiles() async { + final io.ProcessResult changedFilesCommand = await baseGitDir.runCommand(const [ + 'diff', + '--cached', + '-z', + '--name-only', + '--diff-filter=ACM', + ]); + return _splitDiffOutputs(changedFilesCommand.stdout.toString()); + } + + /// Splits the stdout of a `git diff` command into a list of file paths. + /// + /// When `git diff` is run with the `-z` flag, it outputs file paths separated + /// by a null byte (`\u0000`, ASCII 0). Otherwise, it outputs file paths + /// separated by newlines. This method accounts for that by checking for + /// null bytes and falling back to splitting by newlines if none are found. + List _splitDiffOutputs(String stdout) { + if (stdout.isEmpty) { return []; } - final List changedFiles = changedFilesStdout.split('\n') - ..removeWhere((String element) => element.isEmpty); - return changedFiles.toList(); + final List files; + if (stdout.contains('\u0000')) { + files = stdout.split('\u0000'); + } else { + files = stdout.split('\n'); + } + return files.where((String file) => file.isNotEmpty).toList(); } /// Get a list of all the changed files. diff --git a/script/tool/lib/src/common/package_command.dart b/script/tool/lib/src/common/package_command.dart index 8d0fbb591436..636c80f62dd9 100644 --- a/script/tool/lib/src/common/package_command.dart +++ b/script/tool/lib/src/common/package_command.dart @@ -96,7 +96,7 @@ abstract class PackageCommand extends Command { 'Cannot be combined with $_packagesArg.\n', ); argParser.addFlag( - _runOnDirtyPackagesArg, + runOnDirtyPackagesArg, negatable: false, help: 'Run the command on packages with changes that have not been committed.\n' @@ -158,6 +158,10 @@ abstract class PackageCommand extends Command { 'Currently only logs per-package timing for multi-package commands, ' 'but more information may be added in the future.', ); + argParser.addFlag( + runOnStagedPackagesArg, + help: 'Run the command on all packages with staged changes.', + ); } // Package selection. @@ -166,10 +170,16 @@ abstract class PackageCommand extends Command { static const String _currentPackageArg = 'current-package'; static const String _pluginsLegacyAliasArg = 'plugins'; static const String _runOnChangedPackagesArg = 'run-on-changed-packages'; - static const String _runOnDirtyPackagesArg = 'run-on-dirty-packages'; static const String _exactMatchOnlyArg = 'exact-match-only'; static const String _excludeArg = 'exclude'; static const String _filterPackagesArg = 'filter-packages-to'; + + /// Package selection flag for packages with staged changes. + static const String runOnStagedPackagesArg = 'run-on-staged-packages'; + + /// Package selection flag for packages with any changes (staged or not). + static const String runOnDirtyPackagesArg = 'run-on-dirty-packages'; + // Diff base selection. static const String _baseBranchArg = 'base-branch'; static const String _baseShaArg = 'base-sha'; @@ -384,9 +394,10 @@ abstract class PackageCommand extends Command { final packageSelectionFlags = { _packagesArg, _runOnChangedPackagesArg, - _runOnDirtyPackagesArg, + runOnDirtyPackagesArg, _packagesForBranchArg, _currentPackageArg, + runOnStagedPackagesArg, }; if (packageSelectionFlags.where((String flag) => argResults!.wasParsed(flag)).length > 1) { printError( @@ -404,7 +415,8 @@ abstract class PackageCommand extends Command { final bool allowGroupMatching = !(getBoolArg(_exactMatchOnlyArg) || argResults!.wasParsed(_runOnChangedPackagesArg) || - argResults!.wasParsed(_runOnDirtyPackagesArg) || + argResults!.wasParsed(runOnDirtyPackagesArg) || + argResults!.wasParsed(runOnStagedPackagesArg) || argResults!.wasParsed(_packagesForBranchArg)); var packages = Set.from(getStringListArg(_packagesArg)); @@ -458,7 +470,7 @@ abstract class PackageCommand extends Command { print('Running for all packages that have diffs relative to "$baseSha"\n'); packages = _getChangedPackageNames(changedFiles); } - } else if (getBoolArg(_runOnDirtyPackagesArg)) { + } else if (getBoolArg(runOnDirtyPackagesArg)) { final gitVersionFinder = GitVersionFinder(await gitDir, baseSha: 'HEAD'); print('Running for all packages that have uncommitted changes\n'); // _changesRequireFullTest is deliberately not used here, as this flag is @@ -482,6 +494,15 @@ abstract class PackageCommand extends Command { throw ToolExit(exitInvalidArguments); } packages = {currentPackageName}; + } else if (getBoolArg(runOnStagedPackagesArg)) { + final gitVersionFinder = GitVersionFinder(await gitDir, baseSha: 'HEAD'); + print('Running for all packages that have staged changes\n'); + packages = _getChangedPackageNames( + await gitVersionFinder.getStagedFiles(), + ); + if (packages.isEmpty) { + return; + } } final Set excludedPackageNames = getExcludedPackageNames(); diff --git a/script/tool/lib/src/common/package_looping_command.dart b/script/tool/lib/src/common/package_looping_command.dart index 96d71605d84c..0d7f528da23f 100644 --- a/script/tool/lib/src/common/package_looping_command.dart +++ b/script/tool/lib/src/common/package_looping_command.dart @@ -296,9 +296,19 @@ abstract class PackageLoopingCommand extends PackageCommand { final runStart = DateTime.now(); // Populate the list of changed files for subclasses to use. - final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); - baseSha = await gitVersionFinder.getBaseSha(); - changedFiles = await gitVersionFinder.getChangedFiles(); + if (getBoolArg(PackageCommand.runOnDirtyPackagesArg)) { + baseSha = 'HEAD'; + final gitVersionFinder = GitVersionFinder(await gitDir, baseSha: baseSha); + changedFiles = await gitVersionFinder.getChangedFiles(includeUncommitted: true); + } else if (getBoolArg(PackageCommand.runOnStagedPackagesArg)) { + baseSha = 'HEAD'; + final gitVersionFinder = GitVersionFinder(await gitDir, baseSha: baseSha); + changedFiles = await gitVersionFinder.getStagedFiles(); + } else { + final GitVersionFinder gitVersionFinder = await retrieveVersionFinder(); + baseSha = await gitVersionFinder.getBaseSha(); + changedFiles = await gitVersionFinder.getChangedFiles(); + } // Check whether the command needs to run. if (changedFiles.isNotEmpty && changedFiles.every(shouldIgnoreFile)) { diff --git a/script/tool/lib/src/format_command.dart b/script/tool/lib/src/format_command.dart index 2e25c93b8f39..d0d32649a618 100644 --- a/script/tool/lib/src/format_command.dart +++ b/script/tool/lib/src/format_command.dart @@ -12,6 +12,7 @@ import 'package:pub_semver/pub_semver.dart'; import 'common/core.dart'; import 'common/output_utils.dart'; +import 'common/package_command.dart' show PackageCommand; import 'common/package_looping_command.dart'; import 'common/pub_utils.dart'; import 'common/repository_package.dart'; @@ -94,9 +95,6 @@ class FormatCommand extends PackageLoopingCommand { @override Future initializeRun() async { - final String javaFormatterPath = await _downloadJavaFormatterIfNecessary(); - final String kotlinFormatterPath = await _downloadKotlinFormatterIfNecessary(); - // All but Dart is formatted here rather than in runForPackage because // running the formatters separately for each package is an order of // magnitude slower, due to the startup overhead of the formatters. @@ -106,10 +104,10 @@ class FormatCommand extends PackageLoopingCommand { // formatter isn't running in the context of the package. final Iterable files = await _getFilteredFilePaths(getFiles(), relativeTo: packagesDir); if (getBoolArg(_javaArg)) { - await _formatJava(files, javaFormatterPath); + await _formatJava(files); } if (getBoolArg(_kotlinArg)) { - await _formatKotlin(files, kotlinFormatterPath); + await _formatKotlin(files); } if (getBoolArg(_clangFormatArg)) { await _formatCppAndObjectiveC(files); @@ -269,9 +267,10 @@ class FormatCommand extends PackageLoopingCommand { throw ToolExit(_exitDependencyMissing); } - Future _formatJava(Iterable files, String formatterPath) async { + Future _formatJava(Iterable files) async { final Iterable javaFiles = _getPathsWithExtensions(files, {'.java'}); if (javaFiles.isNotEmpty) { + final String formatterPath = await _downloadJavaFormatterIfNecessary(); final String java = getStringArg(_javaPathArg); if (!await _hasDependency(java)) { printError( @@ -294,9 +293,10 @@ class FormatCommand extends PackageLoopingCommand { } } - Future _formatKotlin(Iterable files, String formatterPath) async { + Future _formatKotlin(Iterable files) async { final Iterable kotlinFiles = _getPathsWithExtensions(files, {'.kt'}); if (kotlinFiles.isNotEmpty) { + final String formatterPath = await _downloadKotlinFormatterIfNecessary(); final String java = getStringArg(_javaPathArg); if (!await _hasDependency(java)) { printError( @@ -394,8 +394,16 @@ class FormatCommand extends PackageLoopingCommand { const handFormattedExtension = '.dart'; const handFormattedPragma = '// This file is hand-formatted.'; + final bool useDiff = getBoolArg(PackageCommand.runOnStagedPackagesArg); + return files .where((File file) { + if (useDiff) { + final String repoRelativePath = getRelativePosixPath(file, from: rootDir); + if (!changedFiles.contains(repoRelativePath)) { + return false; + } + } // See comment above near [handFormattedPragma]. return path.extension(file.path) != handFormattedExtension || !file.readAsLinesSync().contains(handFormattedPragma); diff --git a/script/tool/test/common/git_version_finder_test.dart b/script/tool/test/common/git_version_finder_test.dart index 9ac9dd2f4759..8ea776ceae40 100644 --- a/script/tool/test/common/git_version_finder_test.dart +++ b/script/tool/test/common/git_version_finder_test.dart @@ -43,10 +43,7 @@ void main() { }); test('get correct files changed based on git diff', () async { - gitDiffResponse = ''' -file1/file1.cc -file2/file2.cc -'''; + gitDiffResponse = 'file1/file1.cc\u0000file2/file2.cc\u0000'; final finder = GitVersionFinder(gitDir, baseSha: 'some base sha'); final List changedFiles = await finder.getChangedFiles(); @@ -55,10 +52,7 @@ file2/file2.cc test('use correct base sha if not specified', () async { mergeBaseResponse = 'shaqwiueroaaidf12312jnadf123nd'; - gitDiffResponse = ''' -file1/pubspec.yaml -file2/file2.cc -'''; + gitDiffResponse = 'file1/pubspec.yaml\u0000file2/file2.cc\u0000'; final finder = GitVersionFinder(gitDir); await finder.getChangedFiles(); @@ -70,15 +64,12 @@ file2/file2.cc 'HEAD', ], throwOnError: false), ); - verify(gitDir.runCommand(['diff', '--name-only', mergeBaseResponse, 'HEAD'])); + verify(gitDir.runCommand(['diff', '-z', '--name-only', mergeBaseResponse, 'HEAD'])); }); test('uses correct base branch to find base sha if specified', () async { mergeBaseResponse = 'shaqwiueroaaidf12312jnadf123nd'; - gitDiffResponse = ''' -file1/pubspec.yaml -file2/file2.cc -'''; + gitDiffResponse = 'file1/pubspec.yaml\u0000file2/file2.cc\u0000'; final finder = GitVersionFinder(gitDir, baseBranch: 'upstream/main'); await finder.getChangedFiles(); @@ -90,29 +81,48 @@ file2/file2.cc 'HEAD', ], throwOnError: false), ); - verify(gitDir.runCommand(['diff', '--name-only', mergeBaseResponse, 'HEAD'])); + verify(gitDir.runCommand(['diff', '-z', '--name-only', mergeBaseResponse, 'HEAD'])); }); test('use correct base sha if specified', () async { const customBaseSha = 'aklsjdcaskf12312'; - gitDiffResponse = ''' -file1/pubspec.yaml -file2/file2.cc -'''; + gitDiffResponse = 'file1/pubspec.yaml\u0000file2/file2.cc\u0000'; final finder = GitVersionFinder(gitDir, baseSha: customBaseSha); await finder.getChangedFiles(); - verify(gitDir.runCommand(['diff', '--name-only', customBaseSha, 'HEAD'])); + verify(gitDir.runCommand(['diff', '-z', '--name-only', customBaseSha, 'HEAD'])); }); test('include uncommitted files if requested', () async { const customBaseSha = 'aklsjdcaskf12312'; - gitDiffResponse = ''' -file1/pubspec.yaml -file2/file2.cc -'''; + gitDiffResponse = 'file1/pubspec.yaml\u0000file2/file2.cc\u0000'; final finder = GitVersionFinder(gitDir, baseSha: customBaseSha); await finder.getChangedFiles(includeUncommitted: true); // The call should not have HEAD as a final argument like the default diff. - verify(gitDir.runCommand(['diff', '--name-only', customBaseSha])); + verify(gitDir.runCommand(['diff', '-z', '--name-only', customBaseSha])); + }); + + test('handles filenames with spaces and quotes', () async { + gitDiffResponse = 'file name with space.cc\u0000file"with"quote.cc\u0000'; + final finder = GitVersionFinder(gitDir, baseSha: 'some base sha'); + final List changedFiles = await finder.getChangedFiles(); + + expect(changedFiles, equals(['file name with space.cc', 'file"with"quote.cc'])); + }); + + test('handles legacy newline-terminated output', () async { + gitDiffResponse = 'file1.cc\nfile2.cc\n'; + final finder = GitVersionFinder(gitDir, baseSha: 'some base sha'); + final List changedFiles = await finder.getChangedFiles(); + + expect(changedFiles, equals(['file1.cc', 'file2.cc'])); + }); + + test('throws ProcessException when git diff fails', () async { + when( + gitDir.runCommand(argThat(contains('diff')), throwOnError: anyNamed('throwOnError')), + ).thenThrow(const ProcessException('git', ['diff'], 'Git diff failed')); + + final finder = GitVersionFinder(gitDir, baseSha: 'some base sha'); + expect(() => finder.getChangedFiles(), throwsA(isA())); }); } diff --git a/script/tool/test/common/package_command_test.dart b/script/tool/test/common/package_command_test.dart index abe197a9be31..2aa109f050aa 100644 --- a/script/tool/test/common/package_command_test.dart +++ b/script/tool/test/common/package_command_test.dart @@ -349,7 +349,24 @@ packages/plugin1/plugin1/plugin1.dart Error? commandError; final List output = await runCapturingPrint( runner, - ['sample', '--packages-for-branch', '--packages=plugin1'], + ['sample', '--packages-for-branch', '--run-on-changed-packages'], + errorHandler: (Error e) { + commandError = e; + }, + ); + + expect(commandError, isA()); + expect( + output, + containsAllInOrder([contains('Only one of the package selection arguments')]), + ); + }); + + test('does not allow --run-on-dirty-packages with --run-on-staged-packages', () async { + Error? commandError; + final List output = await runCapturingPrint( + runner, + ['sample', '--run-on-dirty-packages', '--run-on-staged-packages'], errorHandler: (Error e) { commandError = e; }, @@ -1018,6 +1035,55 @@ packages/b_package/lib/src/foo.dart expect(command.plugins, unorderedEquals([packageA.path])); }); }); + + group('test run-on-staged-packages', () { + test('no packages should be tested if there are no changes.', () async { + createFakePackage('a_package', packagesDir); + await runCapturingPrint(runner, ['sample', '--run-on-staged-packages']); + + expect(command.plugins, unorderedEquals([])); + }); + + test('Only changed packages should be tested.', () async { + gitProcessRunner.mockProcessesForExecutable['git-diff'] = [ + FakeProcessInfo(MockProcess(stdout: 'packages/a_package/lib/a_package.dart')), + ]; + final RepositoryPackage packageA = createFakePackage('a_package', packagesDir); + createFakePlugin('b_package', packagesDir); + final List output = await runCapturingPrint(runner, [ + 'sample', + '--run-on-staged-packages', + ]); + + expect( + output, + containsAllInOrder([ + contains('Running for all packages that have staged changes'), + ]), + ); + + expect(command.plugins, unorderedEquals([packageA.path])); + }); + + test('multiple packages changed should test all the changed packages', () async { + gitProcessRunner.mockProcessesForExecutable['git-diff'] = [ + FakeProcessInfo( + MockProcess( + stdout: ''' +packages/a_package/lib/a_package.dart +packages/b_package/lib/src/foo.dart +''', + ), + ), + ]; + final RepositoryPackage packageA = createFakePackage('a_package', packagesDir); + final RepositoryPackage packageB = createFakePackage('b_package', packagesDir); + createFakePackage('c_package', packagesDir); + await runCapturingPrint(runner, ['sample', '--run-on-staged-packages']); + + expect(command.plugins, unorderedEquals([packageA.path, packageB.path])); + }); + }); }); group('--packages-for-branch', () { @@ -1053,7 +1119,9 @@ packages/b_package/lib/src/foo.dart // Ensure that it's diffing against the merge-base. expect( gitProcessRunner.recordedCalls, - contains(const ProcessCall('git-diff', ['--name-only', 'abc123', 'HEAD'], null)), + contains( + const ProcessCall('git-diff', ['-z', '--name-only', 'abc123', 'HEAD'], null), + ), ); }); @@ -1084,7 +1152,9 @@ packages/b_package/lib/src/foo.dart // Ensure that it's diffing against the prior commit. expect( gitProcessRunner.recordedCalls, - contains(const ProcessCall('git-diff', ['--name-only', 'HEAD~', 'HEAD'], null)), + contains( + const ProcessCall('git-diff', ['-z', '--name-only', 'HEAD~', 'HEAD'], null), + ), ); }); @@ -1116,7 +1186,9 @@ packages/b_package/lib/src/foo.dart // Ensure that it's diffing against the prior commit. expect( gitProcessRunner.recordedCalls, - contains(const ProcessCall('git-diff', ['--name-only', 'HEAD~', 'HEAD'], null)), + contains( + const ProcessCall('git-diff', ['-z', '--name-only', 'HEAD~', 'HEAD'], null), + ), ); }); @@ -1160,7 +1232,9 @@ packages/b_package/lib/src/foo.dart // Ensure that it's diffing against the prior commit. expect( gitProcessRunner.recordedCalls, - contains(const ProcessCall('git-diff', ['--name-only', 'HEAD~', 'HEAD'], null)), + contains( + const ProcessCall('git-diff', ['-z', '--name-only', 'HEAD~', 'HEAD'], null), + ), ); }); @@ -1191,7 +1265,9 @@ packages/b_package/lib/src/foo.dart // Ensure that it's diffing against the prior commit. expect( gitProcessRunner.recordedCalls, - contains(const ProcessCall('git-diff', ['--name-only', 'HEAD~', 'HEAD'], null)), + contains( + const ProcessCall('git-diff', ['-z', '--name-only', 'HEAD~', 'HEAD'], null), + ), ); }); diff --git a/script/tool/test/format_command_test.dart b/script/tool/test/format_command_test.dart index 18647d28d783..53529c2b4a93 100644 --- a/script/tool/test/format_command_test.dart +++ b/script/tool/test/format_command_test.dart @@ -21,6 +21,7 @@ void main() { late MockPlatform mockPlatform; late Directory packagesDir; late RecordingProcessRunner processRunner; + late RecordingProcessRunner gitProcessRunner; late FormatCommand analyzeCommand; late CommandRunner runner; late String javaFormatPath; @@ -29,7 +30,7 @@ void main() { setUp(() { mockPlatform = MockPlatform(); final GitDir gitDir; - (:packagesDir, :processRunner, gitProcessRunner: _, :gitDir) = configureBaseCommandMocks( + (:packagesDir, :processRunner, :gitProcessRunner, :gitDir) = configureBaseCommandMocks( platform: mockPlatform, ); analyzeCommand = FormatCommand( @@ -179,6 +180,212 @@ void main() { ); }); + test('formats only staged files when --run-on-staged-packages is used', () async { + const files = ['lib/a.dart', 'lib/src/b.dart', 'lib/src/c.dart']; + final RepositoryPackage plugin = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: files, + dartConstraint: _dartConstraint, + ); + fakePubGet(plugin); + + // Mock git diff --cached to return only lib/src/b.dart (called three times) + const stagedFilePath = 'packages/a_plugin/lib/src/b.dart'; + gitProcessRunner.mockProcessesForExecutable['git-diff'] = List.generate( + 3, + (_) => FakeProcessInfo(MockProcess(stdout: stagedFilePath)), + ); + + await runCapturingPrint(runner, [ + 'format', + '--run-on-staged-packages', + ]); + + expect( + processRunner.recordedCalls, + orderedEquals([ + ProcessCall('dart', const ['format', 'lib/src/b.dart'], plugin.path), + ]), + ); + }); + + test('skips formatting when there are no staged packages', () async { + final RepositoryPackage plugin = createFakePlugin( + 'a_plugin', + packagesDir, + dartConstraint: _dartConstraint, + ); + fakePubGet(plugin); + + gitProcessRunner.mockProcessesForExecutable['git-diff'] = List.generate( + 3, + (_) => FakeProcessInfo(MockProcess(stdout: '')), + ); + + await runCapturingPrint(runner, ['format', '--run-on-staged-packages']); + + expect(processRunner.recordedCalls, isEmpty); + }); + + test('formats only staged files across multiple packages', () async { + final RepositoryPackage pluginA = createFakePlugin( + 'plugin_a', + packagesDir, + extraFiles: ['lib/a.dart', 'lib/src/unused_a.dart'], + dartConstraint: _dartConstraint, + ); + fakePubGet(pluginA); + + final RepositoryPackage pluginB = createFakePlugin( + 'plugin_b', + packagesDir, + extraFiles: ['lib/b.dart', 'lib/src/unused_b.dart'], + dartConstraint: _dartConstraint, + ); + fakePubGet(pluginB); + + // Mock git diff to return both staged files (called three times) + const stagedFiles = 'packages/plugin_a/lib/a.dart\npackages/plugin_b/lib/b.dart\n'; + gitProcessRunner.mockProcessesForExecutable['git-diff'] = List.generate( + 3, + (_) => FakeProcessInfo(MockProcess(stdout: stagedFiles)), + ); + + await runCapturingPrint(runner, [ + 'format', + '--run-on-staged-packages', + ]); + + expect( + processRunner.recordedCalls, + unorderedEquals([ + ProcessCall('dart', const ['format', 'lib/a.dart'], pluginA.path), + ProcessCall('dart', const ['format', 'lib/b.dart'], pluginB.path), + ]), + ); + }); + + test( + 'does not format staged files that are hand-formatted or in excluded directories when --run-on-staged-packages is used', + () async { + const files = [ + 'lib/a.dart', + 'lib/src/b.dart', + 'example/build/a.dart', + '.dart_tool/a.dart', + ]; + final RepositoryPackage plugin = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: files, + dartConstraint: _dartConstraint, + ); + fakePubGet(plugin); + + // Write pragma to lib/src/b.dart + final p.Context posixContext = p.posix; + childFileWithSubcomponents( + plugin.directory, + posixContext.split('lib/src/b.dart'), + ).writeAsStringSync('// This file is hand-formatted.\ncode...'); + + // Mock git diff to return all of them + final String stagedFiles = files.map((String f) => 'packages/a_plugin/$f').join('\n'); + gitProcessRunner.mockProcessesForExecutable['git-diff'] = List.generate( + 3, + (_) => FakeProcessInfo(MockProcess(stdout: stagedFiles)), + ); + + await runCapturingPrint(runner, ['format', '--run-on-staged-packages']); + + // Only lib/a.dart should be formatted. + // lib/src/b.dart is hand-formatted. + // example/build/a.dart is in excluded dir. + // .dart_tool/a.dart is in excluded dir. + expect( + processRunner.recordedCalls, + orderedEquals([ + ProcessCall('dart', const ['format', 'lib/a.dart'], plugin.path), + ]), + ); + }, + ); + + test( + 'skips Java and Kotlin formatting when no Java or Kotlin files are staged', + () async { + final RepositoryPackage plugin = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: [ + 'lib/a.dart', + 'android/src/main/java/io/flutter/plugins/a_plugin/a.java', + 'android/src/main/kotlin/io/flutter/plugins/a_plugin/b.kt', + ], + dartConstraint: _dartConstraint, + ); + fakePubGet(plugin); + + // Mock git diff to return only the Dart file + const stagedFilePath = 'packages/a_plugin/lib/a.dart'; + gitProcessRunner.mockProcessesForExecutable['git-diff'] = List.generate( + 3, + (_) => FakeProcessInfo(MockProcess(stdout: stagedFilePath)), + ); + + await runCapturingPrint(runner, ['format', '--run-on-staged-packages']); + + // Should only run dart format, no java format + expect( + processRunner.recordedCalls, + orderedEquals([ + ProcessCall('dart', const ['format', 'lib/a.dart'], plugin.path), + ]), + ); + }, + ); + + test( + 'formats only staged Java files and skips Dart when only Java is staged', + () async { + const javaFile = 'android/src/main/java/io/flutter/plugins/a_plugin/a.java'; + final RepositoryPackage plugin = createFakePlugin( + 'a_plugin', + packagesDir, + extraFiles: [ + 'lib/a.dart', + javaFile, + ], + dartConstraint: _dartConstraint, + ); + fakePubGet(plugin); + + // Mock git diff to return only the Java file + const stagedFilePath = 'packages/a_plugin/$javaFile'; + gitProcessRunner.mockProcessesForExecutable['git-diff'] = List.generate( + 3, + (_) => FakeProcessInfo(MockProcess(stdout: stagedFilePath)), + ); + + await runCapturingPrint(runner, ['format', '--run-on-staged-packages']); + + // Should only run java format, no dart format + expect( + processRunner.recordedCalls, + orderedEquals([ + const ProcessCall('java', ['-version'], null), + ProcessCall('java', [ + '-jar', + javaFormatPath, + '--replace', + ...getPackagesDirRelativePaths(plugin, [javaFile]), + ], packagesDir.path), + ]), + ); + }, + ); + test('skips dart if --no-dart flag is provided', () async { const files = ['lib/a.dart']; final RepositoryPackage plugin = createFakePlugin(