[tool] Provide a --base-branch flag (flutter#3322)

[tool] Provide a --base-branch flag

Author: stuartmorgan-g

script/tool/README.md

@@ -117,17 +117,24 @@ code changes across many packages:
 cd <repository root>
 dart run script/tool/bin/flutter_plugin_tools.dart update-release-info \
   --version=minimal \
+  --base-branch=upstream/main \
   --changelog="Fixes violations of new analysis option some_new_option."
 ```
 
 The `minimal` option for `--version` will skip unchanged packages, and treat
 each changed package as either `bugfix` or `next` depending on the files that
 have changed in that package, so it is often the best choice for a bulk change.
 
-For cases where you know the change time, `minor` or `bugfix` will make the
+For cases where you know the change type, `minor` or `bugfix` will make the
 corresponding version bump, or `next` will update only `CHANGELOG.md` without
 changing the version.
 
+If you have a standard repository setup, `--base-branch=upstream/main` will
+usually give the behavior you want, finding all packages changed relative to
+the branch point from `upstream/main`. For more complex use cases where you want
+a different diff point, you can pass a different `--base-branch`, or use
+`--base-sha` to pick the exact diff point.
+
 ### Publish a Release
 
 **Releases are automated for `flutter/packages`.**

script/tool/lib/src/common/git_version_finder.dart

@@ -11,7 +11,11 @@ import 'package:yaml/yaml.dart';
 /// Finding diffs based on `baseGitDir` and `baseSha`.
 class GitVersionFinder {
   /// Constructor
-  GitVersionFinder(this.baseGitDir, String? baseSha) : _baseSha = baseSha;
+  GitVersionFinder(this.baseGitDir, {String? baseSha, String? baseBranch})
+      : assert(baseSha == null || baseBranch == null,
+            'At most one of baseSha and baseBranch can be provided'),
+        _baseSha = baseSha,
+        _baseBranch = baseBranch ?? 'FETCH_HEAD';
 
   /// The top level directory of the git repo.
   ///
@@ -21,6 +25,9 @@ class GitVersionFinder {
   /// The base sha used to get diff.
   String? _baseSha;
 
+  /// The base branche used to find a merge point if baseSha is not provided.
+  final String _baseBranch;
+
   static bool _isPubspec(String file) {
     return file.trim().endsWith('pubspec.yaml');
   }
@@ -101,13 +108,13 @@ class GitVersionFinder {
     }
 
     io.ProcessResult baseShaFromMergeBase = await baseGitDir.runCommand(
-        <String>['merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'],
+        <String>['merge-base', '--fork-point', _baseBranch, 'HEAD'],
         throwOnError: false);
     final String stdout = (baseShaFromMergeBase.stdout as String? ?? '').trim();
     final String stderr = (baseShaFromMergeBase.stderr as String? ?? '').trim();
     if (stderr.isNotEmpty || stdout.isEmpty) {
       baseShaFromMergeBase = await baseGitDir
-          .runCommand(<String>['merge-base', 'FETCH_HEAD', 'HEAD']);
+          .runCommand(<String>['merge-base', _baseBranch, 'HEAD']);
     }
     baseSha = (baseShaFromMergeBase.stdout as String).trim();
     _baseSha = baseSha;

script/tool/lib/src/common/package_command.dart

@@ -94,12 +94,17 @@ abstract class PackageCommand extends Command<void> {
         help: 'The base sha used to determine git diff. \n'
             'This is useful when $_runOnChangedPackagesArg is specified.\n'
             'If not specified, merge-base is used as base sha.');
+    argParser.addOption(_baseBranchArg,
+        help: 'The base branch whose merge base is used as the base SHA if '
+            '--$_baseShaArg is not provided. \n'
+            'If not specified, FETCH_HEAD is used as the base branch.');
     argParser.addFlag(_logTimingArg,
         help: 'Logs timing information.\n\n'
             'Currently only logs per-package timing for multi-package commands, '
             'but more information may be added in the future.');
   }
 
+  static const String _baseBranchArg = 'base-branch';
   static const String _baseShaArg = 'base-sha';
   static const String _excludeArg = 'exclude';
   static const String _logTimingArg = 'log-timing';
@@ -188,6 +193,11 @@ abstract class PackageCommand extends Command<void> {
     return (argResults![key] as String?) ?? '';
   }
 
+  /// Convenience accessor for String arguments.
+  String? getNullableStringArg(String key) {
+    return argResults![key] as String?;
+  }
+
   /// Convenience accessor for List<String> arguments.
   List<String> getStringListArg(String key) {
     // Clone the list so that if a caller modifies the result it won't change
@@ -338,7 +348,7 @@ abstract class PackageCommand extends Command<void> {
         if (lastCommitOnly) {
           print(
               '--$_packagesForBranchArg: using parent commit as the diff base.');
-          changedFileFinder = GitVersionFinder(await gitDir, 'HEAD~');
+          changedFileFinder = GitVersionFinder(await gitDir, baseSha: 'HEAD~');
         } else {
           changedFileFinder = await retrieveVersionFinder();
         }
@@ -361,7 +371,7 @@ abstract class PackageCommand extends Command<void> {
       }
     } else if (getBoolArg(_runOnDirtyPackagesArg)) {
       final GitVersionFinder gitVersionFinder =
-          GitVersionFinder(await gitDir, 'HEAD');
+          GitVersionFinder(await gitDir, baseSha: 'HEAD');
       print('Running for all packages that have uncommitted changes\n');
       // _changesRequireFullTest is deliberately not used here, as this flag is
       // intended for use in CI to re-test packages changed by
@@ -473,10 +483,12 @@ abstract class PackageCommand extends Command<void> {
   ///
   /// Throws tool exit if [gitDir] nor root directory is a git directory.
   Future<GitVersionFinder> retrieveVersionFinder() async {
-    final String baseSha = getStringArg(_baseShaArg);
+    final String? baseSha = getNullableStringArg(_baseShaArg);
+    final String? baseBranch =
+        baseSha == null ? getNullableStringArg(_baseBranchArg) : null;
 
-    final GitVersionFinder gitVersionFinder =
-        GitVersionFinder(await gitDir, baseSha);
+    final GitVersionFinder gitVersionFinder = GitVersionFinder(await gitDir,
+        baseSha: baseSha, baseBranch: baseBranch);
     return gitVersionFinder;
   }
 

script/tool/test/common/git_version_finder_test.dart

@@ -38,7 +38,8 @@ void main() {
   });
 
   test('No git diff should result no files changed', () async {
-    final GitVersionFinder finder = GitVersionFinder(gitDir, 'some base sha');
+    final GitVersionFinder finder =
+        GitVersionFinder(gitDir, baseSha: 'some base sha');
     final List<String> changedFiles = await finder.getChangedFiles();
 
     expect(changedFiles, isEmpty);
@@ -49,7 +50,8 @@ void main() {
 file1/file1.cc
 file2/file2.cc
 ''';
-    final GitVersionFinder finder = GitVersionFinder(gitDir, 'some base sha');
+    final GitVersionFinder finder =
+        GitVersionFinder(gitDir, baseSha: 'some base sha');
     final List<String> changedFiles = await finder.getChangedFiles();
 
     expect(changedFiles, equals(<String>['file1/file1.cc', 'file2/file2.cc']));
@@ -60,7 +62,8 @@ file2/file2.cc
 file1/pubspec.yaml
 file2/file2.cc
 ''';
-    final GitVersionFinder finder = GitVersionFinder(gitDir, 'some base sha');
+    final GitVersionFinder finder =
+        GitVersionFinder(gitDir, baseSha: 'some base sha');
     final List<String> changedFiles = await finder.getChangedPubSpecs();
 
     expect(changedFiles, equals(<String>['file1/pubspec.yaml']));
@@ -73,8 +76,28 @@ file1/pubspec.yaml
 file2/file2.cc
 ''';
 
-    final GitVersionFinder finder = GitVersionFinder(gitDir, null);
+    final GitVersionFinder finder = GitVersionFinder(gitDir);
     await finder.getChangedFiles();
+    verify(gitDir.runCommand(
+        <String>['merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'],
+        throwOnError: false));
+    verify(gitDir.runCommand(
+        <String>['diff', '--name-only', mergeBaseResponse, 'HEAD']));
+  });
+
+  test('uses correct base branch to find base sha if specified', () async {
+    mergeBaseResponse = 'shaqwiueroaaidf12312jnadf123nd';
+    gitDiffResponse = '''
+file1/pubspec.yaml
+file2/file2.cc
+''';
+
+    final GitVersionFinder finder =
+        GitVersionFinder(gitDir, baseBranch: 'upstream/main');
+    await finder.getChangedFiles();
+    verify(gitDir.runCommand(
+        <String>['merge-base', '--fork-point', 'upstream/main', 'HEAD'],
+        throwOnError: false));
     verify(gitDir.runCommand(
         <String>['diff', '--name-only', mergeBaseResponse, 'HEAD']));
   });
@@ -85,7 +108,8 @@ file2/file2.cc
 file1/pubspec.yaml
 file2/file2.cc
 ''';
-    final GitVersionFinder finder = GitVersionFinder(gitDir, customBaseSha);
+    final GitVersionFinder finder =
+        GitVersionFinder(gitDir, baseSha: customBaseSha);
     await finder.getChangedFiles();
     verify(gitDir
         .runCommand(<String>['diff', '--name-only', customBaseSha, 'HEAD']));
@@ -97,7 +121,8 @@ file2/file2.cc
 file1/pubspec.yaml
 file2/file2.cc
 ''';
-    final GitVersionFinder finder = GitVersionFinder(gitDir, customBaseSha);
+    final GitVersionFinder finder =
+        GitVersionFinder(gitDir, baseSha: customBaseSha);
     await finder.getChangedFiles(includeUncommitted: true);
     // The call should not have HEAD as a final argument like the default diff.
     verify(gitDir.runCommand(<String>['diff', '--name-only', customBaseSha]));