[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
flutter · auto-submit · Jan 30, 2023 · Jan 27, 2023 · Jan 27, 2023 · Jan 30, 2023
commit 085f0d27396532fbf2d53ae5d079a405ba6bc8aa
diff --git a/script/tool/CHANGELOG.md b/script/tool/CHANGELOG.md
@@ -1,3 +1,8 @@
+## 0.13.4+1
+
+* Makes `--packages-for-branch` detect any commin 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

diff --git a/script/tool/lib/src/common/package_command.dart b/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;
   }
 
+  // Retruns true if the environment indicates that the current treeish is from
+  // [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 ProcessResult = await (await gitDir)
+        .runCommand(<String>['merge-base', '--is-ancestor', 'HEAD', 'main']);
+    return ProcessResult.exitCode == 0;
+  }
+
   Future<String?> _getBranch() async {
     final io.ProcessResult branchResult = await (await gitDir).runCommand(
         <String>['rev-parse', '--abbrev-ref', 'HEAD'],

diff --git a/script/tool/pubspec.yaml b/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

diff --git a/script/tool/test/common/package_command_test.dart b/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'),
           ]));
     });
   });