validate and commit after regenerating gradle lockfiles from pub autoroller (#154152)

Fixes https://github.com/flutter/flutter/issues/154151

Validate the git diff generated by the regenerate gradle lockfile script and then commit the changes.

dev/conductor/core/lib/src/packages_autoroller.dart

@@ -6,13 +6,13 @@ import 'dart:convert';
 import 'dart:io' as io;
 
 import 'package:file/file.dart';
-import 'package:meta/meta.dart';
 import 'package:process/process.dart';
 
 import 'git.dart';
 import 'globals.dart';
 import 'repository.dart';
 import 'stdio.dart';
+import 'validate_checkout_post_gradle_regeneration.dart';
 
 /// A service for rolling the SDK's pub packages to latest and open a PR upstream.
 class PackageAutoroller {
@@ -52,6 +52,8 @@ class PackageAutoroller {
 
   static const String hostname = 'github.com';
 
+  String get gitAuthor => '$githubUsername <$githubUsername@google.com>';
+
   String get prBody {
     return '''
 This PR was generated by the automated
@@ -95,7 +97,7 @@ This PR was generated by the automated
         log('Packages are already at latest.');
         return;
       }
-      await generateGradleLockfiles(await framework.checkoutDirectory);
+      await _regenerateGradleLockfiles(await framework.checkoutDirectory);
       await pushBranch();
       await createPr(repository: await framework.checkoutDirectory);
       await authLogout();
@@ -114,8 +116,6 @@ This PR was generated by the automated
   Future<bool> updatePackages({
     bool verbose = true,
   }) async {
-    final String author = '$githubUsername <$githubUsername@google.com>';
-
     await framework.newBranch(await featureBranchName);
     final io.Process flutterProcess = await framework.streamFlutter(<String>[
       if (verbose) '--verbose',
@@ -126,25 +126,44 @@ This PR was generated by the automated
     if (exitCode != 0) {
       throw ConductorException('Failed to update packages with exit code $exitCode');
     }
-    // If the git checkout is clean, then pub packages are already at latest that cleanly resolve.
+    // If the git checkout is clean, then pub packages are already at latest
+    // that cleanly resolve.
     if (await framework.gitCheckoutClean()) {
       return false;
     }
     await framework.commit(
       'roll packages',
       addFirst: true,
-      author: author,
+      author: gitAuthor,
     );
     return true;
   }
 
-  @visibleForTesting
+  Future<void> _regenerateGradleLockfiles(Directory repoRoot) async {
-  Future<void> generateGradleLockfiles(Directory repoRoot) async {
     await framework.runDart(<String>[
       '${repoRoot.path}/dev/tools/bin/generate_gradle_lockfiles.dart',
       '--no-gradle-generation',
       '--no-exclusion',
     ]);
+    switch (CheckoutStatePostGradleRegeneration(await framework.gitStatus(), framework.fileSystem.path)) {
+      // If the git checkout is clean, we did not update any lockfiles and we do
+      // not need an additional commit.
+      case NoDiff():
+        return;
+      case OnlyLockfileChanges():
+        await framework.commit(
+          'Re-generate Gradle lockfiles',
+          addFirst: true,
+          author: gitAuthor,
+        );
+      case NonLockfileChanges(changes: final List<String> changes):
+        throw StateError(
+          'Expected all diffs after re-generating gradle lockfiles to end in '
+          '`.lockfile`, but encountered: $changes',
+        );
+      case MalformedLine(line: final String line):
+        throw StateError('Unexpected line of STDOUT from git status: "$line"');
+    }
   }
 
   Future<void> pushBranch() async {

dev/conductor/core/lib/src/repository.dart

@@ -258,14 +258,21 @@ abstract class Repository {
     );
   }
 
-  /// Verify the repository's git checkout is clean.
+  /// Get the working tree status.
-  Future<bool> gitCheckoutClean() async {
+  ///
-    final String output = await git.getOutput(
+  /// Calls `git status --porcelain` which should output in a stable format
+  /// across git versions.
+  Future<String> gitStatus() async {
+    return git.getOutput(
       <String>['status', '--porcelain'],
       'check that the git checkout is clean',
       workingDirectory: (await checkoutDirectory).path,
     );
-    return output == '';
+  }
+
+  /// Verify the repository's git checkout is clean.
+  Future<bool> gitCheckoutClean() async {
+    return (await gitStatus()).isEmpty;
   }
 
   /// Return the revision for the branch point between two refs.

dev/conductor/core/lib/src/validate_checkout_post_gradle_regeneration.dart

@@ -0,0 +1,77 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:path/path.dart' show Context;
+
+/// Possible states of the Flutter repo checkout after Gradle lockfile
+/// regeneration.
+sealed class CheckoutStatePostGradleRegeneration {
+  factory CheckoutStatePostGradleRegeneration(String gitStatusOutput, Context context) {
+    gitStatusOutput = gitStatusOutput.trim();
+    if (gitStatusOutput.isEmpty) {
+      return const NoDiff();
+    }
+
+    final List<String> changes = gitStatusOutput.trim().split('\n');
+    final List<String> changedPaths = <String>[];
+    for (final String line in changes) {
+      final RegExpMatch? match = pattern.firstMatch(line);
+      if (match == null) {
+        return MalformedLine(line);
+      }
+      changedPaths.add(match.group(1)!);
+    }
+
+    final List<String> nonLockfileDiffs = changedPaths.where((String path) {
+      final String extension = context.extension(path);
+      return extension != '.lockfile';
+    }).toList();
+
+    if (nonLockfileDiffs.isNotEmpty) {
+      return NonLockfileChanges(nonLockfileDiffs);
+    }
+
+    return const OnlyLockfileChanges();
+  }
+
+  /// Output format for `git status --porcelain` and `git status --short`.
+  ///
+  /// The first capture group is the path to the file or directory changed,
+  /// relative to the root of the repository.
+  ///
+  /// See `man git-status` for more reference.
+  static final RegExp pattern = RegExp(r'[ACDMRTU ]{1,2} (\S+)');
+}
+
+/// No files were changed, no commit needed.
+final class NoDiff implements CheckoutStatePostGradleRegeneration {
+  const NoDiff();
+}
+
+/// Only files ending in *.lockfile were changed; changes can be committed.
+final class OnlyLockfileChanges implements CheckoutStatePostGradleRegeneration {
+  const OnlyLockfileChanges();
+}
+
+/// There are changed files that do not end in *.lockfile; fail the script.
+///
+/// Because the script to regenerate Gradle lockfiles triggers a Gradle build,
+/// and because the packages_autoroller can have its PRs merged without a
+/// human review, we are conservative about what changes we commit.
+final class NonLockfileChanges implements CheckoutStatePostGradleRegeneration {
+  const NonLockfileChanges(this.changes);
+
+  final List<String> changes;
+}
+
+/// A line in the output of `git status` does not match the expected pattern;
+/// fail the script.
+///
+/// This likely means there is a bug in the regular expression, and it needs
+/// to be updated.
+final class MalformedLine implements CheckoutStatePostGradleRegeneration {
+  const MalformedLine(this.line);
+
+  final String line;
+}

dev/conductor/core/test/packages_autoroller_test.dart

@@ -8,7 +8,9 @@ import 'dart:io' as io;
 
 import 'package:conductor_core/conductor_core.dart';
 import 'package:conductor_core/packages_autoroller.dart';
+import 'package:conductor_core/src/validate_checkout_post_gradle_regeneration.dart';
 import 'package:file/memory.dart';
+import 'package:path/path.dart' show Context, Style;
 import 'package:platform/platform.dart';
 
 import '../bin/packages_autoroller.dart' show run;
@@ -438,6 +440,39 @@ void main() {
         '--no-gradle-generation',
         '--no-exclusion',
       ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'status',
+        '--porcelain',
+      ], stdout: '''
+ M dev/integration_tests/ui/android/project-app.lockfile
+ M examples/image_list/android/project-app.lockfile
+'''),
+      const FakeCommand(command: <String>[
+        'git',
+        'status',
+        '--porcelain',
+      ], stdout: '''
+ M dev/integration_tests/ui/android/project-app.lockfile
+ M examples/image_list/android/project-app.lockfile
+'''),
+      const FakeCommand(command: <String>[
+        'git',
+        'add',
+        '--all',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'commit',
+        '--message',
+        'Re-generate Gradle lockfiles',
+        '--author="flutter-pub-roller-bot <flutter-pub-roller-bot@google.com>"',
+      ]),
+      const FakeCommand(command: <String>[
+        'git',
+        'rev-parse',
+        'HEAD',
+      ], stdout: '234deadbeef'),
       const FakeCommand(command: <String>[
         'git',
         'push',
@@ -540,6 +575,55 @@ void main() {
     stdio.printTrace('Using $token');
     expect(stdio.logs.last, '[trace] Using $replacement');
   });
+
+  group('CheckoutStatePostGradleRegeneration', () {
+    final Context ctx = Context(style: Style.posix);
+
+    test('empty input returns NoDiff', () {
+      expect(
+        CheckoutStatePostGradleRegeneration('', ctx),
+        const NoDiff(),
+      );
+    });
+
+    test('only *.lockfile changes returns OnlyLockfileChanges', () {
+      expect(
+        CheckoutStatePostGradleRegeneration('''
+ A dev/benchmarks/test_apps/stocks/android/buildscript-gradle.lockfile
+ M dev/integration_tests/ui/android/project-app.lockfile
+ M examples/image_list/android/project-app.lockfile
+''', ctx),
+        const OnlyLockfileChanges(),
+      );
+    });
+
+    test('if a *.zip file is added returns NonLockfileChanges', () {
+      const String pathToZip = 'dev/benchmarks/test_apps/stocks/android/very-large-archive.zip';
+      CheckoutStatePostGradleRegeneration result = CheckoutStatePostGradleRegeneration('''
+ A dev/benchmarks/test_apps/stocks/android/buildscript-gradle.lockfile
+ A $pathToZip
+ M dev/integration_tests/ui/android/project-app.lockfile
+ M examples/image_list/android/project-app.lockfile
+''', ctx);
+      expect(result, isA<NonLockfileChanges>());
+      result = result as NonLockfileChanges;
+      expect(result.changes, hasLength(1));
+      expect(result.changes.single, pathToZip);
+    });
+
+    test('if it contains a line not matching the regex returns MalformedLine', () {
+      const String malformedLine = 'New Git Output.';
+      CheckoutStatePostGradleRegeneration result = CheckoutStatePostGradleRegeneration('''
+$malformedLine
+ A dev/benchmarks/test_apps/stocks/android/buildscript-gradle.lockfile
+ M dev/integration_tests/ui/android/project-app.lockfile
+ M examples/image_list/android/project-app.lockfile
+''', ctx);
+      expect(result, isA<MalformedLine>());
+      result = result as MalformedLine;
+      expect(result.line, malformedLine);
+    });
+  });
 }
 
 class _NoOpStdin extends Fake implements io.Stdin {}