[flutter_conductor] fix pushing local working branch to mirror remote (#88057)

dev/conductor/lib/globals.dart

@@ -182,19 +182,25 @@ String getNewPrLink({
   if (repoName == 'engine') {
     if (state.engine.dartRevision.isNotEmpty) {
       // shorten hashes to make final link manageable
+      // prefix with github org/repo so GitHub will auto-generate a hyperlink
       body.writeln('- Roll dart revision: dart-lang/sdk@${state.engine.dartRevision.substring(0, 9)}');
     }
-    body.writeAll(
-      state.engine.cherrypicks.map<String>((pb.Cherrypick cp) => '- commit: ${cp.trunkRevision.substring(0, 9)}'),
-      '\n',
-    );
+    for (final pb.Cherrypick cp in state.engine.cherrypicks) {
+      // Only list commits that map to a commit that exists upstream.
+      if (cp.trunkRevision.isNotEmpty) {
+        body.writeln('- commit: flutter/engine@${cp.trunkRevision.substring(0, 9)}');
+      }
+    }
   } else {
-    body.writeAll(
-      state.framework.cherrypicks.map<String>((pb.Cherrypick cp) => '- commit: ${cp.trunkRevision.substring(0, 9)}'),
-      '\n',
-    );
+    for (final pb.Cherrypick cp in state.framework.cherrypicks) {
+      // Only list commits that map to a commit that exists upstream.
+      if (cp.trunkRevision.isNotEmpty) {
+        body.writeln('- commit: ${cp.trunkRevision.substring(0, 9)}');
+      }
+    }
   }
-  return 'https://github.com/flutter/$repoName/compare/$candidateBranch...$userName:$workingBranch?'
+  return 'https://github.com/flutter/$repoName/compare/'
+      '$candidateBranch...$userName:$workingBranch?'
       'expand=1'
       '&title=${Uri.encodeQueryComponent(title)}'
       '&body=${Uri.encodeQueryComponent(body.toString())}';

dev/conductor/lib/next.dart

@@ -95,11 +95,31 @@ void runNext({
 
   switch (state.currentPhase) {
     case pb.ReleasePhase.APPLY_ENGINE_CHERRYPICKS:
-      final List<pb.Cherrypick> unappliedCherrypicks = <pb.Cherrypick>[];
-      for (final pb.Cherrypick cherrypick in state.engine.cherrypicks) {
-        if (!finishedStates.contains(cherrypick.state)) {
-          unappliedCherrypicks.add(cherrypick);
-        }
+      final Remote upstream = Remote(
+        name: RemoteName.upstream,
+        url: state.engine.upstream.url,
+      );
+      final EngineRepository engine = EngineRepository(
+        checkouts,
+        initialRef: state.engine.workingBranch,
+        upstreamRemote: upstream,
+        previousCheckoutLocation: state.engine.checkoutPath,
+      );
+      final String headRevision = engine.reverseParse('HEAD');
+
+      // check if the candidate branch is enabled in .ci.yaml
+      if (!engine.ciYaml.enabledBranches.contains(state.engine.candidateBranch)) {
+        engine.ciYaml.enableBranch(state.engine.candidateBranch);
+        // commit
+        final String revision = engine.commit(
+          'add branch ${state.engine.candidateBranch} to enabled_branches in .ci.yaml',
+          addFirst: true,
+        );
+        // append to list of cherrypicks so we know a PR is required
+        state.engine.cherrypicks.add(pb.Cherrypick(
+          appliedRevision: revision,
+          state: pb.CherrypickState.COMPLETED,
+        ));
       }
 
       if (!requiresEnginePR(state)) {
@@ -109,6 +129,13 @@ void runNext({
         break;
       }
 
+      final List<pb.Cherrypick> unappliedCherrypicks = <pb.Cherrypick>[];
+      for (final pb.Cherrypick cherrypick in state.engine.cherrypicks) {
+        if (!finishedStates.contains(cherrypick.state)) {
+          unappliedCherrypicks.add(cherrypick);
+        }
+      }
+
       if (unappliedCherrypicks.isEmpty) {
         stdio.printStatus('All engine cherrypicks have been auto-applied by the conductor.\n');
       } else {
@@ -128,21 +155,11 @@ void runNext({
           return;
         }
       }
-      final Remote upstream = Remote(
-        name: RemoteName.upstream,
-        url: state.engine.upstream.url,
-      );
-      final EngineRepository engine = EngineRepository(
-        checkouts,
-        initialRef: state.engine.workingBranch,
-        upstreamRemote: upstream,
-        previousCheckoutLocation: state.engine.checkoutPath,
-      );
-      final String headRevision = engine.reverseParse('HEAD');
 
       engine.pushRef(
         fromRef: headRevision,
-        toRef: state.engine.workingBranch,
+        // Explicitly create new branch
+        toRef: 'refs/heads/${state.engine.workingBranch}',
         remote: state.engine.mirror.name,
       );
 
@@ -166,13 +183,6 @@ void runNext({
       }
       break;
     case pb.ReleasePhase.APPLY_FRAMEWORK_CHERRYPICKS:
-      final List<pb.Cherrypick> unappliedCherrypicks = <pb.Cherrypick>[];
-      for (final pb.Cherrypick cherrypick in state.framework.cherrypicks) {
-        if (!finishedStates.contains(cherrypick.state)) {
-          unappliedCherrypicks.add(cherrypick);
-        }
-      }
-
       if (state.engine.cherrypicks.isEmpty && state.engine.dartRevision.isEmpty) {
         stdio.printStatus(
           'This release has no engine cherrypicks, and thus the engine.version file\n'
@@ -213,11 +223,41 @@ void runNext({
       );
       final String headRevision = framework.reverseParse('HEAD');
 
+      // Check if the current candidate branch is enabled
+      if (!framework.ciYaml.enabledBranches.contains(state.framework.candidateBranch)) {
+        framework.ciYaml.enableBranch(state.framework.candidateBranch);
+        // commit
+        final String revision = framework.commit(
+          'add branch ${state.framework.candidateBranch} to enabled_branches in .ci.yaml',
+          addFirst: true,
+        );
+        // append to list of cherrypicks so we know a PR is required
+        state.framework.cherrypicks.add(pb.Cherrypick(
+          appliedRevision: revision,
+          state: pb.CherrypickState.COMPLETED,
+        ));
+      }
+
       stdio.printStatus('Rolling new engine hash $engineRevision to framework checkout...');
-      framework.updateEngineRevision(engineRevision);
-      framework.commit(
+      final bool needsCommit = framework.updateEngineRevision(engineRevision);
+      if (needsCommit) {
+        final String revision = framework.commit(
           'Update Engine revision to $engineRevision for ${state.releaseChannel} release ${state.releaseVersion}',
-          addFirst: true);
+          addFirst: true,
+        );
+        // append to list of cherrypicks so we know a PR is required
+        state.framework.cherrypicks.add(pb.Cherrypick(
+          appliedRevision: revision,
+          state: pb.CherrypickState.COMPLETED,
+        ));
+      }
+
+      final List<pb.Cherrypick> unappliedCherrypicks = <pb.Cherrypick>[];
+      for (final pb.Cherrypick cherrypick in state.framework.cherrypicks) {
+        if (!finishedStates.contains(cherrypick.state)) {
+          unappliedCherrypicks.add(cherrypick);
+        }
+      }
 
       if (state.framework.cherrypicks.isEmpty) {
         stdio.printStatus(
@@ -251,7 +291,8 @@ void runNext({
 
       framework.pushRef(
         fromRef: headRevision,
-        toRef: state.framework.workingBranch,
+        // Explicitly create new branch
+        toRef: 'refs/heads/${state.framework.workingBranch}',
         remote: state.framework.mirror.name,
       );
       break;

dev/conductor/lib/repository.dart

@@ -9,6 +9,7 @@ import 'package:file/file.dart';
 import 'package:meta/meta.dart';
 import 'package:platform/platform.dart';
 import 'package:process/process.dart';
+import 'package:yaml/yaml.dart';
 
 import './git.dart';
 import './globals.dart';
@@ -69,6 +70,7 @@ abstract class Repository {
             'Provided previousCheckoutLocation $previousCheckoutLocation does not exist on disk!');
       }
       if (initialRef != null) {
+        assert(initialRef != '');
         git.run(
           <String>['fetch', upstreamRemote.name],
           'Fetch ${upstreamRemote.name} to ensure we have latest refs',
@@ -398,6 +400,14 @@ abstract class Repository {
     bool addFirst = false,
   }) {
     assert(!message.contains("'"));
+    final bool hasChanges = git.getOutput(
+      <String>['status', '--porcelain'],
+      'check for uncommitted changes',
+      workingDirectory: checkoutDirectory.path,
+    ).trim().isNotEmpty;
+    if (!hasChanges) {
+      throw ConductorException('Tried to commit with message $message but no changes were present');
+    }
     if (addFirst) {
       git.run(
         <String>['add', '--all'],
@@ -490,6 +500,7 @@ class FrameworkRepository extends Repository {
   }
 
   final Checkouts checkouts;
+  late final CiYaml ciYaml = CiYaml(checkoutDirectory.childFile('.ci.yaml'));
   static const String defaultUpstream =
       'https://github.com/flutter/flutter.git';
 
@@ -586,7 +597,10 @@ class FrameworkRepository extends Repository {
     return Version.fromString(versionJson['frameworkVersion'] as String);
   }
 
-  void updateEngineRevision(
+  /// Update this framework's engine version file.
+  ///
+  /// Returns [true] if the version file was updated and a commit is needed.
+  bool updateEngineRevision(
     String newEngine, {
     @visibleForTesting File? engineVersionFile,
   }) {
@@ -597,8 +611,19 @@ class FrameworkRepository extends Repository {
         .childFile('engine.version');
     assert(engineVersionFile.existsSync());
     final String oldEngine = engineVersionFile.readAsStringSync();
+    if (oldEngine.trim() == newEngine.trim()) {
+      stdio.printTrace(
+        'Tried to update the engine revision but version file is already up to date at: $newEngine',
+      );
+      return false;
+    }
     stdio.printStatus('Updating engine revision from $oldEngine to $newEngine');
-    engineVersionFile.writeAsStringSync(newEngine.trim(), flush: true);
+    engineVersionFile.writeAsStringSync(
+      // Version files have trailing newlines
+      '${newEngine.trim()}\n',
+      flush: true,
+    );
+    return true;
   }
 }
 
@@ -699,6 +724,8 @@ class EngineRepository extends Repository {
 
   final Checkouts checkouts;
 
+  late final CiYaml ciYaml = CiYaml(checkoutDirectory.childFile('.ci.yaml'));
+
   static const String defaultUpstream = 'https://github.com/flutter/engine.git';
   static const String defaultBranch = 'master';
 
@@ -766,3 +793,68 @@ class Checkouts {
   final ProcessManager processManager;
   final Stdio stdio;
 }
+
+class CiYaml {
+  CiYaml(this.file) {
+    if (!file.existsSync()) {
+      throw ConductorException('Could not find the .ci.yaml file at ${file.path}');
+    }
+  }
+
+  /// Underlying [File] that this object wraps.
+  final File file;
+
+  /// Returns the raw string contents of this file.
+  ///
+  /// This is not cached as the contents can be written to while the conductor
+  /// is running.
+  String get stringContents => file.readAsStringSync();
+
+  /// Returns the parsed contents of the file as a [YamlMap].
+  ///
+  /// This is not cached as the contents can be written to while the conductor
+  /// is running.
+  YamlMap get contents => loadYaml(stringContents) as YamlMap;
+
+  List<String> get enabledBranches {
+    final YamlList yamlList = contents['enabled_branches'] as YamlList;
+    return yamlList.map<String>((dynamic element) {
+      return element as String;
+    }).toList();
+  }
+
+  static final RegExp _enabledBranchPattern = RegExp(r'^enabled_branches:');
+
+  /// Update this .ci.yaml file with the given branch name.
+  ///
+  /// The underlying [File] is written to, but not committed to git. This method
+  /// will throw a [ConductorException] if the [branchName] is already present
+  /// in the file or if the file does not have an "enabled_branches:" field.
+  void enableBranch(String branchName) {
+    final List<String> newStrings = <String>[];
+    if (enabledBranches.contains(branchName)) {
+      throw ConductorException('${file.path} already contains the branch $branchName');
+    }
+    if (!_enabledBranchPattern.hasMatch(stringContents)) {
+      throw ConductorException(
+        'Did not find the expected string "enabled_branches:" in the file ${file.path}',
+      );
+    }
+    final List<String> lines = stringContents.split('\n');
+    bool insertedCurrentBranch = false;
+    for (final String line in lines) {
+      // Every existing line should be copied to the new Yaml
+      newStrings.add(line);
+      if (insertedCurrentBranch) {
+        continue;
+      }
+      if (_enabledBranchPattern.hasMatch(line)) {
+        insertedCurrentBranch = true;
+        // Indent two spaces
+        final String indent = ' ' * 2;
+        newStrings.add('$indent- ${branchName.trim()}');
+      }
+    }
+    file.writeAsStringSync(newStrings.join('\n'), flush: true);
+  }
+}

dev/conductor/lib/state.dart

@@ -267,8 +267,7 @@ bool requiresEnginePR(pb.ConductorState state) {
 /// This release will require a new Framework PR.
 ///
 /// The logic is if there was an Engine PR OR there are framework cherrypicks
-/// that have not been abandoned OR the increment level is 'm', then return
-/// true, else false.
+/// that have not been abandoned.
 bool requiresFrameworkPR(pb.ConductorState state) {
   if (requiresEnginePR(state)) {
     return true;
@@ -278,9 +277,5 @@ bool requiresFrameworkPR(pb.ConductorState state) {
   if (hasRequiredCherrypicks) {
     return true;
   }
-  if (state.incrementLevel == 'm') {
-    // requires an update to .ci.yaml
-    return true;
-  }
   return false;
 }

dev/conductor/test/globals_test.dart

@@ -89,8 +89,9 @@ void main() {
 ## Scheduled Cherrypicks
 
 - Roll dart revision: dart-lang/sdk@${dartRevision.substring(0, 9)}
-- commit: ${engineCherrypick1.substring(0, 9)}
-- commit: ${engineCherrypick2.substring(0, 9)}''';
+- commit: flutter/engine@${engineCherrypick1.substring(0, 9)}
+- commit: flutter/engine@${engineCherrypick2.substring(0, 9)}
+''';
       expect(
         Uri.decodeQueryComponent(bodyPattern.firstMatch(link)?.group(1) ?? ''),
         expectedBody,
@@ -120,7 +121,8 @@ void main() {
 
 ## Scheduled Cherrypicks
 
-- commit: ${frameworkCherrypick.substring(0, 9)}''';
+- commit: ${frameworkCherrypick.substring(0, 9)}
+''';
       expect(
         Uri.decodeQueryComponent(bodyPattern.firstMatch(link)?.group(1) ?? ''),
         expectedBody,

dev/conductor/test/repository_test.dart

@@ -224,6 +224,65 @@ vars = {
       );
     });
 
+    test('commit() throws if there are no local changes to commit', () {
+      const String commit1 = 'abc123';
+      const String commit2 = 'def456';
+      const String message = 'This is a commit message.';
+      final TestStdio stdio = TestStdio();
+      final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+      final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+        FakeCommand(command: <String>[
+          'git',
+          'clone',
+          '--origin',
+          'upstream',
+          '--',
+          EngineRepository.defaultUpstream,
+          fileSystem.path
+              .join(rootDir, 'flutter_conductor_checkouts', 'engine'),
+        ]),
+        const FakeCommand(command: <String>[
+          'git',
+          'checkout',
+          'upstream/master',
+        ]),
+        const FakeCommand(command: <String>[
+          'git',
+          'rev-parse',
+          'HEAD',
+        ], stdout: commit1),
+        const FakeCommand(command: <String>[
+          'git',
+          'status',
+          '--porcelain',
+        ]),
+        const FakeCommand(command: <String>[
+          'git',
+          'commit',
+          "--message='$message'",
+        ]),
+        const FakeCommand(command: <String>[
+          'git',
+          'rev-parse',
+          'HEAD',
+        ], stdout: commit2),
+      ]);
+
+      final Checkouts checkouts = Checkouts(
+        fileSystem: fileSystem,
+        parentDirectory: fileSystem.directory(rootDir),
+        platform: platform,
+        processManager: processManager,
+        stdio: stdio,
+      );
+
+      final EngineRepository repo = EngineRepository(checkouts);
+      expect(
+        () => repo.commit(message),
+        throwsExceptionWith('Tried to commit with message $message but no changes were present'),
+      );
+    });
+
     test('commit() passes correct commit message', () {
       const String commit1 = 'abc123';
       const String commit2 = 'def456';
@@ -251,6 +310,10 @@ vars = {
           'rev-parse',
           'HEAD',
         ], stdout: commit1),
+        const FakeCommand(
+          command: <String>['git', 'status', '--porcelain'],
+          stdout: 'MM path/to/file.txt',
+        ),
         const FakeCommand(command: <String>[
           'git',
           'commit',
@@ -274,6 +337,167 @@ vars = {
       final EngineRepository repo = EngineRepository(checkouts);
       repo.commit(message);
     });
+
+    test('updateEngineRevision() returns false if newCommit is the same as version file', () {
+      const String commit1 = 'abc123';
+      const String commit2 = 'def456';
+      final TestStdio stdio = TestStdio();
+      final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+      final File engineVersionFile = fileSystem.file('/engine.version')..writeAsStringSync(commit2);
+      final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+        FakeCommand(command: <String>[
+          'git',
+          'clone',
+          '--origin',
+          'upstream',
+          '--',
+          FrameworkRepository.defaultUpstream,
+          fileSystem.path
+              .join(rootDir, 'flutter_conductor_checkouts', 'framework'),
+        ]),
+        const FakeCommand(command: <String>[
+          'git',
+          'rev-parse',
+          'HEAD',
+        ], stdout: commit1),
+      ]);
+
+      final Checkouts checkouts = Checkouts(
+        fileSystem: fileSystem,
+        parentDirectory: fileSystem.directory(rootDir),
+        platform: platform,
+        processManager: processManager,
+        stdio: stdio,
+      );
+
+      final FrameworkRepository repo = FrameworkRepository(checkouts);
+      final bool didUpdate = repo.updateEngineRevision(commit2, engineVersionFile: engineVersionFile);
+      expect(didUpdate, false);
+    });
+
+    test('CiYaml(file) will throw if file does not exist', () {
+      final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+      final File file = fileSystem.file('/non/existent/file.txt');
+
+      expect(
+        () => CiYaml(file),
+        throwsExceptionWith('Could not find the .ci.yaml file at /non/existent/file.txt'),
+      );
+    });
+
+    test('ciYaml.enableBranch() will prepend the given branch to the yaml list of enabled_branches', () {
+      const String commit1 = 'abc123';
+      final TestStdio stdio = TestStdio();
+      final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+      final File ciYaml = fileSystem.file('/flutter_conductor_checkouts/framework/.ci.yaml');
+      final ProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+        FakeCommand(
+            command: <String>[
+          'git',
+          'clone',
+          '--origin',
+          'upstream',
+          '--',
+          FrameworkRepository.defaultUpstream,
+          fileSystem.path
+              .join(rootDir, 'flutter_conductor_checkouts', 'framework'),
+        ],
+        onRun: () {
+          ciYaml.createSync(recursive: true);
+          ciYaml.writeAsStringSync('''
+enabled_branches:
+  - master
+  - dev
+  - beta
+  - stable
+''');
+        }),
+        const FakeCommand(command: <String>[
+          'git',
+          'rev-parse',
+          'HEAD',
+        ], stdout: commit1),
+      ]);
+      final Checkouts checkouts = Checkouts(
+        fileSystem: fileSystem,
+        parentDirectory: fileSystem.directory(rootDir),
+        platform: platform,
+        processManager: processManager,
+        stdio: stdio,
+      );
+
+      final FrameworkRepository framework = FrameworkRepository(checkouts);
+      expect(
+        framework.ciYaml.enabledBranches,
+        <String>['master', 'dev', 'beta', 'stable'],
+      );
+
+      framework.ciYaml.enableBranch('foo');
+      expect(
+        framework.ciYaml.enabledBranches,
+        <String>['foo', 'master', 'dev', 'beta', 'stable'],
+      );
+
+      expect(
+        framework.ciYaml.stringContents,
+        '''
+enabled_branches:
+  - foo
+  - master
+  - dev
+  - beta
+  - stable
+'''
+      );
+    });
+
+    test('ciYaml.enableBranch() will throw if the input branch is already present in the yaml file', () {
+      const String commit1 = 'abc123';
+      final TestStdio stdio = TestStdio();
+      final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+      final File ciYaml = fileSystem.file('/flutter_conductor_checkouts/framework/.ci.yaml');
+      final ProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+        FakeCommand(
+            command: <String>[
+          'git',
+          'clone',
+          '--origin',
+          'upstream',
+          '--',
+          FrameworkRepository.defaultUpstream,
+          fileSystem.path
+              .join(rootDir, 'flutter_conductor_checkouts', 'framework'),
+        ],
+        onRun: () {
+          ciYaml.createSync(recursive: true);
+          ciYaml.writeAsStringSync('''
+enabled_branches:
+  - master
+  - dev
+  - beta
+  - stable
+''');
+        }),
+        const FakeCommand(command: <String>[
+          'git',
+          'rev-parse',
+          'HEAD',
+        ], stdout: commit1),
+      ]);
+      final Checkouts checkouts = Checkouts(
+        fileSystem: fileSystem,
+        parentDirectory: fileSystem.directory(rootDir),
+        platform: platform,
+        processManager: processManager,
+        stdio: stdio,
+      );
+
+      final FrameworkRepository framework = FrameworkRepository(checkouts);
+      expect(
+        () => framework.ciYaml.enableBranch('master'),
+        throwsExceptionWith('.ci.yaml already contains the branch master'),
+      );
+    });
   });
 }
 

dev/conductor/test/start_test.dart

@@ -161,6 +161,10 @@ void main() {
             'cherrypicks-$candidateBranch',
           ],
         ),
+        const FakeCommand(
+          command: <String>['git', 'status', '--porcelain'],
+          stdout: 'MM path/to/DEPS',
+        ),
         const FakeCommand(
           command: <String>['git', 'add', '--all'],
         ),
@@ -272,6 +276,7 @@ void main() {
         jsonDecode(stateFile.readAsStringSync()),
       );
 
+      expect(processManager.hasRemainingExpectations, false);
       expect(state.isInitialized(), true);
       expect(state.releaseChannel, releaseChannel);
       expect(state.releaseVersion, nextVersion);