[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
flutter · Jan 30, 2023 · 3843b38 · 3843b38
1 parent 8f12b27
commit 3843b38
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 14 deletions.
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 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

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;
   }
 
+  // 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'],

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'),
           ]));
     });
   });