[tool] Improve main-branch detection (#7038)

* [tool] Improve main-branch detection

Currently main-branch detection for `--packages-for-branch` looks at
branch names, but this no longer works on LUCI which now seems to be
checking out specific hashes rather than branches. This updates the
behavior so that it will treat any hash that is an ancestor of `main` as
being part of `main`, which should allow post-submit detection to work
under LUCI.

Fixes flutter/flutter#119330

* Fix throw

* Fix typos

* Update comment

Author: stuartmorgan-g

script/tool/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 0.13.4+1
+
+* Makes `--packages-for-branch` detect any commit on `main` as being `main`,
+  so that it works with pinned checkouts (e.g., on LUCI).
+
 ## 0.13.4
 
 * Adds the ability to validate minimum supported Dart/Flutter versions in

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

@@ -316,17 +316,28 @@ abstract class PackageCommand extends Command<void> {
     } else if (getBoolArg(_packagesForBranchArg)) {
       final String? branch = await _getBranch();
       if (branch == null) {
-        printError('Unabled to determine branch; --$_packagesForBranchArg can '
+        printError('Unable to determine branch; --$_packagesForBranchArg can '
             'only be used in a git repository.');
         throw ToolExit(exitInvalidArguments);
       } else {
         // Configure the change finder the correct mode for the branch.
-        final bool lastCommitOnly = branch == 'main' || branch == 'master';
+        // Log the mode to make it easier to audit logs to see that the
+        // intended diff was used (or why).
+        final bool lastCommitOnly;
+        if (branch == 'main' || branch == 'master') {
+          print('--$_packagesForBranchArg: running on default branch.');
+          lastCommitOnly = true;
+        } else if (await _isCheckoutFromBranch('main')) {
+          print(
+              '--$_packagesForBranchArg: running on a commit from default branch.');
+          lastCommitOnly = true;
+        } else {
+          print('--$_packagesForBranchArg: running on branch "$branch".');
+          lastCommitOnly = false;
+        }
         if (lastCommitOnly) {
-          // Log the mode to make it easier to audit logs to see that the
-          // intended diff was used.
-          print('--$_packagesForBranchArg: running on default branch; '
-              'using parent commit as the diff base.');
+          print(
+              '--$_packagesForBranchArg: using parent commit as the diff base.');
           changedFileFinder = GitVersionFinder(await gitDir, 'HEAD~');
         } else {
           changedFileFinder = await retrieveVersionFinder();
@@ -522,6 +533,17 @@ abstract class PackageCommand extends Command<void> {
     return packages;
   }
 
+  // Returns true if the current checkout is on an ancestor of [branch].
+  //
+  // This is used because CI may check out a specific hash rather than a branch,
+  // in which case branch-name detection won't work.
+  Future<bool> _isCheckoutFromBranch(String branchName) async {
+    final io.ProcessResult result = await (await gitDir).runCommand(
+        <String>['merge-base', '--is-ancestor', 'HEAD', branchName],
+        throwOnError: false);
+    return result.exitCode == 0;
+  }
+
   Future<String?> _getBranch() async {
     final io.ProcessResult branchResult = await (await gitDir).runCommand(
         <String>['rev-parse', '--abbrev-ref', 'HEAD'],

script/tool/pubspec.yaml

@@ -1,7 +1,7 @@
 name: flutter_plugin_tools
 description: Productivity utils for flutter/plugins and flutter/packages
 repository: https://github.com/flutter/plugins/tree/main/script/tool
-version: 0.13.4
+version: 0.13.4+1
 
 dependencies:
   args: ^2.1.0

script/tool/test/common/package_command_test.dart

@@ -778,7 +778,8 @@ packages/b_package/lib/src/foo.dart
         MockProcess(stdout: 'a-branch'),
       ];
       processRunner.mockProcessesForExecutable['git-merge-base'] = <Process>[
-        MockProcess(stdout: 'abc123'),
+        MockProcess(exitCode: 1), // --is-ancestor check
+        MockProcess(stdout: 'abc123'), // finding merge base
       ];
       final RepositoryPackage plugin1 =
           createFakePlugin('plugin1', packagesDir);
@@ -791,6 +792,7 @@ packages/b_package/lib/src/foo.dart
       expect(
           output,
           containsAllInOrder(<Matcher>[
+            contains('--packages-for-branch: running on branch "a-branch"'),
             contains(
                 'Running for all packages that have diffs relative to "abc123"'),
           ]));
@@ -822,8 +824,9 @@ packages/b_package/lib/src/foo.dart
       expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('--packages-for-branch: running on default branch; '
-                'using parent commit as the diff base'),
+            contains('--packages-for-branch: running on default branch.'),
+            contains(
+                '--packages-for-branch: using parent commit as the diff base'),
             contains(
                 'Running for all packages that have diffs relative to "HEAD~"'),
           ]));
@@ -836,7 +839,45 @@ packages/b_package/lib/src/foo.dart
           ));
     });
 
-    test('tests all packages on master', () async {
+    test(
+        'only tests changed packages relative to the previous commit if '
+        'running on a specific hash from main', () async {
+      processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
+        MockProcess(stdout: 'packages/plugin1/plugin1.dart'),
+      ];
+      processRunner.mockProcessesForExecutable['git-rev-parse'] = <Process>[
+        MockProcess(stdout: 'HEAD'),
+      ];
+      final RepositoryPackage plugin1 =
+          createFakePlugin('plugin1', packagesDir);
+      createFakePlugin('plugin2', packagesDir);
+
+      final List<String> output = await runCapturingPrint(
+          runner, <String>['sample', '--packages-for-branch']);
+
+      expect(command.plugins, unorderedEquals(<String>[plugin1.path]));
+      expect(
+          output,
+          containsAllInOrder(<Matcher>[
+            contains(
+                '--packages-for-branch: running on a commit from default branch.'),
+            contains(
+                '--packages-for-branch: using parent commit as the diff base'),
+            contains(
+                'Running for all packages that have diffs relative to "HEAD~"'),
+          ]));
+      // Ensure that it's diffing against the prior commit.
+      expect(
+          processRunner.recordedCalls,
+          contains(
+            const ProcessCall(
+                'git-diff', <String>['--name-only', 'HEAD~', 'HEAD'], null),
+          ));
+    });
+
+    test(
+        'only tests changed packages relative to the previous commit on master',
+        () async {
       processRunner.mockProcessesForExecutable['git-diff'] = <Process>[
         MockProcess(stdout: 'packages/plugin1/plugin1.dart'),
       ];
@@ -854,8 +895,9 @@ packages/b_package/lib/src/foo.dart
       expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('--packages-for-branch: running on default branch; '
-                'using parent commit as the diff base'),
+            contains('--packages-for-branch: running on default branch.'),
+            contains(
+                '--packages-for-branch: using parent commit as the diff base'),
             contains(
                 'Running for all packages that have diffs relative to "HEAD~"'),
           ]));
@@ -887,7 +929,7 @@ packages/b_package/lib/src/foo.dart
       expect(
           output,
           containsAllInOrder(<Matcher>[
-            contains('Unabled to determine branch'),
+            contains('Unable to determine branch'),
           ]));
     });
   });