Move git_repo_tools and process_fakes outside of clang_tidy. (flutter/engine#46017)

Closes https://github.com/flutter/flutter/issues/134988.

engine/src/flutter/testing/run_tests.py

@@ -1045,6 +1045,25 @@ def gather_engine_repo_tools_tests(build_dir):
     )
 
 
+def gather_git_repo_tools_tests(build_dir):
+  test_dir = os.path.join(
+      BUILDROOT_DIR, 'flutter', 'tools', 'pkg', 'git_repo_tools'
+  )
+  dart_tests = glob.glob('%s/*_test.dart' % test_dir)
+  for dart_test_file in dart_tests:
+    opts = [
+        '--disable-dart-dev',
+        dart_test_file,
+    ]
+    yield EngineExecutableTask(
+        build_dir,
+        os.path.join('dart-sdk', 'bin', 'dart'),
+        None,
+        flags=opts,
+        cwd=test_dir
+    )
+
+
 def gather_api_consistency_tests(build_dir):
   test_dir = os.path.join(BUILDROOT_DIR, 'flutter', 'tools', 'api_check')
   dart_tests = glob.glob('%s/test/*_test.dart' % test_dir)
@@ -1355,6 +1374,7 @@ Flutter Wiki page on the subject: https://github.com/flutter/flutter/wiki/Testin
     tasks += list(gather_build_bucket_golden_scraper_tests(build_dir))
     tasks += list(gather_engine_build_configs_tests(build_dir))
     tasks += list(gather_engine_repo_tools_tests(build_dir))
+    tasks += list(gather_git_repo_tools_tests(build_dir))
     tasks += list(gather_api_consistency_tests(build_dir))
     tasks += list(gather_path_ops_tests(build_dir))
     tasks += list(gather_const_finder_tests(build_dir))

engine/src/flutter/tools/clang_tidy/lib/clang_tidy.dart

@@ -6,13 +6,13 @@ import 'dart:convert' show LineSplitter, jsonDecode;
 import 'dart:io' as io show File, stderr, stdout;
 
 import 'package:engine_repo_tools/engine_repo_tools.dart';
+import 'package:git_repo_tools/git_repo_tools.dart';
 import 'package:meta/meta.dart';
 import 'package:path/path.dart' as path;
 import 'package:process/process.dart';
 import 'package:process_runner/process_runner.dart';
 
 import 'src/command.dart';
-import 'src/git_repo.dart';
 import 'src/options.dart';
 
 const String _linterOutputHeader = '''
@@ -211,7 +211,7 @@ class ClangTidy {
         .toList();
     }
 
-    final GitRepo repo = GitRepo(
+    final GitRepo repo = GitRepo.fromRoot(
       options.repoPath,
       processManager: _processManager,
       verbose: options.verbose,

engine/src/flutter/tools/clang_tidy/pubspec.yaml

@@ -18,6 +18,7 @@ environment:
 
 dependencies:
   args: any
+  git_repo_tools: any
   engine_repo_tools: any
   meta: any
   path: any
@@ -28,6 +29,7 @@ dev_dependencies:
   async_helper: any
   expect: any
   litetest: any
+  process_fakes: any
   smith: any
 
 dependency_overrides:
@@ -45,6 +47,8 @@ dependency_overrides:
     path: ../../../third_party/dart/pkg/expect
   file:
     path: ../../../third_party/pkg/file/packages/file
+  git_repo_tools:
+    path: ../pkg/git_repo_tools
   litetest:
     path: ../../testing/litetest
   meta:
@@ -55,6 +59,8 @@ dependency_overrides:
     path: ../../../third_party/pkg/platform
   process:
     path: ../../../third_party/pkg/process
+  process_fakes:
+    path: ../pkg/process_fakes
   process_runner:
     path: ../../../third_party/pkg/process_runner
   smith:

engine/src/flutter/tools/clang_tidy/test/clang_tidy_test.dart

@@ -11,9 +11,9 @@ import 'package:engine_repo_tools/engine_repo_tools.dart';
 import 'package:litetest/litetest.dart';
 import 'package:path/path.dart' as path;
 import 'package:process/process.dart';
+import 'package:process_fakes/process_fakes.dart';
 import 'package:process_runner/process_runner.dart';
 
-import 'process_fakes.dart';
 
 /// A test fixture for the `clang-tidy` tool.
 final class Fixture {

engine/src/flutter/tools/githooks/pubspec.yaml

@@ -45,6 +45,8 @@ dependency_overrides:
     path: ../../../third_party/dart/pkg/expect
   file:
     path: ../../../third_party/pkg/file/packages/file
+  git_repo_tools:
+    path: ../pkg/git_repo_tools
   litetest:
     path: ../../testing/litetest
   meta:

engine/src/flutter/tools/pkg/git_repo_tools/README.md

@@ -0,0 +1,23 @@
+# `git_repo_tools`
+
+This is a repo-internal library for `flutter/engine`, that contains shared code
+for writing tools that want to interact with the `git` repository. For example,
+finding all changed files in the current branch:
+
+```dart
+import 'dart:io' as io show File, Platform;
+
+import 'package:engine_repo_tools/engine_repo_tools.dart';
+import 'package:git_repo_tools/git_repo_tools.dart';
+import 'package:path/path.dart' as path;
+
+void main() async {
+  // Finds the root of the engine repository from the current script.
+  final Engine engine = Engine.findWithin(path.dirname(path.fromUri(io.Platform.script)));
+  final GitRepo gitRepo = GitRepo(engine.flutterDir);
+
+  for (final io.File file in gitRepo.changedFiles) {
+    print('Changed file: ${file.path}');
+  }
+}
+```

engine/src/flutter/tools/pkg/git_repo_tools/lib/git_repo_tools.dart

@@ -2,58 +2,66 @@
 // 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 show Directory, File;
+import 'dart:io' as io show Directory, File, stdout;
 
 import 'package:path/path.dart' as path;
 import 'package:process/process.dart';
 import 'package:process_runner/process_runner.dart';
 
-/// Utility methods for working with a git repo.
-class GitRepo {
+/// Utility methods for working with a git repository.
+final class GitRepo {
   /// The git repository rooted at `root`.
-  GitRepo(this.root, {
+  GitRepo.fromRoot(this.root, {
     this.verbose = false,
+    StringSink? logSink,
     ProcessManager processManager = const LocalProcessManager(),
-  }) : _processManager = processManager;
+  }) :
+      _processManager = processManager,
+      logSink = logSink ?? io.stdout;
 
   /// Whether to produce verbose log output.
+  ///
+  /// If true, output of git commands will be printed to [logSink].
   final bool verbose;
 
+  /// Where to send verbose log output.
+  ///
+  /// Defaults to [io.stdout].
+  final StringSink logSink;
+
   /// The root of the git repo.
   final io.Directory root;
 
   /// The delegate to use for running processes.
   final ProcessManager _processManager;
 
-  List<io.File>? _changedFiles;
-
   /// Returns a list of all non-deleted files which differ from the nearest
   /// merge-base with `main`. If it can't find a fork point, uses the default
   /// merge-base.
   ///
   /// This is only computed once and cached. Subsequent invocations of the
   /// getter will return the same result.
-  Future<List<io.File>> get changedFiles async =>
-    _changedFiles ??= await _getChangedFiles();
+  late final Future<List<io.File>> changedFiles = _changedFiles();
 
-  List<io.File>? _changedFilesAtHead;
-
-  /// Returns a list of non-deleted files which differ between the HEAD
-  /// commit and its parent.
-  Future<List<io.File>> get changedFilesAtHead async =>
-    _changedFilesAtHead ??= await _getChangedFilesAtHead();
-
-  Future<List<io.File>> _getChangedFiles() async {
+  Future<List<io.File>> _changedFiles() async {
     final ProcessRunner processRunner = ProcessRunner(
       defaultWorkingDirectory: root,
       processManager: _processManager,
     );
     await _fetch(processRunner);
+    // Find the merge base between the current branch and the branch that was
+    // checked out at the time of the last fetch. The merge base is the common
+    // ancestor of the two branches, and the output is the hash of the merge
+    // base.
     ProcessRunnerResult mergeBaseResult = await processRunner.runProcess(
       <String>['git', 'merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'],
       failOk: true,
     );
     if (mergeBaseResult.exitCode != 0) {
+      if (verbose) {
+        logSink.writeln('git merge-base --fork-point failed, using default merge-base');
+        logSink.writeln('Output:\n${mergeBaseResult.stdout}');
+      }
       mergeBaseResult = await processRunner.runProcess(<String>[
         'git',
         'merge-base',
@@ -62,8 +70,7 @@ class GitRepo {
       ]);
     }
     final String mergeBase = mergeBaseResult.stdout.trim();
-    final ProcessRunnerResult masterResult = await processRunner
-        .runProcess(<String>[
+    final ProcessRunnerResult masterResult = await processRunner.runProcess(<String>[
       'git',
       'diff',
       '--name-only',
@@ -73,9 +80,17 @@ class GitRepo {
     return _gitOutputToList(masterResult);
   }
 
-  Future<List<io.File>> _getChangedFilesAtHead() async {
+  /// Returns a list of non-deleted files which differ between the HEAD
+  /// commit and its parent.
+  ///
+  /// This is only computed once and cached. Subsequent invocations of the
+  /// getter will return the same result.
+  late final Future<List<io.File>> changedFilesAtHead = _changedFilesAtHead();
+
+  Future<List<io.File>> _changedFilesAtHead() async {
     final ProcessRunner processRunner = ProcessRunner(
       defaultWorkingDirectory: root,
+      processManager: _processManager,
     );
     await _fetch(processRunner);
     final ProcessRunnerResult diffTreeResult = await processRunner.runProcess(
@@ -98,6 +113,10 @@ class GitRepo {
       failOk: true,
     );
     if (fetchResult.exitCode != 0) {
+      if (verbose) {
+        logSink.writeln('git fetch upstream main failed, using origin main');
+        logSink.writeln('Output:\n${fetchResult.stdout}');
+      }
       await processRunner.runProcess(<String>[
         'git',
         'fetch',
@@ -110,7 +129,7 @@ class GitRepo {
   List<io.File> _gitOutputToList(ProcessRunnerResult result) {
     final String diffOutput = result.stdout.trim();
     if (verbose) {
-      print('git diff output:\n$diffOutput');
+      logSink.writeln('git diff output:\n$diffOutput');
     }
     final Set<String> resultMap = <String>{};
     resultMap.addAll(diffOutput.split('\n').where(

engine/src/flutter/tools/pkg/git_repo_tools/pubspec.yaml

@@ -0,0 +1,60 @@
+# 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.
+
+name: git_repo_tools
+publish_to: none
+environment:
+  sdk: '>=3.2.0-0 <4.0.0'
+
+# Do not add any dependencies that require more than what is provided in
+# //third_party/pkg, //third_party/dart/pkg, or
+# //third_party/dart/third_party/pkg. In particular, package:test is not usable
+# here.
+
+# If you do add packages here, make sure you can run `pub get --offline`, and
+# check the .packages and .package_config to make sure all the paths are
+# relative to this directory into //third_party/dart
+
+dependencies:
+  meta: any
+  path: any
+  process: any
+  process_runner: any
+
+dev_dependencies:
+  async_helper: any
+  expect: any
+  litetest: any
+  process_fakes: any
+  smith: any
+
+dependency_overrides:
+  args:
+    path: ../../../../third_party/dart/third_party/pkg/args
+  async:
+    path: ../../../../third_party/dart/third_party/pkg/async
+  async_helper:
+    path: ../../../../third_party/dart/pkg/async_helper
+  collection:
+    path: ../../../../third_party/dart/third_party/pkg/collection
+  expect:
+    path: ../../../../third_party/dart/pkg/expect
+  file:
+    path: ../../../../third_party/pkg/file/packages/file
+  litetest:
+    path: ../../../testing/litetest
+  meta:
+    path: ../../../../third_party/dart/pkg/meta
+  path:
+    path: ../../../../third_party/dart/third_party/pkg/path
+  platform:
+    path: ../../../../third_party/pkg/platform
+  process:
+    path: ../../../../third_party/pkg/process
+  process_fakes:
+    path: ../process_fakes
+  process_runner:
+    path: ../../../../third_party/pkg/process_runner
+  smith:
+    path: ../../../../third_party/dart/pkg/smith

engine/src/flutter/tools/pkg/git_repo_tools/test/git_repo_tools_test.dart

@@ -0,0 +1,215 @@
+// 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:git_repo_tools/git_repo_tools.dart';
+import 'package:litetest/litetest.dart';
+import 'package:process_fakes/process_fakes.dart';
+
+void main() {
+  const String fakeShaHash = 'fake-sha-hash';
+
+
+  test('returns non-deleted files which differ from merge-base with main', () async {
+    final Fixture fixture = Fixture(
+      processManager: FakeProcessManager(
+        onStart: (List<String> command) {
+          // Succeed calling "git merge-base --fork-point FETCH_HEAD HEAD".
+          if (command.join(' ').startsWith('git merge-base --fork-point')) {
+            return FakeProcess(stdout: fakeShaHash);
+          }
+
+          // Succeed calling "git fetch upstream main".
+          if (command.join(' ') == 'git fetch upstream main') {
+            return FakeProcess();
+          }
+
+          // Succeed calling "git diff --name-only --diff-filter=ACMRT fake-sha-hash".
+          if (command.join(' ') == 'git diff --name-only --diff-filter=ACMRT $fakeShaHash') {
+            return FakeProcess(stdout: 'file1\nfile2');
+          }
+
+          // Otherwise, fail.
+          return FakeProcessManager.unhandledStart(command);
+        },
+      ),
+    );
+
+    try {
+      final List<io.File> changedFiles = await fixture.gitRepo.changedFiles;
+      expect(changedFiles, hasLength(2));
+      expect(changedFiles[0].path, endsWith('file1'));
+      expect(changedFiles[1].path, endsWith('file2'));
+    } finally {
+      fixture.gitRepo.root.deleteSync(recursive: true);
+    }
+  });
+
+  test('returns non-deleted files which differ from default merge-base', () async {
+    final Fixture fixture = Fixture(
+      processManager: FakeProcessManager(
+        onStart: (List<String> command) {
+          if (command.join(' ').startsWith('git merge-base --fork-point')) {
+            return FakeProcess(exitCode: 1);
+          }
+
+          if (command.join(' ').startsWith('git merge-base')) {
+            return FakeProcess(stdout: fakeShaHash);
+          }
+
+          if (command.join(' ') == 'git fetch upstream main') {
+            return FakeProcess();
+          }
+
+          if (command.join(' ') == 'git diff --name-only --diff-filter=ACMRT $fakeShaHash') {
+            return FakeProcess(stdout: 'file1\nfile2');
+          }
+
+          // Otherwise, fail.
+          return FakeProcessManager.unhandledStart(command);
+        },
+      ),
+    );
+
+    try {
+      final List<io.File> changedFiles = await fixture.gitRepo.changedFiles;
+      expect(changedFiles, hasLength(2));
+      expect(changedFiles[0].path, endsWith('file1'));
+      expect(changedFiles[1].path, endsWith('file2'));
+    } finally {
+      fixture.gitRepo.root.deleteSync(recursive: true);
+    }
+  });
+
+  test('returns non-deleted files which differ from HEAD', () async {
+    final Fixture fixture = Fixture(
+      processManager: FakeProcessManager(
+        onStart: (List<String> command) {
+          if (command.join(' ') == 'git fetch upstream main') {
+            return FakeProcess();
+          }
+
+          if (command.join(' ') == 'git diff-tree --no-commit-id --name-only --diff-filter=ACMRT -r HEAD') {
+            return FakeProcess(stdout: 'file1\nfile2');
+          }
+
+          // Otherwise, fail.
+          return FakeProcessManager.unhandledStart(command);
+        },
+      ),
+    );
+
+    try {
+      final List<io.File> changedFiles = await fixture.gitRepo.changedFilesAtHead;
+      expect(changedFiles, hasLength(2));
+      expect(changedFiles[0].path, endsWith('file1'));
+      expect(changedFiles[1].path, endsWith('file2'));
+    } finally {
+      fixture.gitRepo.root.deleteSync(recursive: true);
+    }
+  });
+
+  test('returns non-deleted files which differ from HEAD when merge-base fails', () async {
+    final Fixture fixture = Fixture(
+      processManager: FakeProcessManager(
+        onStart: (List<String> command) {
+          if (command.join(' ') == 'git fetch upstream main') {
+            return FakeProcess();
+          }
+
+          if (command.join(' ') == 'git diff-tree --no-commit-id --name-only --diff-filter=ACMRT -r HEAD') {
+            return FakeProcess(stdout: 'file1\nfile2');
+          }
+
+          if (command.join(' ').startsWith('git merge-base --fork-point')) {
+            return FakeProcess(exitCode: 1);
+          }
+
+          if (command.join(' ').startsWith('git merge-base')) {
+            return FakeProcess(stdout: fakeShaHash);
+          }
+
+          // Otherwise, fail.
+          return FakeProcessManager.unhandledStart(command);
+        },
+      ),
+    );
+
+    try {
+      final List<io.File> changedFiles = await fixture.gitRepo.changedFilesAtHead;
+      expect(changedFiles, hasLength(2));
+      expect(changedFiles[0].path, endsWith('file1'));
+      expect(changedFiles[1].path, endsWith('file2'));
+    } finally {
+      fixture.gitRepo.root.deleteSync(recursive: true);
+    }
+  });
+
+  test('verbose output is captured', () async {
+    final Fixture fixture = Fixture(
+      processManager: FakeProcessManager(
+        onStart: (List<String> command) {
+          if (command.join(' ').startsWith('git merge-base --fork-point')) {
+            return FakeProcess(exitCode: 1);
+          }
+
+          if (command.join(' ').startsWith('git merge-base')) {
+            return FakeProcess(stdout: fakeShaHash);
+          }
+
+          if (command.join(' ') == 'git fetch upstream main') {
+            return FakeProcess();
+          }
+
+          if (command.join(' ') == 'git diff --name-only --diff-filter=ACMRT $fakeShaHash') {
+            return FakeProcess(stdout: 'file1\nfile2');
+          }
+
+          // Otherwise, fail.
+          return FakeProcessManager.unhandledStart(command);
+        },
+      ),
+      verbose: true,
+    );
+
+    try {
+      await fixture.gitRepo.changedFiles;
+      expect(fixture.logSink.toString(), contains('git merge-base --fork-point failed, using default merge-base'));
+      expect(fixture.logSink.toString(), contains('git diff output:\nfile1\nfile2'));
+    } finally {
+      fixture.gitRepo.root.deleteSync(recursive: true);
+    }
+  });
+}
+
+final class Fixture {
+  factory Fixture({
+    FakeProcessManager? processManager,
+    bool verbose = false,
+  }) {
+    final io.Directory root = io.Directory.systemTemp.createTempSync('git_repo_tools.test');
+    final StringBuffer logSink = StringBuffer();
+    processManager ??= FakeProcessManager();
+    return Fixture._(
+      gitRepo: GitRepo.fromRoot(root,
+        logSink: logSink,
+        processManager: processManager,
+        verbose: verbose,
+      ),
+      logSink: logSink,
+      processManager: processManager,
+    );
+  }
+
+  const Fixture._({
+    required this.gitRepo,
+    required this.logSink,
+    required this.processManager,
+  });
+
+  final GitRepo gitRepo;
+  final StringBuffer logSink;
+  final FakeProcessManager processManager;
+}

engine/src/flutter/tools/pkg/process_fakes/README.md

@@ -0,0 +1,7 @@
+# `process_fakes`
+
+Fake implementations of `Process` and `ProcessManager` for testing.
+
+This is not a great package, and is the bare minimum needed for fairly basic
+tooling that uses `ProcessManager`. If we ever need a more complete solution
+we should look at upstreaming [`flutter_tools/.../fake_proecss_manager.dart`](https://github.com/flutter/flutter/blob/a9183f696c8e12617d05a26b0b5e80035e515f2a/packages/flutter_tools/test/src/fake_process_manager.dart#L223)

engine/src/flutter/tools/pkg/process_fakes/lib/process_fakes.dart

@@ -9,15 +9,20 @@ import 'package:process/process.dart';
 
 /// A fake implementation of [ProcessManager] that allows control for testing.
 final class FakeProcessManager implements ProcessManager {
+  /// Creates a fake process manager delegates to [onRun] and [onStart].
+  ///
+  /// If either is not provided, it throws an [UnsupportedError] when called.
   FakeProcessManager({
     io.ProcessResult Function(List<String> command) onRun = unhandledRun,
     io.Process Function(List<String> command) onStart = unhandledStart,
   }) : _onRun = onRun, _onStart = onStart;
 
+  /// A default implementation of [onRun] that throws an [UnsupportedError].
   static io.ProcessResult unhandledRun(List<String> command) {
     throw UnsupportedError('Unhandled run: ${command.join(' ')}');
   }
 
+  /// A default implementation of [onStart] that throws an [UnsupportedError].
   static io.Process unhandledStart(List<String> command) {
     throw UnsupportedError('Unhandled start: ${command.join(' ')}');
   }

engine/src/flutter/tools/pkg/process_fakes/pubspec.yaml

@@ -0,0 +1,36 @@
+# 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.
+
+name: process_fakes
+publish_to: none
+environment:
+  sdk: '>=3.2.0-0 <4.0.0'
+
+# Do not add any dependencies that require more than what is provided in
+# //third_party/pkg, //third_party/dart/pkg, or
+# //third_party/dart/third_party/pkg. In particular, package:test is not usable
+# here.
+
+# If you do add packages here, make sure you can run `pub get --offline`, and
+# check the .packages and .package_config to make sure all the paths are
+# relative to this directory into //third_party/dart
+
+dependencies:
+  file: any
+  meta: any
+  path: any
+  platform: any
+  process: any
+
+dependency_overrides:
+  file:
+    path: ../../../../third_party/pkg/file/packages/file
+  meta:
+    path: ../../../../third_party/dart/pkg/meta
+  path:
+    path: ../../../../third_party/dart/third_party/pkg/path
+  platform:
+    path: ../../../../third_party/pkg/platform
+  process:
+    path: ../../../../third_party/pkg/process

engine/src/flutter/tools/pub_get_offline.py

@@ -44,6 +44,8 @@ ALL_PACKAGES = [
     os.path.join(ENGINE_DIR, 'tools', 'path_ops', 'dart'),
     os.path.join(ENGINE_DIR, 'tools', 'pkg', 'engine_build_configs'),
     os.path.join(ENGINE_DIR, 'tools', 'pkg', 'engine_repo_tools'),
+    os.path.join(ENGINE_DIR, 'tools', 'pkg', 'git_repo_tools'),
+    os.path.join(ENGINE_DIR, 'tools', 'pkg', 'process_fakes'),
 ]