From 44daf7893eabceaf3156ccae8527e449b3dca6e6 Mon Sep 17 00:00:00 2001
From: Stuart Morgan <stuartmorgan@google.com>
Date: Fri, 25 Jun 2021 16:01:59 -0400
Subject: [PATCH 1/6] Factor out some helpers

---
 script/tool/lib/src/common/core.dart          | 10 ++
 .../src/common/package_looping_command.dart   |  3 +
 .../tool/lib/src/common/plugin_command.dart   | 43 ++++----
 .../tool/lib/src/publish_plugin_command.dart  | 12 +--
 .../tool/lib/src/pubspec_check_command.dart   |  1 -
 .../tool/lib/src/version_check_command.dart   | 97 ++++++++++++-------
 ...t.dart => version_check_command_test.dart} |  5 +-
 7 files changed, 104 insertions(+), 67 deletions(-)
 rename script/tool/test/{version_check_test.dart => version_check_command_test.dart} (99%)

diff --git a/script/tool/lib/src/common/core.dart b/script/tool/lib/src/common/core.dart
index b2be8f56d172..3b07baf5dc1e 100644
--- a/script/tool/lib/src/common/core.dart
+++ b/script/tool/lib/src/common/core.dart
@@ -58,6 +58,16 @@ void printSuccess(String successMessage) {
   print(Colorize(successMessage)..green());
 }
 
+/// Prints `warningMessage` in yellow.
+///
+/// Warnings are not surfaced in CI summaries, so this is only useful for
+/// highlighting something when someone is already looking though the log
+/// messages. DO NOT RELY on someone noticing a warning; instead, use it for
+/// things that might be useful to someone debugging an unexpected result.
+void printWarning(String warningMessage) {
+  print(Colorize(warningMessage)..yellow());
+}
+
 /// Prints `errorMessage` in red.
 void printError(String errorMessage) {
   print(Colorize(errorMessage)..red());
diff --git a/script/tool/lib/src/common/package_looping_command.dart b/script/tool/lib/src/common/package_looping_command.dart
index 1349a5ed5dcc..c6b7c2a0c501 100644
--- a/script/tool/lib/src/common/package_looping_command.dart
+++ b/script/tool/lib/src/common/package_looping_command.dart
@@ -99,6 +99,9 @@ abstract class PackageLoopingCommand extends PluginCommand {
     return packageName;
   }
 
+  /// The suggested indentation for printed output.
+  String get indentation => hasLongOutput ? '' : '  ';
+
   // ----------------------------------------
 
   @override
diff --git a/script/tool/lib/src/common/plugin_command.dart b/script/tool/lib/src/common/plugin_command.dart
index 4c095858e45a..c146b1aba904 100644
--- a/script/tool/lib/src/common/plugin_command.dart
+++ b/script/tool/lib/src/common/plugin_command.dart
@@ -20,8 +20,8 @@ abstract class PluginCommand extends Command<void> {
   PluginCommand(
     this.packagesDir, {
     this.processRunner = const ProcessRunner(),
-    this.gitDir,
-  }) {
+    GitDir? gitDir,
+  }) : _gitDir = gitDir {
     argParser.addMultiOption(
       _pluginsArg,
       splitCommas: true,
@@ -76,10 +76,11 @@ abstract class PluginCommand extends Command<void> {
   /// This can be overridden for testing.
   final ProcessRunner processRunner;
 
-  /// The git directory to use. By default it uses the parent directory.
+  /// The git directory to use. If unset, [getGitDir] uses the packages
+  /// directory's enclosing repository.
   ///
   /// This can be mocked for testing.
-  final GitDir? gitDir;
+  GitDir? _gitDir;
 
   int? _shardIndex;
   int? _shardCount;
@@ -100,6 +101,26 @@ abstract class PluginCommand extends Command<void> {
     return _shardCount!;
   }
 
+  /// Returns the [GitDir] containing [packagesDir].
+  Future<GitDir> getGitDir() async {
+    GitDir? gitDir = _gitDir;
+    if (gitDir != null) {
+      return gitDir;
+    }
+
+    // Ensure there are no symlinks in the path, as it can break
+    // GitDir's allowSubdirectory:true.
+    final String packagesPath = packagesDir.resolveSymbolicLinksSync();
+    if (!await GitDir.isGitDir(packagesPath)) {
+      printError('$packagesPath is not a valid Git repository.');
+      throw ToolExit(2);
+    }
+    gitDir =
+        await GitDir.fromExisting(packagesDir.path, allowSubdirectory: true);
+    _gitDir = gitDir;
+    return gitDir;
+  }
+
   /// Convenience accessor for boolean arguments.
   bool getBoolArg(String key) {
     return (argResults![key] as bool?) ?? false;
@@ -285,22 +306,10 @@ abstract class PluginCommand extends Command<void> {
   ///
   /// Throws tool exit if [gitDir] nor root directory is a git directory.
   Future<GitVersionFinder> retrieveVersionFinder() async {
-    final String rootDir = packagesDir.parent.absolute.path;
     final String baseSha = getStringArg(_kBaseSha);
 
-    GitDir? baseGitDir = gitDir;
-    if (baseGitDir == null) {
-      if (!await GitDir.isGitDir(rootDir)) {
-        printError(
-          '$rootDir is not a valid Git repository.',
-        );
-        throw ToolExit(2);
-      }
-      baseGitDir = await GitDir.fromExisting(rootDir);
-    }
-
     final GitVersionFinder gitVersionFinder =
-        GitVersionFinder(baseGitDir, baseSha);
+        GitVersionFinder(await getGitDir(), baseSha);
     return gitVersionFinder;
   }
 
diff --git a/script/tool/lib/src/publish_plugin_command.dart b/script/tool/lib/src/publish_plugin_command.dart
index 622a1a3cb133..6c4076ca9b8f 100644
--- a/script/tool/lib/src/publish_plugin_command.dart
+++ b/script/tool/lib/src/publish_plugin_command.dart
@@ -149,15 +149,7 @@ class PublishPluginCommand extends PluginCommand {
     }
 
     _print('Checking local repo...');
-    // Ensure there are no symlinks in the path, as it can break
-    // GitDir's allowSubdirectory:true.
-    final String packagesPath = packagesDir.resolveSymbolicLinksSync();
-    if (!await GitDir.isGitDir(packagesPath)) {
-      _print('$packagesPath is not a valid Git repository.');
-      throw ToolExit(1);
-    }
-    final GitDir baseGitDir = gitDir ??
-        await GitDir.fromExisting(packagesPath, allowSubdirectory: true);
+    final GitDir gitDir = await getGitDir();
 
     final bool shouldPushTag = getBoolArg(_pushTagsOption);
     _RemoteInfo? remote;
@@ -179,7 +171,7 @@ class PublishPluginCommand extends PluginCommand {
     bool successful;
     if (publishAllChanged) {
       successful = await _publishAllChangedPackages(
-        baseGitDir: baseGitDir,
+        baseGitDir: gitDir,
         remoteForTagPush: remote,
       );
     } else {
diff --git a/script/tool/lib/src/pubspec_check_command.dart b/script/tool/lib/src/pubspec_check_command.dart
index 44b6b061542c..d257638971db 100644
--- a/script/tool/lib/src/pubspec_check_command.dart
+++ b/script/tool/lib/src/pubspec_check_command.dart
@@ -70,7 +70,6 @@ class PubspecCheckCommand extends PackageLoopingCommand {
     File pubspecFile, {
     required String packageName,
   }) async {
-    const String indentation = '  ';
     final String contents = pubspecFile.readAsStringSync();
     final Pubspec? pubspec = _tryParsePubspec(contents);
     if (pubspec == null) {
diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart
index 5e9f55333f8e..7d01f02e30f7 100644
--- a/script/tool/lib/src/version_check_command.dart
+++ b/script/tool/lib/src/version_check_command.dart
@@ -6,6 +6,7 @@ import 'package:file/file.dart';
 import 'package:git/git.dart';
 import 'package:http/http.dart' as http;
 import 'package:meta/meta.dart';
+import 'package:path/path.dart' as p;
 import 'package:pub_semver/pub_semver.dart';
 import 'package:pubspec_parse/pubspec_parse.dart';
 
@@ -102,6 +103,8 @@ class VersionCheckCommand extends PluginCommand {
 
   final PubVersionFinder _pubVersionFinder;
 
+  String get indentation => '  ';
+
   @override
   Future<void> run() async {
     final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
@@ -111,7 +114,6 @@ class VersionCheckCommand extends PluginCommand {
 
     final List<String> badVersionChangePubspecs = <String>[];
 
-    const String indentation = '  ';
     for (final String pubspecPath in changedPubspecs) {
       print('Checking versions for $pubspecPath...');
       final File pubspecFile = packagesDir.fileSystem.file(pubspecPath);
@@ -136,40 +138,26 @@ class VersionCheckCommand extends PluginCommand {
       }
       Version? sourceVersion;
       if (getBoolArg(_againstPubFlag)) {
-        final String packageName = pubspecFile.parent.basename;
-        final PubVersionFinderResponse pubVersionFinderResponse =
-            await _pubVersionFinder.getPackageVersion(package: packageName);
-        switch (pubVersionFinderResponse.result) {
-          case PubVersionFinderResult.success:
-            sourceVersion = pubVersionFinderResponse.versions.first;
-            print(
-                '$indentation$packageName: Current largest version on pub: $sourceVersion');
-            break;
-          case PubVersionFinderResult.fail:
-            printError('''
-${indentation}Error fetching version on pub for $packageName.
-${indentation}HTTP Status ${pubVersionFinderResponse.httpResponse.statusCode}
-${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
-''');
-            badVersionChangePubspecs.add(pubspecPath);
-            continue;
-          case PubVersionFinderResult.noPackageFound:
-            sourceVersion = null;
-            break;
+        sourceVersion =
+            await _getPreviousVersionFromPub(pubspec.name, pubspecFile);
+        if (sourceVersion == null) {
+          badVersionChangePubspecs.add(pubspecPath);
+          continue;
+        }
+        if (sourceVersion != Version.none) {
+          print(
+              '$indentation${pubspec.name}: Current largest version on pub: $sourceVersion');
         }
       } else {
-        sourceVersion = await gitVersionFinder.getPackageVersion(pubspecPath);
+        sourceVersion = await _getPreviousVersionFromGit(pubspecFile,
+                gitVersionFinder: gitVersionFinder) ??
+            Version.none;
       }
-      if (sourceVersion == null) {
-        String safeToIgnoreMessage;
-        if (getBoolArg(_againstPubFlag)) {
-          safeToIgnoreMessage =
-              '${indentation}Unable to find package on pub server.';
-        } else {
-          safeToIgnoreMessage =
-              '${indentation}Unable to find pubspec in master.';
-        }
-        print('$safeToIgnoreMessage Safe to ignore if the project is new.');
+      if (sourceVersion == Version.none) {
+        print('${indentation}Unable to find previous version '
+            '${getBoolArg(_againstPubFlag) ? 'on pub server' : 'at git base'}.');
+        printWarning(
+            '${indentation}If this plugin is not new, something has gone wrong.');
         continue;
       }
 
@@ -251,16 +239,51 @@ $indentation${mismatchedVersionPlugins.join('\n$indentation')}
     print('No version check errors found!');
   }
 
+  /// Returns the previous published version of [package].
+  ///
+  /// [packageName] must be the actual name of the package as published (i.e.,
+  /// the name from pubspec.yaml, not the on disk name if different.)
+  Future<Version?> _getPreviousVersionFromPub(String packageName,
+      File pubspec // XXX change to package; get pubspec file internally.
+      ) async {
+    final PubVersionFinderResponse pubVersionFinderResponse =
+        await _pubVersionFinder.getPackageVersion(package: packageName);
+    switch (pubVersionFinderResponse.result) {
+      case PubVersionFinderResult.success:
+        return pubVersionFinderResponse.versions.first;
+      case PubVersionFinderResult.fail:
+        printError('''
+${indentation}Error fetching version on pub for $packageName.
+${indentation}HTTP Status ${pubVersionFinderResponse.httpResponse.statusCode}
+${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
+''');
+        return null;
+      case PubVersionFinderResult.noPackageFound:
+        return Version.none;
+    }
+  }
+
+  /// Returns the version of [package] from git at the base comparison hash.
+  Future<Version?> _getPreviousVersionFromGit(
+    File pubspec // XXX change to package; get pubspec file internally.
+    , {
+    required GitVersionFinder gitVersionFinder,
+  }) async {
+    final GitDir gitDir = await getGitDir();
+    return await gitVersionFinder.getPackageVersion(
+        p.relative(pubspec.absolute.path, from: gitDir.path));
+  }
+
   /// Returns whether or not the pubspec version and CHANGELOG version for
   /// [plugin] match.
-  Future<bool> _checkVersionsMatch(Directory plugin) async {
+  Future<bool> _checkVersionsMatch(Directory package) async {
     // get version from pubspec
-    final String packageName = plugin.basename;
+    final String packageName = package.basename;
     print('-----------------------------------------');
     print(
         'Checking the first version listed in CHANGELOG.md matches the version in pubspec.yaml for $packageName.');
 
-    final Pubspec? pubspec = _tryParsePubspec(plugin);
+    final Pubspec? pubspec = _tryParsePubspec(package);
     if (pubspec == null) {
       printError('Cannot parse version from pubspec.yaml');
       return false;
@@ -268,7 +291,7 @@ $indentation${mismatchedVersionPlugins.join('\n$indentation')}
     final Version? fromPubspec = pubspec.version;
 
     // get first version from CHANGELOG
-    final File changelog = plugin.childFile('CHANGELOG.md');
+    final File changelog = package.childFile('CHANGELOG.md');
     final List<String> lines = changelog.readAsLinesSync();
     String? firstLineWithText;
     final Iterator<String> iterator = lines.iterator;
@@ -301,7 +324,7 @@ $indentation${mismatchedVersionPlugins.join('\n$indentation')}
         versionString == null ? null : Version.parse(versionString);
     if (fromChangeLog == null) {
       printError(
-          'Cannot find version on the first line of ${plugin.path}/CHANGELOG.md');
+          'Cannot find version on the first line of ${package.path}/CHANGELOG.md');
       return false;
     }
 
diff --git a/script/tool/test/version_check_test.dart b/script/tool/test/version_check_command_test.dart
similarity index 99%
rename from script/tool/test/version_check_test.dart
rename to script/tool/test/version_check_command_test.dart
index 6035360a221c..906712421ba8 100644
--- a/script/tool/test/version_check_test.dart
+++ b/script/tool/test/version_check_command_test.dart
@@ -69,6 +69,7 @@ void main() {
       gitDiffResponse = '';
       gitShowResponses = <String, String>{};
       gitDir = MockGitDir();
+      when(gitDir.path).thenReturn(packagesDir.parent.path);
       when(gitDir.runCommand(any, throwOnError: anyNamed('throwOnError')))
           .thenAnswer((Invocation invocation) {
         gitDirCommands.add(invocation.positionalArguments[0] as List<String>);
@@ -183,7 +184,7 @@ void main() {
       expect(
         output,
         containsAllInOrder(<String>[
-          '${indentation}Unable to find pubspec in master. Safe to ignore if the project is new.',
+          '${indentation}Unable to find previous version at git base.',
           'No version check errors found!',
         ]),
       );
@@ -704,7 +705,7 @@ ${indentation}HTTP response: xx
       expect(
         result,
         containsAllInOrder(<String>[
-          '${indentation}Unable to find package on pub server. Safe to ignore if the project is new.',
+          '${indentation}Unable to find previous version on pub server.',
           'No version check errors found!',
         ]),
       );

From d1f2eb1188a25fd4db26c1946771b83951a6ee0a Mon Sep 17 00:00:00 2001
From: Stuart Morgan <stuartmorgan@google.com>
Date: Fri, 25 Jun 2021 16:37:50 -0400
Subject: [PATCH 2/6] Switch to package-based iteration, and use version from
 disk rather than git:HEAD

---
 .../tool/lib/src/version_check_command.dart   |  87 ++++++-------
 .../tool/test/version_check_command_test.dart | 123 +++---------------
 2 files changed, 61 insertions(+), 149 deletions(-)

diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart
index 7d01f02e30f7..d905aa9a5351 100644
--- a/script/tool/lib/src/version_check_command.dart
+++ b/script/tool/lib/src/version_check_command.dart
@@ -32,39 +32,40 @@ enum NextVersionType {
 }
 
 /// Returns the set of allowed next versions, with their change type, for
-/// [masterVersion].
+/// [version].
 ///
-/// [headVerison] is used to check whether this is a pre-1.0 version bump, as
+/// [newVersion] is used to check whether this is a pre-1.0 version bump, as
 /// those have different semver rules.
 @visibleForTesting
 Map<Version, NextVersionType> getAllowedNextVersions(
-    {required Version masterVersion, required Version headVersion}) {
+  Version version, {
+  required Version newVersion,
+}) {
   final Map<Version, NextVersionType> allowedNextVersions =
       <Version, NextVersionType>{
-    masterVersion.nextMajor: NextVersionType.BREAKING_MAJOR,
-    masterVersion.nextMinor: NextVersionType.MINOR,
-    masterVersion.nextPatch: NextVersionType.PATCH,
+    version.nextMajor: NextVersionType.BREAKING_MAJOR,
+    version.nextMinor: NextVersionType.MINOR,
+    version.nextPatch: NextVersionType.PATCH,
   };
 
-  if (masterVersion.major < 1 && headVersion.major < 1) {
+  if (version.major < 1 && newVersion.major < 1) {
     int nextBuildNumber = -1;
-    if (masterVersion.build.isEmpty) {
+    if (version.build.isEmpty) {
       nextBuildNumber = 1;
     } else {
-      final int currentBuildNumber = masterVersion.build.first as int;
+      final int currentBuildNumber = version.build.first as int;
       nextBuildNumber = currentBuildNumber + 1;
     }
     final Version preReleaseVersion = Version(
-      masterVersion.major,
-      masterVersion.minor,
-      masterVersion.patch,
+      version.major,
+      version.minor,
+      version.patch,
       build: nextBuildNumber.toString(),
     );
     allowedNextVersions.clear();
-    allowedNextVersions[masterVersion.nextMajor] = NextVersionType.RELEASE;
-    allowedNextVersions[masterVersion.nextMinor] =
-        NextVersionType.BREAKING_MAJOR;
-    allowedNextVersions[masterVersion.nextPatch] = NextVersionType.MINOR;
+    allowedNextVersions[version.nextMajor] = NextVersionType.RELEASE;
+    allowedNextVersions[version.nextMinor] = NextVersionType.BREAKING_MAJOR;
+    allowedNextVersions[version.nextPatch] = NextVersionType.MINOR;
     allowedNextVersions[preReleaseVersion] = NextVersionType.PATCH;
   }
   return allowedNextVersions;
@@ -107,53 +108,48 @@ class VersionCheckCommand extends PluginCommand {
 
   @override
   Future<void> run() async {
+    final GitDir gitDir = await getGitDir();
     final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
 
-    final List<String> changedPubspecs =
-        await gitVersionFinder.getChangedPubSpecs();
-
     final List<String> badVersionChangePubspecs = <String>[];
 
-    for (final String pubspecPath in changedPubspecs) {
+    await for (final Directory package in getPackages()) {
+      final File pubspecFile = package.childFile('pubspec.yaml');
+      final String pubspecPath =
+          p.relative(pubspecFile.absolute.path, from: gitDir.path);
       print('Checking versions for $pubspecPath...');
-      final File pubspecFile = packagesDir.fileSystem.file(pubspecPath);
-      if (!pubspecFile.existsSync()) {
-        print('${indentation}Deleted; skipping.');
-        continue;
-      }
       final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync());
       if (pubspec.publishTo == 'none') {
         print('${indentation}Found "publish_to: none"; skipping.');
         continue;
       }
 
-      final Version? headVersion =
-          await gitVersionFinder.getPackageVersion(pubspecPath, gitRef: 'HEAD');
-      if (headVersion == null) {
+      final Version? currentVersion = pubspec.version;
+      if (currentVersion == null) {
         printError('${indentation}No version found. A package that '
             'intentionally has no version should be marked '
             '"publish_to: none".');
         badVersionChangePubspecs.add(pubspecPath);
         continue;
       }
-      Version? sourceVersion;
+      Version? previousVersion;
       if (getBoolArg(_againstPubFlag)) {
-        sourceVersion =
+        previousVersion =
             await _getPreviousVersionFromPub(pubspec.name, pubspecFile);
-        if (sourceVersion == null) {
+        if (previousVersion == null) {
           badVersionChangePubspecs.add(pubspecPath);
           continue;
         }
-        if (sourceVersion != Version.none) {
+        if (previousVersion != Version.none) {
           print(
-              '$indentation${pubspec.name}: Current largest version on pub: $sourceVersion');
+              '$indentation${pubspec.name}: Current largest version on pub: $previousVersion');
         }
       } else {
-        sourceVersion = await _getPreviousVersionFromGit(pubspecFile,
+        previousVersion = await _getPreviousVersionFromGit(pubspecFile,
                 gitVersionFinder: gitVersionFinder) ??
             Version.none;
       }
-      if (sourceVersion == Version.none) {
+      if (previousVersion == Version.none) {
         print('${indentation}Unable to find previous version '
             '${getBoolArg(_againstPubFlag) ? 'on pub server' : 'at git base'}.');
         printWarning(
@@ -161,20 +157,19 @@ class VersionCheckCommand extends PluginCommand {
         continue;
       }
 
-      if (sourceVersion == headVersion) {
+      if (previousVersion == currentVersion) {
         print('${indentation}No version change.');
         continue;
       }
 
       // Check for reverts when doing local validation.
-      if (!getBoolArg(_againstPubFlag) && headVersion < sourceVersion) {
+      if (!getBoolArg(_againstPubFlag) && currentVersion < previousVersion) {
         final Map<Version, NextVersionType> possibleVersionsFromNewVersion =
-            getAllowedNextVersions(
-                masterVersion: headVersion, headVersion: sourceVersion);
+            getAllowedNextVersions(currentVersion, newVersion: previousVersion);
         // Since this skips validation, try to ensure that it really is likely
         // to be a revert rather than a typo by checking that the transition
         // from the lower version to the new version would have been valid.
-        if (possibleVersionsFromNewVersion.containsKey(sourceVersion)) {
+        if (possibleVersionsFromNewVersion.containsKey(previousVersion)) {
           print('${indentation}New version is lower than previous version. '
               'This is assumed to be a revert.');
           continue;
@@ -182,24 +177,24 @@ class VersionCheckCommand extends PluginCommand {
       }
 
       final Map<Version, NextVersionType> allowedNextVersions =
-          getAllowedNextVersions(
-              masterVersion: sourceVersion, headVersion: headVersion);
+          getAllowedNextVersions(previousVersion, newVersion: currentVersion);
 
-      if (!allowedNextVersions.containsKey(headVersion)) {
+      if (!allowedNextVersions.containsKey(currentVersion)) {
         final String source = (getBoolArg(_againstPubFlag)) ? 'pub' : 'master';
         printError('${indentation}Incorrectly updated version.\n'
-            '${indentation}HEAD: $headVersion, $source: $sourceVersion.\n'
+            '${indentation}HEAD: $currentVersion, $source: $previousVersion.\n'
             '${indentation}Allowed versions: $allowedNextVersions');
         badVersionChangePubspecs.add(pubspecPath);
         continue;
       } else {
-        print('$indentation$headVersion -> $sourceVersion');
+        print('$indentation$currentVersion -> $previousVersion');
       }
 
       final bool isPlatformInterface =
           pubspec.name.endsWith('_platform_interface');
       if (isPlatformInterface &&
-          allowedNextVersions[headVersion] == NextVersionType.BREAKING_MAJOR) {
+          allowedNextVersions[currentVersion] ==
+              NextVersionType.BREAKING_MAJOR) {
         printError('$pubspecPath breaking change detected.\n'
             'Breaking changes to platform interfaces are strongly discouraged.\n');
         badVersionChangePubspecs.add(pubspecPath);
diff --git a/script/tool/test/version_check_command_test.dart b/script/tool/test/version_check_command_test.dart
index 906712421ba8..ae56f0b72218 100644
--- a/script/tool/test/version_check_command_test.dart
+++ b/script/tool/test/version_check_command_test.dart
@@ -29,7 +29,7 @@ void testAllowedVersion(
   final Version master = Version.parse(masterVersion);
   final Version head = Version.parse(headVersion);
   final Map<Version, NextVersionType> allowedVersions =
-      getAllowedNextVersions(masterVersion: master, headVersion: head);
+      getAllowedNextVersions(master, newVersion: head);
   if (allowed) {
     expect(allowedVersions, contains(head));
     if (nextVersionType != null) {
@@ -58,7 +58,6 @@ void main() {
     late CommandRunner<void> runner;
     late RecordingProcessRunner processRunner;
     late List<List<String>> gitDirCommands;
-    String gitDiffResponse;
     Map<String, String> gitShowResponses;
     late MockGitDir gitDir;
 
@@ -66,7 +65,6 @@ void main() {
       fileSystem = MemoryFileSystem();
       packagesDir = createPackagesDirectory(fileSystem: fileSystem);
       gitDirCommands = <List<String>>[];
-      gitDiffResponse = '';
       gitShowResponses = <String, String>{};
       gitDir = MockGitDir();
       when(gitDir.path).thenReturn(packagesDir.parent.path);
@@ -74,10 +72,7 @@ void main() {
           .thenAnswer((Invocation invocation) {
         gitDirCommands.add(invocation.positionalArguments[0] as List<String>);
         final MockProcessResult mockProcessResult = MockProcessResult();
-        if (invocation.positionalArguments[0][0] == 'diff') {
-          when<String?>(mockProcessResult.stdout as String?)
-              .thenReturn(gitDiffResponse);
-        } else if (invocation.positionalArguments[0][0] == 'show') {
+        if (invocation.positionalArguments[0][0] == 'show') {
           final String? response =
               gitShowResponses[invocation.positionalArguments[0][1]];
           if (response == null) {
@@ -101,12 +96,9 @@ void main() {
     });
 
     test('allows valid version', () async {
-      const String newVersion = '2.0.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '2.0.0');
       gitShowResponses = <String, String>{
         'master:packages/plugin/pubspec.yaml': 'version: 1.0.0',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       final List<String> output = await runCapturingPrint(
           runner, <String>['version-check', '--base-sha=master']);
@@ -117,23 +109,18 @@ void main() {
           'No version check errors found!',
         ]),
       );
-      expect(gitDirCommands.length, equals(3));
+      expect(gitDirCommands.length, equals(1));
       expect(
           gitDirCommands,
           containsAll(<Matcher>[
-            equals(<String>['diff', '--name-only', 'master', 'HEAD']),
             equals(<String>['show', 'master:packages/plugin/pubspec.yaml']),
-            equals(<String>['show', 'HEAD:packages/plugin/pubspec.yaml']),
           ]));
     });
 
     test('denies invalid version', () async {
-      const String newVersion = '0.2.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '0.2.0');
       gitShowResponses = <String, String>{
         'master:packages/plugin/pubspec.yaml': 'version: 0.0.1',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       final Future<List<String>> result = runCapturingPrint(
           runner, <String>['version-check', '--base-sha=master']);
@@ -142,23 +129,18 @@ void main() {
         result,
         throwsA(const TypeMatcher<ToolExit>()),
       );
-      expect(gitDirCommands.length, equals(3));
+      expect(gitDirCommands.length, equals(1));
       expect(
           gitDirCommands,
           containsAll(<Matcher>[
-            equals(<String>['diff', '--name-only', 'master', 'HEAD']),
             equals(<String>['show', 'master:packages/plugin/pubspec.yaml']),
-            equals(<String>['show', 'HEAD:packages/plugin/pubspec.yaml']),
           ]));
     });
 
     test('allows valid version without explicit base-sha', () async {
-      const String newVersion = '2.0.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '2.0.0');
       gitShowResponses = <String, String>{
         'abc123:packages/plugin/pubspec.yaml': 'version: 1.0.0',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       final List<String> output =
           await runCapturingPrint(runner, <String>['version-check']);
@@ -172,12 +154,7 @@ void main() {
     });
 
     test('allows valid version for new package.', () async {
-      const String newVersion = '1.0.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
-      gitShowResponses = <String, String>{
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
-      };
+      createFakePlugin('plugin', packagesDir, version: '1.0.0');
       final List<String> output =
           await runCapturingPrint(runner, <String>['version-check']);
 
@@ -191,12 +168,9 @@ void main() {
     });
 
     test('allows likely reverts.', () async {
-      const String newVersion = '0.6.1';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '0.6.1');
       gitShowResponses = <String, String>{
         'abc123:packages/plugin/pubspec.yaml': 'version: 0.6.2',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       final List<String> output =
           await runCapturingPrint(runner, <String>['version-check']);
@@ -210,12 +184,9 @@ void main() {
     });
 
     test('denies lower version that could not be a simple revert', () async {
-      const String newVersion = '0.5.1';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '0.5.1');
       gitShowResponses = <String, String>{
         'abc123:packages/plugin/pubspec.yaml': 'version: 0.6.2',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       final Future<List<String>> result =
           runCapturingPrint(runner, <String>['version-check']);
@@ -227,12 +198,9 @@ void main() {
     });
 
     test('denies invalid version without explicit base-sha', () async {
-      const String newVersion = '0.2.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '0.2.0');
       gitShowResponses = <String, String>{
         'abc123:packages/plugin/pubspec.yaml': 'version: 0.0.1',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       final Future<List<String>> result =
           runCapturingPrint(runner, <String>['version-check']);
@@ -243,38 +211,12 @@ void main() {
       );
     });
 
-    test('gracefully handles missing pubspec.yaml', () async {
-      final Directory pluginDir =
-          createFakePlugin('plugin', packagesDir, examples: <String>[]);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
-      pluginDir.childFile('pubspec.yaml').deleteSync();
-      final List<String> output = await runCapturingPrint(
-          runner, <String>['version-check', '--base-sha=master']);
-
-      expect(
-        output,
-        orderedEquals(<String>[
-          'Determine diff with base sha: master',
-          'Checking versions for packages/plugin/pubspec.yaml...',
-          '  Deleted; skipping.',
-          'No version check errors found!',
-        ]),
-      );
-      expect(gitDirCommands.length, equals(1));
-      expect(gitDirCommands.first.join(' '),
-          equals('diff --name-only master HEAD'));
-    });
-
     test('allows minor changes to platform interfaces', () async {
-      const String newVersion = '1.1.0';
       createFakePlugin('plugin_platform_interface', packagesDir,
-          version: newVersion);
-      gitDiffResponse = 'packages/plugin_platform_interface/pubspec.yaml';
+          version: '1.1.0');
       gitShowResponses = <String, String>{
         'master:packages/plugin_platform_interface/pubspec.yaml':
             'version: 1.0.0',
-        'HEAD:packages/plugin_platform_interface/pubspec.yaml':
-            'version: $newVersion',
       };
       final List<String> output = await runCapturingPrint(
           runner, <String>['version-check', '--base-sha=master']);
@@ -284,32 +226,23 @@ void main() {
           'No version check errors found!',
         ]),
       );
-      expect(gitDirCommands.length, equals(3));
+      expect(gitDirCommands.length, equals(1));
       expect(
           gitDirCommands,
           containsAll(<Matcher>[
-            equals(<String>['diff', '--name-only', 'master', 'HEAD']),
             equals(<String>[
               'show',
               'master:packages/plugin_platform_interface/pubspec.yaml'
             ]),
-            equals(<String>[
-              'show',
-              'HEAD:packages/plugin_platform_interface/pubspec.yaml'
-            ]),
           ]));
     });
 
     test('disallows breaking changes to platform interfaces', () async {
-      const String newVersion = '2.0.0';
       createFakePlugin('plugin_platform_interface', packagesDir,
-          version: newVersion);
-      gitDiffResponse = 'packages/plugin_platform_interface/pubspec.yaml';
+          version: '2.0.0');
       gitShowResponses = <String, String>{
         'master:packages/plugin_platform_interface/pubspec.yaml':
             'version: 1.0.0',
-        'HEAD:packages/plugin_platform_interface/pubspec.yaml':
-            'version: $newVersion',
       };
       final Future<List<String>> output = runCapturingPrint(
           runner, <String>['version-check', '--base-sha=master']);
@@ -317,19 +250,14 @@ void main() {
         output,
         throwsA(const TypeMatcher<ToolExit>()),
       );
-      expect(gitDirCommands.length, equals(3));
+      expect(gitDirCommands.length, equals(1));
       expect(
           gitDirCommands,
           containsAll(<Matcher>[
-            equals(<String>['diff', '--name-only', 'master', 'HEAD']),
             equals(<String>[
               'show',
               'master:packages/plugin_platform_interface/pubspec.yaml'
             ]),
-            equals(<String>[
-              'show',
-              'HEAD:packages/plugin_platform_interface/pubspec.yaml'
-            ]),
           ]));
     });
 
@@ -339,6 +267,7 @@ void main() {
       final Directory pluginDirectory =
           createFakePlugin('plugin', packagesDir, version: version);
       const String changelog = '''
+
 ## $version
 * Some changes.
 ''';
@@ -566,12 +495,9 @@ The first version listed in CHANGELOG.md is 1.0.0.
           'version_check_command', 'Test for $VersionCheckCommand');
       runner.addCommand(command);
 
-      const String newVersion = '2.0.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '2.0.0');
       gitShowResponses = <String, String>{
         'master:packages/plugin/pubspec.yaml': 'version: 1.0.0',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       final List<String> output = await runCapturingPrint(runner,
           <String>['version-check', '--base-sha=master', '--against-pub']);
@@ -603,12 +529,9 @@ The first version listed in CHANGELOG.md is 1.0.0.
           'version_check_command', 'Test for $VersionCheckCommand');
       runner.addCommand(command);
 
-      const String newVersion = '2.0.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '2.0.0');
       gitShowResponses = <String, String>{
         'master:packages/plugin/pubspec.yaml': 'version: 1.0.0',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
 
       bool hasError = false;
@@ -648,12 +571,9 @@ ${indentation}Allowed versions: {1.0.0: NextVersionType.BREAKING_MAJOR, 0.1.0: N
           'version_check_command', 'Test for $VersionCheckCommand');
       runner.addCommand(command);
 
-      const String newVersion = '2.0.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '2.0.0');
       gitShowResponses = <String, String>{
         'master:packages/plugin/pubspec.yaml': 'version: 1.0.0',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       bool hasError = false;
       final List<String> result = await runCapturingPrint(runner, <String>[
@@ -692,12 +612,9 @@ ${indentation}HTTP response: xx
           'version_check_command', 'Test for $VersionCheckCommand');
       runner.addCommand(command);
 
-      const String newVersion = '2.0.0';
-      createFakePlugin('plugin', packagesDir, version: newVersion);
-      gitDiffResponse = 'packages/plugin/pubspec.yaml';
+      createFakePlugin('plugin', packagesDir, version: '2.0.0');
       gitShowResponses = <String, String>{
         'master:packages/plugin/pubspec.yaml': 'version: 1.0.0',
-        'HEAD:packages/plugin/pubspec.yaml': 'version: $newVersion',
       };
       final List<String> result = await runCapturingPrint(runner,
           <String>['version-check', '--base-sha=master', '--against-pub']);

From 5379e2bb22b9ebf308afef625c7d7a2c521fa1b1 Mon Sep 17 00:00:00 2001
From: Stuart Morgan <stuartmorgan@google.com>
Date: Sat, 26 Jun 2021 21:00:29 -0400
Subject: [PATCH 3/6] Extract a helper for the version change check.

---
 .../tool/lib/src/version_check_command.dart   | 156 ++++++++++--------
 1 file changed, 83 insertions(+), 73 deletions(-)

diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart
index d905aa9a5351..75f7a6368361 100644
--- a/script/tool/lib/src/version_check_command.dart
+++ b/script/tool/lib/src/version_check_command.dart
@@ -109,7 +109,6 @@ class VersionCheckCommand extends PluginCommand {
   @override
   Future<void> run() async {
     final GitDir gitDir = await getGitDir();
-    final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
 
     final List<String> badVersionChangePubspecs = <String>[];
 
@@ -124,79 +123,16 @@ class VersionCheckCommand extends PluginCommand {
         continue;
       }
 
-      final Version? currentVersion = pubspec.version;
-      if (currentVersion == null) {
+      final Version? currentPubspecVersion = pubspec.version;
+      if (currentPubspecVersion == null) {
         printError('${indentation}No version found. A package that '
             'intentionally has no version should be marked '
             '"publish_to: none".');
         badVersionChangePubspecs.add(pubspecPath);
         continue;
       }
-      Version? previousVersion;
-      if (getBoolArg(_againstPubFlag)) {
-        previousVersion =
-            await _getPreviousVersionFromPub(pubspec.name, pubspecFile);
-        if (previousVersion == null) {
-          badVersionChangePubspecs.add(pubspecPath);
-          continue;
-        }
-        if (previousVersion != Version.none) {
-          print(
-              '$indentation${pubspec.name}: Current largest version on pub: $previousVersion');
-        }
-      } else {
-        previousVersion = await _getPreviousVersionFromGit(pubspecFile,
-                gitVersionFinder: gitVersionFinder) ??
-            Version.none;
-      }
-      if (previousVersion == Version.none) {
-        print('${indentation}Unable to find previous version '
-            '${getBoolArg(_againstPubFlag) ? 'on pub server' : 'at git base'}.');
-        printWarning(
-            '${indentation}If this plugin is not new, something has gone wrong.');
-        continue;
-      }
-
-      if (previousVersion == currentVersion) {
-        print('${indentation}No version change.');
-        continue;
-      }
-
-      // Check for reverts when doing local validation.
-      if (!getBoolArg(_againstPubFlag) && currentVersion < previousVersion) {
-        final Map<Version, NextVersionType> possibleVersionsFromNewVersion =
-            getAllowedNextVersions(currentVersion, newVersion: previousVersion);
-        // Since this skips validation, try to ensure that it really is likely
-        // to be a revert rather than a typo by checking that the transition
-        // from the lower version to the new version would have been valid.
-        if (possibleVersionsFromNewVersion.containsKey(previousVersion)) {
-          print('${indentation}New version is lower than previous version. '
-              'This is assumed to be a revert.');
-          continue;
-        }
-      }
-
-      final Map<Version, NextVersionType> allowedNextVersions =
-          getAllowedNextVersions(previousVersion, newVersion: currentVersion);
-
-      if (!allowedNextVersions.containsKey(currentVersion)) {
-        final String source = (getBoolArg(_againstPubFlag)) ? 'pub' : 'master';
-        printError('${indentation}Incorrectly updated version.\n'
-            '${indentation}HEAD: $currentVersion, $source: $previousVersion.\n'
-            '${indentation}Allowed versions: $allowedNextVersions');
-        badVersionChangePubspecs.add(pubspecPath);
-        continue;
-      } else {
-        print('$indentation$currentVersion -> $previousVersion');
-      }
 
-      final bool isPlatformInterface =
-          pubspec.name.endsWith('_platform_interface');
-      if (isPlatformInterface &&
-          allowedNextVersions[currentVersion] ==
-              NextVersionType.BREAKING_MAJOR) {
-        printError('$pubspecPath breaking change detected.\n'
-            'Breaking changes to platform interfaces are strongly discouraged.\n');
+      if (!await _checkVersionChange(package, pubspec: pubspec)) {
         badVersionChangePubspecs.add(pubspecPath);
         continue;
       }
@@ -238,9 +174,7 @@ $indentation${mismatchedVersionPlugins.join('\n$indentation')}
   ///
   /// [packageName] must be the actual name of the package as published (i.e.,
   /// the name from pubspec.yaml, not the on disk name if different.)
-  Future<Version?> _getPreviousVersionFromPub(String packageName,
-      File pubspec // XXX change to package; get pubspec file internally.
-      ) async {
+  Future<Version?> _getPreviousVersionFromPub(String packageName) async {
     final PubVersionFinderResponse pubVersionFinderResponse =
         await _pubVersionFinder.getPackageVersion(package: packageName);
     switch (pubVersionFinderResponse.result) {
@@ -260,13 +194,89 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
 
   /// Returns the version of [package] from git at the base comparison hash.
   Future<Version?> _getPreviousVersionFromGit(
-    File pubspec // XXX change to package; get pubspec file internally.
-    , {
+    Directory package, {
     required GitVersionFinder gitVersionFinder,
   }) async {
+    final File pubspecFile = package.childFile('pubspec.yaml');
     final GitDir gitDir = await getGitDir();
     return await gitVersionFinder.getPackageVersion(
-        p.relative(pubspec.absolute.path, from: gitDir.path));
+        p.relative(pubspecFile.absolute.path, from: gitDir.path));
+  }
+
+  /// Returns true if the version of [package] is either unchanged relative to
+  /// the comparison base (git or pub, depending on flags), or is a valid
+  /// version transition.
+  Future<bool> _checkVersionChange(
+    Directory package, {
+    required Pubspec pubspec,
+  }) async {
+    // This method isn't called unless `version` is non-null.
+    final Version currentVersion = pubspec.version!;
+    Version? previousVersion;
+    if (getBoolArg(_againstPubFlag)) {
+      previousVersion = await _getPreviousVersionFromPub(pubspec.name);
+      if (previousVersion == null) {
+        return false;
+      }
+      if (previousVersion != Version.none) {
+        print(
+            '$indentation${pubspec.name}: Current largest version on pub: $previousVersion');
+      }
+    } else {
+      final GitVersionFinder gitVersionFinder = await retrieveVersionFinder();
+      previousVersion = await _getPreviousVersionFromGit(package,
+              gitVersionFinder: gitVersionFinder) ??
+          Version.none;
+    }
+    if (previousVersion == Version.none) {
+      print('${indentation}Unable to find previous version '
+          '${getBoolArg(_againstPubFlag) ? 'on pub server' : 'at git base'}.');
+      printWarning(
+          '${indentation}If this plugin is not new, something has gone wrong.');
+      return true;
+    }
+
+    if (previousVersion == currentVersion) {
+      print('${indentation}No version change.');
+      return true;
+    }
+
+    // Check for reverts when doing local validation.
+    if (!getBoolArg(_againstPubFlag) && currentVersion < previousVersion) {
+      final Map<Version, NextVersionType> possibleVersionsFromNewVersion =
+          getAllowedNextVersions(currentVersion, newVersion: previousVersion);
+      // Since this skips validation, try to ensure that it really is likely
+      // to be a revert rather than a typo by checking that the transition
+      // from the lower version to the new version would have been valid.
+      if (possibleVersionsFromNewVersion.containsKey(previousVersion)) {
+        print('${indentation}New version is lower than previous version. '
+            'This is assumed to be a revert.');
+        return true;
+      }
+    }
+
+    final Map<Version, NextVersionType> allowedNextVersions =
+        getAllowedNextVersions(previousVersion, newVersion: currentVersion);
+
+    if (!allowedNextVersions.containsKey(currentVersion)) {
+      final String source = (getBoolArg(_againstPubFlag)) ? 'pub' : 'master';
+      printError('${indentation}Incorrectly updated version.\n'
+          '${indentation}HEAD: $currentVersion, $source: $previousVersion.\n'
+          '${indentation}Allowed versions: $allowedNextVersions');
+      return false;
+    } else {
+      print('$indentation$currentVersion -> $previousVersion');
+    }
+
+    final bool isPlatformInterface =
+        pubspec.name.endsWith('_platform_interface');
+    if (isPlatformInterface &&
+        allowedNextVersions[currentVersion] == NextVersionType.BREAKING_MAJOR) {
+      printError('Breaking change detected.\n'
+          'Breaking changes to platform interfaces are strongly discouraged.\n');
+      return false;
+    }
+    return true;
   }
 
   /// Returns whether or not the pubspec version and CHANGELOG version for

From 00fd1c55908ae0c5632c2b3f16326bf2dd94f9a4 Mon Sep 17 00:00:00 2001
From: Stuart Morgan <stuartmorgan@google.com>
Date: Sat, 26 Jun 2021 21:02:24 -0400
Subject: [PATCH 4/6] Combine the loops

---
 script/tool/lib/src/version_check_command.dart | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart
index 75f7a6368361..8a915547e705 100644
--- a/script/tool/lib/src/version_check_command.dart
+++ b/script/tool/lib/src/version_check_command.dart
@@ -111,6 +111,7 @@ class VersionCheckCommand extends PluginCommand {
     final GitDir gitDir = await getGitDir();
 
     final List<String> badVersionChangePubspecs = <String>[];
+    final List<String> mismatchedVersionPlugins = <String>[];
 
     await for (final Directory package in getPackages()) {
       final File pubspecFile = package.childFile('pubspec.yaml');
@@ -134,19 +135,13 @@ class VersionCheckCommand extends PluginCommand {
 
       if (!await _checkVersionChange(package, pubspec: pubspec)) {
         badVersionChangePubspecs.add(pubspecPath);
-        continue;
       }
-    }
-    _pubVersionFinder.httpClient.close();
 
-    // TODO(stuartmorgan): Unify the way iteration works for these checks; the
-    // two checks shouldn't be operating independently on different lists.
-    final List<String> mismatchedVersionPlugins = <String>[];
-    await for (final Directory plugin in getPlugins()) {
-      if (!(await _checkVersionsMatch(plugin))) {
-        mismatchedVersionPlugins.add(plugin.basename);
+      if (!(await _checkVersionsMatch(package))) {
+        mismatchedVersionPlugins.add(package.basename);
       }
     }
+    _pubVersionFinder.httpClient.close();
 
     bool passed = true;
     if (badVersionChangePubspecs.isNotEmpty) {

From 2ecd1877c2d08ca0540e81f050f344aaf2725694 Mon Sep 17 00:00:00 2001
From: Stuart Morgan <stuartmorgan@google.com>
Date: Sat, 26 Jun 2021 21:53:11 -0400
Subject: [PATCH 5/6] Migrate to new base command; simplify output

---
 .../src/common/package_looping_command.dart   |   6 +
 .../tool/lib/src/version_check_command.dart   | 145 ++++++++----------
 .../tool/test/version_check_command_test.dart | 119 +++++---------
 3 files changed, 110 insertions(+), 160 deletions(-)

diff --git a/script/tool/lib/src/common/package_looping_command.dart b/script/tool/lib/src/common/package_looping_command.dart
index c6b7c2a0c501..cfe99313068e 100644
--- a/script/tool/lib/src/common/package_looping_command.dart
+++ b/script/tool/lib/src/common/package_looping_command.dart
@@ -35,6 +35,10 @@ abstract class PackageLoopingCommand extends PluginCommand {
   /// in the final summary. An empty list indicates success.
   Future<List<String>> runForPackage(Directory package);
 
+  /// Called during [run] after all calls to [runForPackage]. This provides an
+  /// opportunity to do any cleanup of run-level state.
+  Future<void> completeRun() async {}
+
   /// Whether or not the output (if any) of [runForPackage] is long, or short.
   ///
   /// This changes the logging that happens at the start of each package's
@@ -118,6 +122,8 @@ abstract class PackageLoopingCommand extends PluginCommand {
       results[package] = await runForPackage(package);
     }
 
+    completeRun();
+
     // If there were any errors reported, summarize them and exit.
     if (results.values.any((List<String> failures) => failures.isNotEmpty)) {
       const String indentation = '  ';
diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart
index 8a915547e705..7b786d6334ad 100644
--- a/script/tool/lib/src/version_check_command.dart
+++ b/script/tool/lib/src/version_check_command.dart
@@ -12,7 +12,7 @@ import 'package:pubspec_parse/pubspec_parse.dart';
 
 import 'common/core.dart';
 import 'common/git_version_finder.dart';
-import 'common/plugin_command.dart';
+import 'common/package_looping_command.dart';
 import 'common/process_runner.dart';
 import 'common/pub_version_finder.dart';
 
@@ -72,7 +72,7 @@ Map<Version, NextVersionType> getAllowedNextVersions(
 }
 
 /// A command to validate version changes to packages.
-class VersionCheckCommand extends PluginCommand {
+class VersionCheckCommand extends PackageLoopingCommand {
   /// Creates an instance of the version check command.
   VersionCheckCommand(
     Directory packagesDir, {
@@ -93,6 +93,8 @@ class VersionCheckCommand extends PluginCommand {
 
   static const String _againstPubFlag = 'against-pub';
 
+  final PubVersionFinder _pubVersionFinder;
+
   @override
   final String name = 'version-check';
 
@@ -102,67 +104,51 @@ class VersionCheckCommand extends PluginCommand {
       'Also checks if the latest version in CHANGELOG matches the version in pubspec.\n\n'
       'This command requires "pub" and "flutter" to be in your path.';
 
-  final PubVersionFinder _pubVersionFinder;
-
-  String get indentation => '  ';
-
   @override
-  Future<void> run() async {
-    final GitDir gitDir = await getGitDir();
+  bool get hasLongOutput => false;
 
-    final List<String> badVersionChangePubspecs = <String>[];
-    final List<String> mismatchedVersionPlugins = <String>[];
-
-    await for (final Directory package in getPackages()) {
-      final File pubspecFile = package.childFile('pubspec.yaml');
-      final String pubspecPath =
-          p.relative(pubspecFile.absolute.path, from: gitDir.path);
-      print('Checking versions for $pubspecPath...');
-      final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync());
-      if (pubspec.publishTo == 'none') {
-        print('${indentation}Found "publish_to: none"; skipping.');
-        continue;
-      }
+  @override
+  Future<void> initializeRun() async {}
 
-      final Version? currentPubspecVersion = pubspec.version;
-      if (currentPubspecVersion == null) {
-        printError('${indentation}No version found. A package that '
-            'intentionally has no version should be marked '
-            '"publish_to: none".');
-        badVersionChangePubspecs.add(pubspecPath);
-        continue;
-      }
+  @override
+  Future<List<String>> runForPackage(Directory package) async {
+    final List<String> errors = <String>[];
 
-      if (!await _checkVersionChange(package, pubspec: pubspec)) {
-        badVersionChangePubspecs.add(pubspecPath);
-      }
+    final Pubspec? pubspec = _tryParsePubspec(package);
+    Pubspec.parse(package.childFile('pubspec.yaml').readAsStringSync());
+    if (pubspec == null) {
+      errors.add('Invalid pubspec.yaml.');
+      return errors; // No remaining checks make sense.
+    }
 
-      if (!(await _checkVersionsMatch(package))) {
-        mismatchedVersionPlugins.add(package.basename);
-      }
+    if (pubspec.publishTo == 'none') {
+      printSkip('${indentation}Found "publish_to: none".');
+      return PackageLoopingCommand.success;
     }
-    _pubVersionFinder.httpClient.close();
 
-    bool passed = true;
-    if (badVersionChangePubspecs.isNotEmpty) {
-      passed = false;
-      printError('''
-The following pubspecs failed validaton:
-$indentation${badVersionChangePubspecs.join('\n$indentation')}
-''');
+    final Version? currentPubspecVersion = pubspec.version;
+    if (currentPubspecVersion == null) {
+      printError('${indentation}No version found in pubspec.yaml. A package '
+          'that intentionally has no version should be marked '
+          '"publish_to: none".');
+      errors.add('No pubspec.yaml version.');
+      return errors; // No remaining checks make sense.
     }
-    if (mismatchedVersionPlugins.isNotEmpty) {
-      passed = false;
-      printError('''
-The following pubspecs have different versions in pubspec.yaml and CHANGELOG.md:
-$indentation${mismatchedVersionPlugins.join('\n$indentation')}
-''');
+
+    if (!await _checkVersionChange(package, pubspec: pubspec)) {
+      errors.add('Disallowed version change.');
     }
-    if (!passed) {
-      throw ToolExit(1);
+
+    if (!(await _checkVersionsMatch(package, pubspec: pubspec))) {
+      errors.add('pubspec.yaml and CHANGELOG.md have different versions');
     }
 
-    print('No version check errors found!');
+    return errors;
+  }
+
+  @override
+  Future<void> completeRun() async {
+    _pubVersionFinder.httpClient.close();
   }
 
   /// Returns the previous published version of [package].
@@ -253,22 +239,24 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
     final Map<Version, NextVersionType> allowedNextVersions =
         getAllowedNextVersions(previousVersion, newVersion: currentVersion);
 
-    if (!allowedNextVersions.containsKey(currentVersion)) {
+    if (allowedNextVersions.containsKey(currentVersion)) {
+      print('$indentation$previousVersion -> $currentVersion');
+    } else {
       final String source = (getBoolArg(_againstPubFlag)) ? 'pub' : 'master';
       printError('${indentation}Incorrectly updated version.\n'
           '${indentation}HEAD: $currentVersion, $source: $previousVersion.\n'
           '${indentation}Allowed versions: $allowedNextVersions');
       return false;
-    } else {
-      print('$indentation$currentVersion -> $previousVersion');
     }
 
     final bool isPlatformInterface =
         pubspec.name.endsWith('_platform_interface');
+    // TODO(stuartmorgan): Relax this check. See
+    // https://github.com/flutter/flutter/issues/85391
     if (isPlatformInterface &&
         allowedNextVersions[currentVersion] == NextVersionType.BREAKING_MAJOR) {
-      printError('Breaking change detected.\n'
-          'Breaking changes to platform interfaces are strongly discouraged.\n');
+      printError('${indentation}Breaking change detected.\n'
+          '${indentation}Breaking changes to platform interfaces are strongly discouraged.\n');
       return false;
     }
     return true;
@@ -276,19 +264,12 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
 
   /// Returns whether or not the pubspec version and CHANGELOG version for
   /// [plugin] match.
-  Future<bool> _checkVersionsMatch(Directory package) async {
-    // get version from pubspec
-    final String packageName = package.basename;
-    print('-----------------------------------------');
-    print(
-        'Checking the first version listed in CHANGELOG.md matches the version in pubspec.yaml for $packageName.');
-
-    final Pubspec? pubspec = _tryParsePubspec(package);
-    if (pubspec == null) {
-      printError('Cannot parse version from pubspec.yaml');
-      return false;
-    }
-    final Version? fromPubspec = pubspec.version;
+  Future<bool> _checkVersionsMatch(
+    Directory package, {
+    required Pubspec pubspec,
+  }) async {
+    // This method isn't called unless `version` is non-null.
+    final Version fromPubspec = pubspec.version!;
 
     // get first version from CHANGELOG
     final File changelog = package.childFile('CHANGELOG.md');
@@ -308,7 +289,8 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
     // changes that don't warrant publishing on their own.
     final bool hasNextSection = versionString == 'NEXT';
     if (hasNextSection) {
-      print('Found NEXT; validating next version in the CHANGELOG.');
+      print(
+          '${indentation}Found NEXT; validating next version in the CHANGELOG.');
       // Ensure that the version in pubspec hasn't changed without updating
       // CHANGELOG. That means the next version entry in the CHANGELOG pass the
       // normal validation.
@@ -324,15 +306,15 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
         versionString == null ? null : Version.parse(versionString);
     if (fromChangeLog == null) {
       printError(
-          'Cannot find version on the first line of ${package.path}/CHANGELOG.md');
+          '${indentation}Cannot find version on the first line CHANGELOG.md');
       return false;
     }
 
     if (fromPubspec != fromChangeLog) {
       printError('''
-versions for $packageName in CHANGELOG.md and pubspec.yaml do not match.
-The version in pubspec.yaml is $fromPubspec.
-The first version listed in CHANGELOG.md is $fromChangeLog.
+${indentation}Versions in CHANGELOG.md and pubspec.yaml do not match.
+${indentation}The version in pubspec.yaml is $fromPubspec.
+${indentation}The first version listed in CHANGELOG.md is $fromChangeLog.
 ''');
       return false;
     }
@@ -341,15 +323,13 @@ The first version listed in CHANGELOG.md is $fromChangeLog.
     if (!hasNextSection) {
       final RegExp nextRegex = RegExp(r'^#+\s*NEXT\s*$');
       if (lines.any((String line) => nextRegex.hasMatch(line))) {
-        printError('''
-When bumping the version for release, the NEXT section should be incorporated
-into the new version's release notes.
-''');
+        printError('${indentation}When bumping the version for release, the '
+            'NEXT section should be incorporated into the new version\'s '
+            'release notes.');
         return false;
       }
     }
 
-    print('$packageName passed version check');
     return true;
   }
 
@@ -360,9 +340,8 @@ into the new version's release notes.
       final Pubspec pubspec = Pubspec.parse(pubspecFile.readAsStringSync());
       return pubspec;
     } on Exception catch (exception) {
-      printError(
-          'Failed to parse `pubspec.yaml` at ${pubspecFile.path}: $exception}');
+      printError('${indentation}Failed to parse `pubspec.yaml`: $exception}');
+      return null;
     }
-    return null;
   }
 }
diff --git a/script/tool/test/version_check_command_test.dart b/script/tool/test/version_check_command_test.dart
index ae56f0b72218..4d884692046d 100644
--- a/script/tool/test/version_check_command_test.dart
+++ b/script/tool/test/version_check_command_test.dart
@@ -42,14 +42,6 @@ void testAllowedVersion(
 
 class MockProcessResult extends Mock implements io.ProcessResult {}
 
-const String _redColorMessagePrefix = '\x1B[31m';
-const String _redColorMessagePostfix = '\x1B[0m';
-
-// Some error message was printed in a "Colorized" red message. So `\x1B[31m` and `\x1B[0m` needs to be included.
-String _redColorString(String string) {
-  return '$_redColorMessagePrefix$string$_redColorMessagePostfix';
-}
-
 void main() {
   const String indentation = '  ';
   group('$VersionCheckCommand', () {
@@ -105,8 +97,9 @@ void main() {
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          'No version check errors found!',
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
+          contains('1.0.0 -> 2.0.0'),
         ]),
       );
       expect(gitDirCommands.length, equals(1));
@@ -147,8 +140,9 @@ void main() {
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          'No version check errors found!',
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
+          contains('1.0.0 -> 2.0.0'),
         ]),
       );
     });
@@ -160,9 +154,9 @@ void main() {
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          '${indentation}Unable to find previous version at git base.',
-          'No version check errors found!',
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
+          contains('Unable to find previous version at git base.'),
         ]),
       );
     });
@@ -177,8 +171,9 @@ void main() {
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          '${indentation}New version is lower than previous version. This is assumed to be a revert.',
+        containsAllInOrder(<Matcher>[
+          contains('New version is lower than previous version. '
+              'This is assumed to be a revert.'),
         ]),
       );
     });
@@ -222,8 +217,9 @@ void main() {
           runner, <String>['version-check', '--base-sha=master']);
       expect(
         output,
-        containsAllInOrder(<String>[
-          'No version check errors found!',
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
+          contains('1.0.0 -> 1.1.0'),
         ]),
       );
       expect(gitDirCommands.length, equals(1));
@@ -276,10 +272,8 @@ void main() {
           runner, <String>['version-check', '--base-sha=master']);
       expect(
         output,
-        containsAllInOrder(<String>[
-          'Checking the first version listed in CHANGELOG.md matches the version in pubspec.yaml for plugin.',
-          'plugin passed version check',
-          'No version check errors found!'
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
         ]),
       );
     });
@@ -305,12 +299,8 @@ void main() {
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          _redColorString('''
-versions for plugin in CHANGELOG.md and pubspec.yaml do not match.
-The version in pubspec.yaml is 1.0.1.
-The first version listed in CHANGELOG.md is 1.0.2.
-'''),
+        containsAllInOrder(<Matcher>[
+          contains('Versions in CHANGELOG.md and pubspec.yaml do not match.'),
         ]),
       );
     });
@@ -329,10 +319,8 @@ The first version listed in CHANGELOG.md is 1.0.2.
           runner, <String>['version-check', '--base-sha=master']);
       expect(
         output,
-        containsAllInOrder(<String>[
-          'Checking the first version listed in CHANGELOG.md matches the version in pubspec.yaml for plugin.',
-          'plugin passed version check',
-          'No version check errors found!'
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
         ]),
       );
     });
@@ -363,14 +351,8 @@ The first version listed in CHANGELOG.md is 1.0.2.
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          _redColorString(
-            '''
-versions for plugin in CHANGELOG.md and pubspec.yaml do not match.
-The version in pubspec.yaml is 1.0.0.
-The first version listed in CHANGELOG.md is 1.0.1.
-''',
-          )
+        containsAllInOrder(<Matcher>[
+          contains('Versions in CHANGELOG.md and pubspec.yaml do not match.'),
         ]),
       );
     });
@@ -392,10 +374,9 @@ The first version listed in CHANGELOG.md is 1.0.1.
           runner, <String>['version-check', '--base-sha=master']);
       await expectLater(
         output,
-        containsAllInOrder(<String>[
-          'Found NEXT; validating next version in the CHANGELOG.',
-          'plugin passed version check',
-          'No version check errors found!',
+        containsAllInOrder(<Matcher>[
+          contains('Running for plugin'),
+          contains('Found NEXT; validating next version in the CHANGELOG.'),
         ]),
       );
     });
@@ -428,13 +409,9 @@ The first version listed in CHANGELOG.md is 1.0.1.
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          _redColorString(
-            '''
-When bumping the version for release, the NEXT section should be incorporated
-into the new version's release notes.
-''',
-          )
+        containsAllInOrder(<Matcher>[
+          contains('When bumping the version for release, the NEXT section '
+              'should be incorporated into the new version\'s release notes.')
         ]),
       );
     });
@@ -463,15 +440,9 @@ into the new version's release notes.
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          'Found NEXT; validating next version in the CHANGELOG.',
-          _redColorString(
-            '''
-versions for plugin in CHANGELOG.md and pubspec.yaml do not match.
-The version in pubspec.yaml is 1.0.1.
-The first version listed in CHANGELOG.md is 1.0.0.
-''',
-          )
+        containsAllInOrder(<Matcher>[
+          contains('Found NEXT; validating next version in the CHANGELOG.'),
+          contains('Versions in CHANGELOG.md and pubspec.yaml do not match.'),
         ]),
       );
     });
@@ -504,9 +475,8 @@ The first version listed in CHANGELOG.md is 1.0.0.
 
       expect(
         output,
-        containsAllInOrder(<String>[
-          '${indentation}plugin: Current largest version on pub: 1.0.0',
-          'No version check errors found!',
+        containsAllInOrder(<Matcher>[
+          contains('plugin: Current largest version on pub: 1.0.0'),
         ]),
       );
     });
@@ -547,13 +517,11 @@ The first version listed in CHANGELOG.md is 1.0.0.
 
       expect(
         result,
-        containsAllInOrder(<String>[
-          _redColorString(
-            '''
+        containsAllInOrder(<Matcher>[
+          contains('''
 ${indentation}Incorrectly updated version.
 ${indentation}HEAD: 2.0.0, pub: 0.0.2.
-${indentation}Allowed versions: {1.0.0: NextVersionType.BREAKING_MAJOR, 0.1.0: NextVersionType.MINOR, 0.0.3: NextVersionType.PATCH}''',
-          )
+${indentation}Allowed versions: {1.0.0: NextVersionType.BREAKING_MAJOR, 0.1.0: NextVersionType.MINOR, 0.0.3: NextVersionType.PATCH}''')
         ]),
       );
     });
@@ -588,14 +556,12 @@ ${indentation}Allowed versions: {1.0.0: NextVersionType.BREAKING_MAJOR, 0.1.0: N
 
       expect(
         result,
-        containsAllInOrder(<String>[
-          _redColorString(
-            '''
+        containsAllInOrder(<Matcher>[
+          contains('''
 ${indentation}Error fetching version on pub for plugin.
 ${indentation}HTTP Status 400
 ${indentation}HTTP response: xx
-''',
-          )
+''')
         ]),
       );
     });
@@ -621,9 +587,8 @@ ${indentation}HTTP response: xx
 
       expect(
         result,
-        containsAllInOrder(<String>[
-          '${indentation}Unable to find previous version on pub server.',
-          'No version check errors found!',
+        containsAllInOrder(<Matcher>[
+          contains('Unable to find previous version on pub server.'),
         ]),
       );
     });

From 13ed3c9e46bc7805b0e198a75f4b9a32ef0f0fed Mon Sep 17 00:00:00 2001
From: Stuart Morgan <stuartmorgan@google.com>
Date: Mon, 28 Jun 2021 14:18:46 -0400
Subject: [PATCH 6/6] Address review feedback

---
 script/tool/lib/src/common/plugin_command.dart  |  8 ++++----
 script/tool/lib/src/publish_plugin_command.dart |  4 ++--
 script/tool/lib/src/version_check_command.dart  | 16 +++++++---------
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/script/tool/lib/src/common/plugin_command.dart b/script/tool/lib/src/common/plugin_command.dart
index c146b1aba904..2eef48c63878 100644
--- a/script/tool/lib/src/common/plugin_command.dart
+++ b/script/tool/lib/src/common/plugin_command.dart
@@ -76,8 +76,8 @@ abstract class PluginCommand extends Command<void> {
   /// This can be overridden for testing.
   final ProcessRunner processRunner;
 
-  /// The git directory to use. If unset, [getGitDir] uses the packages
-  /// directory's enclosing repository.
+  /// The git directory to use. If unset, [gitDir] populates it from the
+  /// packages directory's enclosing repository.
   ///
   /// This can be mocked for testing.
   GitDir? _gitDir;
@@ -102,7 +102,7 @@ abstract class PluginCommand extends Command<void> {
   }
 
   /// Returns the [GitDir] containing [packagesDir].
-  Future<GitDir> getGitDir() async {
+  Future<GitDir> get gitDir async {
     GitDir? gitDir = _gitDir;
     if (gitDir != null) {
       return gitDir;
@@ -309,7 +309,7 @@ abstract class PluginCommand extends Command<void> {
     final String baseSha = getStringArg(_kBaseSha);
 
     final GitVersionFinder gitVersionFinder =
-        GitVersionFinder(await getGitDir(), baseSha);
+        GitVersionFinder(await gitDir, baseSha);
     return gitVersionFinder;
   }
 
diff --git a/script/tool/lib/src/publish_plugin_command.dart b/script/tool/lib/src/publish_plugin_command.dart
index 6c4076ca9b8f..045c2c2f1853 100644
--- a/script/tool/lib/src/publish_plugin_command.dart
+++ b/script/tool/lib/src/publish_plugin_command.dart
@@ -149,7 +149,7 @@ class PublishPluginCommand extends PluginCommand {
     }
 
     _print('Checking local repo...');
-    final GitDir gitDir = await getGitDir();
+    final GitDir repository = await gitDir;
 
     final bool shouldPushTag = getBoolArg(_pushTagsOption);
     _RemoteInfo? remote;
@@ -171,7 +171,7 @@ class PublishPluginCommand extends PluginCommand {
     bool successful;
     if (publishAllChanged) {
       successful = await _publishAllChangedPackages(
-        baseGitDir: gitDir,
+        baseGitDir: repository,
         remoteForTagPush: remote,
       );
     } else {
diff --git a/script/tool/lib/src/version_check_command.dart b/script/tool/lib/src/version_check_command.dart
index 7b786d6334ad..2584d70c5fc9 100644
--- a/script/tool/lib/src/version_check_command.dart
+++ b/script/tool/lib/src/version_check_command.dart
@@ -115,7 +115,6 @@ class VersionCheckCommand extends PackageLoopingCommand {
     final List<String> errors = <String>[];
 
     final Pubspec? pubspec = _tryParsePubspec(package);
-    Pubspec.parse(package.childFile('pubspec.yaml').readAsStringSync());
     if (pubspec == null) {
       errors.add('Invalid pubspec.yaml.');
       return errors; // No remaining checks make sense.
@@ -135,11 +134,11 @@ class VersionCheckCommand extends PackageLoopingCommand {
       return errors; // No remaining checks make sense.
     }
 
-    if (!await _checkVersionChange(package, pubspec: pubspec)) {
+    if (!await _hasValidVersionChange(package, pubspec: pubspec)) {
       errors.add('Disallowed version change.');
     }
 
-    if (!(await _checkVersionsMatch(package, pubspec: pubspec))) {
+    if (!(await _hasConsistentVersion(package, pubspec: pubspec))) {
       errors.add('pubspec.yaml and CHANGELOG.md have different versions');
     }
 
@@ -155,7 +154,7 @@ class VersionCheckCommand extends PackageLoopingCommand {
   ///
   /// [packageName] must be the actual name of the package as published (i.e.,
   /// the name from pubspec.yaml, not the on disk name if different.)
-  Future<Version?> _getPreviousVersionFromPub(String packageName) async {
+  Future<Version?> _fetchPreviousVersionFromPub(String packageName) async {
     final PubVersionFinderResponse pubVersionFinderResponse =
         await _pubVersionFinder.getPackageVersion(package: packageName);
     switch (pubVersionFinderResponse.result) {
@@ -179,15 +178,14 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
     required GitVersionFinder gitVersionFinder,
   }) async {
     final File pubspecFile = package.childFile('pubspec.yaml');
-    final GitDir gitDir = await getGitDir();
     return await gitVersionFinder.getPackageVersion(
-        p.relative(pubspecFile.absolute.path, from: gitDir.path));
+        p.relative(pubspecFile.absolute.path, from: (await gitDir).path));
   }
 
   /// Returns true if the version of [package] is either unchanged relative to
   /// the comparison base (git or pub, depending on flags), or is a valid
   /// version transition.
-  Future<bool> _checkVersionChange(
+  Future<bool> _hasValidVersionChange(
     Directory package, {
     required Pubspec pubspec,
   }) async {
@@ -195,7 +193,7 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
     final Version currentVersion = pubspec.version!;
     Version? previousVersion;
     if (getBoolArg(_againstPubFlag)) {
-      previousVersion = await _getPreviousVersionFromPub(pubspec.name);
+      previousVersion = await _fetchPreviousVersionFromPub(pubspec.name);
       if (previousVersion == null) {
         return false;
       }
@@ -264,7 +262,7 @@ ${indentation}HTTP response: ${pubVersionFinderResponse.httpResponse.body}
 
   /// Returns whether or not the pubspec version and CHANGELOG version for
   /// [plugin] match.
-  Future<bool> _checkVersionsMatch(
+  Future<bool> _hasConsistentVersion(
     Directory package, {
     required Pubspec pubspec,
   }) async {