diff --git a/dev/conductor/core/lib/src/git.dart b/dev/conductor/core/lib/src/git.dart index 4418aabcce..7fb5addc0c 100644 --- a/dev/conductor/core/lib/src/git.dart +++ b/dev/conductor/core/lib/src/git.dart @@ -74,15 +74,55 @@ class Git { if ((result.stderr as String).isNotEmpty) { message.writeln('stderr from git:\n${result.stderr}\n'); } - throw GitException(message.toString()); + throw GitException(message.toString(), args); } } +enum GitExceptionType { + /// Git push failed because the remote branch contained commits the local did + /// not. + /// + /// Either the local branch was wrong, and needs a rebase before pushing + /// again, or the remote branch needs to be overwritten with a force push. + /// + /// Example output: + /// + /// ``` + /// To github.com:user/engine.git + /// + /// ! [rejected] HEAD -> cherrypicks-flutter-2.8-candidate.3 (non-fast-forward) + /// error: failed to push some refs to 'github.com:user/engine.git' + /// hint: Updates were rejected because the tip of your current branch is behind + /// hint: its remote counterpart. Integrate the remote changes (e.g. + /// hint: 'git pull ...') before pushing again. + /// hint: See the 'Note about fast-forwards' in 'git push --help' for details. + /// ``` + PushRejected, +} + +/// An exception created because a git subprocess failed. +/// +/// Known git failures will be assigned a [GitExceptionType] in the [type] +/// field. If this field is null it means and unknown git failure. class GitException implements Exception { - GitException(this.message); + GitException(this.message, this.args) { + if (_pushRejectedPattern.hasMatch(message)) { + type = GitExceptionType.PushRejected; + } else { + // because type is late final, it must be explicitly set before it is + // accessed. + type = null; + } + } + + static final RegExp _pushRejectedPattern = RegExp( + r'Updates were rejected because the tip of your current branch is behind', + ); final String message; + final List args; + late final GitExceptionType? type; @override - String toString() => 'Exception: $message'; + String toString() => 'Exception on command "${args.join(' ')}": $message'; } diff --git a/dev/conductor/core/lib/src/next.dart b/dev/conductor/core/lib/src/next.dart index 490f7c911c..9ece748e56 100644 --- a/dev/conductor/core/lib/src/next.dart +++ b/dev/conductor/core/lib/src/next.dart @@ -4,8 +4,10 @@ import 'package:args/command_runner.dart'; import 'package:file/file.dart' show File; +import 'package:meta/meta.dart' show visibleForTesting; import 'context.dart'; +import 'git.dart'; import 'globals.dart'; import 'proto/conductor_state.pb.dart' as pb; import 'proto/conductor_state.pbenum.dart'; @@ -152,13 +154,7 @@ class NextContext extends Context { } } - await engine.pushRef( - fromRef: 'HEAD', - // Explicitly create new branch - toRef: 'refs/heads/${state.engine.workingBranch}', - remote: state.engine.mirror.name, - ); - + await pushWorkingBranch(engine, state.engine); break; case pb.ReleasePhase.CODESIGN_ENGINE_BINARIES: stdio.printStatus([ @@ -285,12 +281,7 @@ class NextContext extends Context { } } - await framework.pushRef( - fromRef: 'HEAD', - // Explicitly create new branch - toRef: 'refs/heads/${state.framework.workingBranch}', - remote: state.framework.mirror.name, - ); + await pushWorkingBranch(framework, state.framework); break; case pb.ReleasePhase.PUBLISH_VERSION: stdio.printStatus('Please ensure that you have merged your framework PR and that'); @@ -381,4 +372,37 @@ class NextContext extends Context { updateState(state, stdio.logs); } + + /// Push the working branch to the user's mirror. + /// + /// [repository] represents the actual Git repository on disk, and is used to + /// call `git push`, while [pbRepository] represents the user-specified + /// configuration for the repository, and is used to read the name of the + /// working branch and the mirror's remote name. + /// + /// May throw either a [ConductorException] if the user already has a branch + /// of the same name on their mirror, or a [GitException] for any other + /// failures from the underlying git process call. + @visibleForTesting + Future pushWorkingBranch(Repository repository, pb.Repository pbRepository) async { + try { + await repository.pushRef( + fromRef: 'HEAD', + // Explicitly create new branch + toRef: 'refs/heads/${pbRepository.workingBranch}', + remote: pbRepository.mirror.name, + force: force, + ); + } on GitException catch (exception) { + if (exception.type == GitExceptionType.PushRejected && force == false) { + throw ConductorException( + 'Push failed because the working branch named ' + '${pbRepository.workingBranch} already exists on your mirror. ' + 'Re-run this command with --force to overwrite the remote branch.\n' + '${exception.message}', + ); + } + rethrow; + } + } } diff --git a/dev/conductor/core/lib/src/repository.dart b/dev/conductor/core/lib/src/repository.dart index dc4b703a6f..36e44403aa 100644 --- a/dev/conductor/core/lib/src/repository.dart +++ b/dev/conductor/core/lib/src/repository.dart @@ -556,7 +556,7 @@ class FrameworkRepository extends Repository { } @override - Future cloneRepository(String? cloneName) async { + Future cloneRepository(String? cloneName) async { assert(localUpstream); cloneName ??= 'clone-of-$name'; return FrameworkRepository( diff --git a/dev/conductor/core/test/next_test.dart b/dev/conductor/core/test/next_test.dart index 9036498d04..1eb17036ac 100644 --- a/dev/conductor/core/test/next_test.dart +++ b/dev/conductor/core/test/next_test.dart @@ -3,6 +3,8 @@ // found in the LICENSE file. import 'package:args/command_runner.dart'; +import 'package:conductor_core/src/git.dart'; +import 'package:conductor_core/src/globals.dart'; import 'package:conductor_core/src/next.dart'; import 'package:conductor_core/src/proto/conductor_state.pb.dart' as pb; import 'package:conductor_core/src/proto/conductor_state.pbenum.dart' show ReleasePhase; @@ -16,23 +18,24 @@ import 'package:platform/platform.dart'; import './common.dart'; void main() { + const String flutterRoot = '/flutter'; + const String checkoutsParentDirectory = '$flutterRoot/dev/conductor'; + const String candidateBranch = 'flutter-1.2-candidate.3'; + const String workingBranch = 'cherrypicks-$candidateBranch'; + const String remoteUrl = 'https://github.com/org/repo.git'; + const String revision1 = 'd3af60d18e01fcb36e0c0fa06c8502e4935ed095'; + const String revision2 = 'f99555c1e1392bf2a8135056b9446680c2af4ddf'; + const String revision3 = '98a5ca242b9d270ce000b26309b8a3cdc9c89df5'; + const String revision4 = '280e23318a0d8341415c66aa32581352a421d974'; + const String releaseVersion = '1.2.0-3.0.pre'; + const String releaseChannel = 'beta'; + const String stateFile = '/state-file.json'; + final String localPathSeparator = const LocalPlatform().pathSeparator; + final String localOperatingSystem = const LocalPlatform().pathSeparator; + group('next command', () { - const String flutterRoot = '/flutter'; - const String checkoutsParentDirectory = '$flutterRoot/dev/conductor'; - const String candidateBranch = 'flutter-1.2-candidate.3'; - const String workingBranch = 'cherrypicks-$candidateBranch'; - const String remoteUrl = 'https://github.com/org/repo.git'; - final String localPathSeparator = const LocalPlatform().pathSeparator; - final String localOperatingSystem = const LocalPlatform().pathSeparator; - const String revision1 = 'd3af60d18e01fcb36e0c0fa06c8502e4935ed095'; - const String revision2 = 'f99555c1e1392bf2a8135056b9446680c2af4ddf'; - const String revision3 = '98a5ca242b9d270ce000b26309b8a3cdc9c89df5'; - const String revision4 = '280e23318a0d8341415c66aa32581352a421d974'; - const String releaseVersion = '1.2.0-3.0.pre'; - const String releaseChannel = 'beta'; late MemoryFileSystem fileSystem; late TestStdio stdio; - const String stateFile = '/state-file.json'; setUp(() { stdio = TestStdio(); @@ -1102,6 +1105,68 @@ void main() { ); }); }); + + group('.pushWorkingBranch()', () { + late MemoryFileSystem fileSystem; + late TestStdio stdio; + late Platform platform; + + setUp(() { + stdio = TestStdio(); + fileSystem = MemoryFileSystem.test(); + platform = FakePlatform(); + }); + + test('catches GitException if the push was rejected and instead throws a helpful ConductorException', () async { + const String gitPushErrorMessage = ''' + To github.com:user/engine.git + + ! [rejected] HEAD -> cherrypicks-flutter-2.8-candidate.3 (non-fast-forward) + error: failed to push some refs to 'github.com:user/engine.git' + hint: Updates were rejected because the tip of your current branch is behind + hint: its remote counterpart. Integrate the remote changes (e.g. + hint: 'git pull ...') before pushing again. + hint: See the 'Note about fast-forwards' in 'git push --help' for details. +'''; + final Checkouts checkouts = Checkouts( + fileSystem: fileSystem, + parentDirectory: fileSystem.directory(checkoutsParentDirectory)..createSync(recursive: true), + platform: platform, + processManager: FakeProcessManager.empty(), + stdio: stdio, + ); + final Repository testRepository = _TestRepository.fromCheckouts(checkouts); + final pb.Repository testPbRepository = pb.Repository(); + (checkouts.processManager as FakeProcessManager).addCommands([ + FakeCommand( + command: ['git', 'clone', '--origin', 'upstream', '--', testRepository.upstreamRemote.url, '/flutter/dev/conductor/flutter_conductor_checkouts/test-repo/test-repo'], + ), + const FakeCommand( + command: ['git', 'rev-parse', 'HEAD'], + stdout: revision1, + ), + FakeCommand( + command: const ['git', 'push', '', 'HEAD:refs/heads/'], + exception: GitException(gitPushErrorMessage, ['git', 'push', '--force', '', 'HEAD:refs/heads/']), + ) + ]); + final NextContext nextContext = NextContext( + autoAccept: false, + checkouts: checkouts, + force: false, + stateFile: fileSystem.file(stateFile), + ); + + expect( + () => nextContext.pushWorkingBranch(testRepository, testPbRepository), + throwsA(isA().having( + (ConductorException exception) => exception.message, + 'has correct message', + contains('Re-run this command with --force to overwrite the remote branch'), + )), + ); + }); + }); } /// A [Stdio] that will throw an exception if any of its methods are called. @@ -1135,6 +1200,24 @@ class _UnimplementedStdio implements Stdio { String readLineSync() => _throw(); } +class _TestRepository extends Repository { + _TestRepository.fromCheckouts(Checkouts checkouts, [String name = 'test-repo']) : super( + fileSystem: checkouts.fileSystem, + parentDirectory: checkouts.directory.childDirectory(name), + platform: checkouts.platform, + processManager: checkouts.processManager, + name: name, + requiredLocalBranches: [], + stdio: checkouts.stdio, + upstreamRemote: const Remote(name: RemoteName.upstream, url: 'git@github.com:upstream/repo.git'), + ); + + @override + Future<_TestRepository> cloneRepository(String? cloneName) async { + throw Exception('Unimplemented!'); + } +} + class _TestNextContext extends NextContext { const _TestNextContext({ bool autoAccept = false,