[flutter_tools] Update roll_dev.dart (#59215)

dev/tools/lib/roll_dev.dart

@@ -22,6 +22,7 @@ const String kJustPrint = 'just-print';
 const String kYes = 'yes';
 const String kHelp = 'help';
 const String kForce = 'force';
+const String kSkipTagging = 'skip-tagging';
 
 const String kUpstreamRemote = 'git@github.com:flutter/flutter.git';
 
@@ -64,6 +65,7 @@ bool run({
   final bool autoApprove = argResults[kYes] as bool;
   final bool help = argResults[kHelp] as bool;
   final bool force = argResults[kForce] as bool;
+  final bool skipTagging = argResults[kSkipTagging] as bool;
 
   if (help || level == null || commit == null) {
     print(
@@ -79,8 +81,8 @@ bool run({
   );
   if (remote != kUpstreamRemote) {
     throw Exception(
-      'The current directory is not a Flutter repository checkout with a '
-      'correctly configured upstream remote.\nFor more details see: '
+      'The remote named $origin is set to $remote, when $kUpstreamRemote was '
+      'expected.\nFor more details see: '
       'https://github.com/flutter/flutter/wiki/Release-process'
     );
   }
@@ -92,22 +94,45 @@ bool run({
     );
   }
 
-  // TODO(fujino): move this after `justPrint`
   git.run('fetch $origin', 'fetch $origin');
-  git.run('reset $commit --hard', 'reset to the release commit');
 
-  String version = getFullTag(git, origin);
+  final String lastVersion = getFullTag(git, origin);
+
+  final String version = skipTagging
+    ? lastVersion
+    : incrementLevel(lastVersion, level);
 
-  version = incrementLevel(version, level);
+  if (git.getOutput(
+    'rev-parse $lastVersion',
+    'check if commit is already on dev',
+  ).contains(commit.trim())) {
+    throw Exception('Commit $commit is already on the dev branch as $lastVersion.');
+  }
 
   if (justPrint) {
     print(version);
     return false;
   }
 
-  final String hash = git.getOutput('rev-parse HEAD', 'Get git hash for $commit');
+  if (skipTagging) {
+    git.run(
+      'describe --exact-match --tags $commit',
+      'verify $commit is already tagged. You can only use the flag '
+      '`$kSkipTagging` if the commit has already been tagged.'
+    );
+  }
 
-  git.run('tag $version', 'tag the commit with the version label');
+  if (!force) {
+    git.run(
+      'merge-base --is-ancestor $lastVersion $commit',
+      'verify $lastVersion is a direct ancestor of $commit. The flag `$kForce`'
+      'is required to force push a new release past a cherry-pick',
+    );
+  }
+
+  git.run('reset $commit --hard', 'reset to the release commit');
+
+  final String hash = git.getOutput('rev-parse HEAD', 'Get git hash for $commit');
 
   // PROMPT
 
@@ -118,13 +143,15 @@ bool run({
       'to the "dev" channel.');
     stdout.write('Are you? [yes/no] ');
     if (stdin.readLineSync() != 'yes') {
-      git.run('tag -d $version', 'remove the tag you did not want to publish');
       print('The dev roll has been aborted.');
       return false;
     }
   }
 
+  if (!skipTagging) {
+    git.run('tag $version', 'tag the commit with the version label');
     git.run('push $origin $version', 'publish the version');
+  }
   git.run(
     'push ${force ? "--force " : ""}$origin HEAD:dev',
     'land the new version on the "dev" branch',
@@ -170,6 +197,12 @@ ArgResults parseArguments(ArgParser argParser, List<String> args) {
         "Don't actually roll the dev channel; "
         'just print the would-be version and quit.',
   );
+  argParser.addFlag(
+    kSkipTagging,
+    negatable: false,
+    help: 'Do not create tag and push to remote, only update release branch. '
+    'For recovering when the script fails trying to git push to the release branch.'
+  );
   argParser.addFlag(kYes, negatable: false, abbr: 'y', help: 'Skip the confirmation prompt.');
   argParser.addFlag(kHelp, negatable: false, help: 'Show this help message.', hide: true);
 
@@ -208,6 +241,7 @@ String getVersionFromParts(List<int> parts) {
   return buf.toString();
 }
 
+/// A wrapper around git process calls that can be mocked for unit testing.
 class Git {
   const Git();
 
@@ -230,16 +264,17 @@ class Git {
   }
 
   void _reportFailureAndExit(ProcessResult result, String explanation) {
+    final StringBuffer message = StringBuffer();
     if (result.exitCode != 0) {
-      print('Failed to $explanation. Git exited with error code ${result.exitCode}.');
+      message.writeln('Failed to $explanation. Git exited with error code ${result.exitCode}.');
     } else {
-      print('Failed to $explanation.');
+      message.writeln('Failed to $explanation.');
     }
     if ((result.stdout as String).isNotEmpty)
-      print('stdout from git:\n${result.stdout}\n');
+      message.writeln('stdout from git:\n${result.stdout}\n');
     if ((result.stderr as String).isNotEmpty)
-      print('stderr from git:\n${result.stderr}\n');
-    exit(1);
+      message.writeln('stderr from git:\n${result.stderr}\n');
+    throw Exception(message);
   }
 }
 

dev/tools/test/roll_dev_test.dart

@@ -14,6 +14,8 @@ void main() {
     const String level = 'z';
     const String commit = 'abcde012345';
     const String origin = 'upstream';
+    const String lastVersion = '1.2.0-0.0.pre';
+    const String nextVersion = '1.2.0-1.0.pre';
     FakeArgResults fakeArgResults;
     MockGit mockGit;
 
@@ -26,8 +28,6 @@ void main() {
         level: level,
         commit: commit,
         origin: origin,
-        justPrint: false,
-        autoApprove: true,
         help: true,
       );
       expect(
@@ -42,12 +42,9 @@ void main() {
 
     test('returns false if level not provided', () {
       fakeArgResults = FakeArgResults(
-        level: level,
+        level: null,
         commit: commit,
         origin: origin,
-        justPrint: false,
-        autoApprove: true,
-        help: true,
       );
       expect(
         run(
@@ -62,11 +59,8 @@ void main() {
     test('returns false if commit not provided', () {
       fakeArgResults = FakeArgResults(
         level: level,
-        commit: commit,
+        commit: null,
         origin: origin,
-        justPrint: false,
-        autoApprove: true,
-        help: true,
       );
       expect(
         run(
@@ -79,28 +73,22 @@ void main() {
     });
 
     test('throws exception if upstream remote wrong', () {
-      when(mockGit.getOutput('remote get-url $origin', any)).thenReturn('wrong-remote');
+      const String remote = 'wrong-remote';
+      when(mockGit.getOutput('remote get-url $origin', any)).thenReturn(remote);
       fakeArgResults = FakeArgResults(
         level: level,
         commit: commit,
         origin: origin,
-        justPrint: false,
-        autoApprove: true,
-        help: false,
       );
-      Exception exception;
-      try {
-        run(
+      const String errorMessage = 'The remote named $origin is set to $remote, when $kUpstreamRemote was expected.';
+      expect(
+        () => run(
           usage: usage,
           argResults: fakeArgResults,
           git: mockGit,
+        ),
+        throwsExceptionWith(errorMessage),
       );
-      } on Exception catch (e) {
-        exception = e;
-      }
-      const String pattern = r'The current directory is not a Flutter '
-        'repository checkout with a correctly configured upstream remote.';
-      expect(exception?.toString(), contains(pattern));
     });
 
     test('throws exception if git checkout not clean', () {
@@ -112,9 +100,6 @@ void main() {
         level: level,
         commit: commit,
         origin: origin,
-        justPrint: false,
-        autoApprove: true,
-        help: false,
       );
       Exception exception;
       try {
@@ -132,20 +117,22 @@ void main() {
       expect(exception?.toString(), contains(pattern));
     });
 
-    test('does not tag if --just-print is specified', () {
+    test('does not reset or tag if --just-print is specified', () {
       when(mockGit.getOutput('remote get-url $origin', any)).thenReturn(kUpstreamRemote);
       when(mockGit.getOutput('status --porcelain', any)).thenReturn('');
       when(mockGit.getOutput(
         'describe --match *.*.*-*.*.pre --exact-match --tags refs/remotes/$origin/dev',
         any,
-      )).thenReturn('1.2.3-0.0.pre');
+      )).thenReturn(lastVersion);
+      when(mockGit.getOutput(
+        'rev-parse $lastVersion',
+        any,
+      )).thenReturn('zxy321');
       fakeArgResults = FakeArgResults(
         level: level,
         commit: commit,
         origin: origin,
         justPrint: true,
-        autoApprove: true,
-        help: false,
       );
       expect(run(
         usage: usage,
@@ -153,25 +140,172 @@ void main() {
         git: mockGit,
       ), false);
       verify(mockGit.run('fetch $origin', any));
-      verify(mockGit.run('reset $commit --hard', any));
+      verifyNever(mockGit.run('reset $commit --hard', any));
       verifyNever(mockGit.getOutput('rev-parse HEAD', any));
     });
 
-    test('successfully tags and publishes release', () {
+    test('exits with exception if --skip-tagging is provided but commit isn\'t '
+         'already tagged', () {
+      when(mockGit.getOutput('remote get-url $origin', any)).thenReturn(kUpstreamRemote);
+      when(mockGit.getOutput('status --porcelain', any)).thenReturn('');
+      when(mockGit.getOutput(
+        'describe --match *.*.*-*.*.pre --exact-match --tags refs/remotes/$origin/dev',
+        any,
+      )).thenReturn(lastVersion);
+      when(mockGit.getOutput(
+        'rev-parse $lastVersion',
+        any,
+      )).thenReturn('zxy321');
+      const String exceptionMessage = 'Failed to verify $commit is already '
+        'tagged. You can only use the flag `$kSkipTagging` if the commit has '
+        'already been tagged.';
+      when(mockGit.run(
+        'describe --exact-match --tags $commit',
+        any,
+      )).thenThrow(Exception(exceptionMessage));
+
+      fakeArgResults = FakeArgResults(
+        level: level,
+        commit: commit,
+        origin: origin,
+        skipTagging: true,
+      );
+      expect(
+        () => run(
+          usage: usage,
+          argResults: fakeArgResults,
+          git: mockGit,
+        ),
+        throwsExceptionWith(exceptionMessage),
+      );
+      verify(mockGit.run('fetch $origin', any));
+      verifyNever(mockGit.run('reset $commit --hard', any));
+      verifyNever(mockGit.getOutput('rev-parse HEAD', any));
+    });
+
+    test('throws exception if desired commit is already tip of dev branch', () {
       when(mockGit.getOutput('remote get-url $origin', any)).thenReturn(kUpstreamRemote);
       when(mockGit.getOutput('status --porcelain', any)).thenReturn('');
       when(mockGit.getOutput(
         'describe --match *.*.*-*.*.pre --exact-match --tags refs/remotes/$origin/dev',
         any,
-      )).thenReturn('1.2.3-0.0.pre');
+      )).thenReturn(lastVersion);
+      when(mockGit.getOutput(
+        'rev-parse $lastVersion',
+        any,
+      )).thenReturn(commit);
+      fakeArgResults = FakeArgResults(
+        level: level,
+        commit: commit,
+        origin: origin,
+        justPrint: true,
+      );
+      expect(
+        () => run(
+          usage: usage,
+          argResults: fakeArgResults,
+          git: mockGit,
+        ),
+        throwsExceptionWith('is already on the dev branch as'),
+      );
+      verify(mockGit.run('fetch $origin', any));
+      verifyNever(mockGit.run('reset $commit --hard', any));
+      verifyNever(mockGit.getOutput('rev-parse HEAD', any));
+    });
+
+    test('does not tag if last release is not direct ancestor of desired '
+        'commit and --force not supplied', () {
+      when(mockGit.getOutput('remote get-url $origin', any))
+        .thenReturn(kUpstreamRemote);
+      when(mockGit.getOutput('status --porcelain', any))
+        .thenReturn('');
+      when(mockGit.getOutput(
+        'describe --match *.*.*-*.*.pre --exact-match --tags refs/remotes/$origin/dev',
+        any,
+      )).thenReturn(lastVersion);
+      when(mockGit.getOutput(
+        'rev-parse $lastVersion',
+        any,
+      )).thenReturn('zxy321');
+      when(mockGit.run('merge-base --is-ancestor $lastVersion $commit', any))
+        .thenThrow(Exception(
+          'Failed to verify $lastVersion is a direct ancestor of $commit. The '
+          'flag `--force` is required to force push a new release past a '
+          'cherry-pick',
+        ));
+      fakeArgResults = FakeArgResults(
+        level: level,
+        commit: commit,
+        origin: origin,
+      );
+      const String errorMessage = 'Failed to verify $lastVersion is a direct '
+        'ancestor of $commit. The flag `--force` is required to force push a '
+        'new release past a cherry-pick';
+      expect(
+        () => run(
+          argResults: fakeArgResults,
+          git: mockGit,
+          usage: usage,
+        ),
+        throwsExceptionWith(errorMessage),
+      );
+
+      verify(mockGit.run('fetch $origin', any));
+      verifyNever(mockGit.run('reset $commit --hard', any));
+      verifyNever(mockGit.run('push $origin HEAD:dev', any));
+      verifyNever(mockGit.run('tag $nextVersion', any));
+    });
+
+    test('does not tag but updates branch if --skip-tagging provided', () {
+      when(mockGit.getOutput('remote get-url $origin', any))
+        .thenReturn(kUpstreamRemote);
+      when(mockGit.getOutput('status --porcelain', any))
+        .thenReturn('');
+      when(mockGit.getOutput(
+        'describe --match *.*.*-*.*.pre --exact-match --tags refs/remotes/$origin/dev',
+        any,
+      )).thenReturn(lastVersion);
+      when(mockGit.getOutput(
+        'rev-parse $lastVersion',
+        any,
+      )).thenReturn('zxy321');
+      when(mockGit.getOutput('rev-parse HEAD', any)).thenReturn(commit);
+      fakeArgResults = FakeArgResults(
+        level: level,
+        commit: commit,
+        origin: origin,
+        skipTagging: true,
+      );
+      expect(run(
+        usage: usage,
+        argResults: fakeArgResults,
+        git: mockGit,
+      ), true);
+      verify(mockGit.run('fetch $origin', any));
+      verify(mockGit.run('reset $commit --hard', any));
+      verifyNever(mockGit.run('tag $nextVersion', any));
+      verifyNever(mockGit.run('push $origin $nextVersion', any));
+      verify(mockGit.run('push $origin HEAD:dev', any));
+    });
+
+    test('successfully tags and publishes release', () {
+      when(mockGit.getOutput('remote get-url $origin', any))
+        .thenReturn(kUpstreamRemote);
+      when(mockGit.getOutput('status --porcelain', any))
+        .thenReturn('');
+      when(mockGit.getOutput(
+        'describe --match *.*.*-*.*.pre --exact-match --tags refs/remotes/$origin/dev',
+        any,
+      )).thenReturn('1.2.0-0.0.pre');
+      when(mockGit.getOutput(
+        'rev-parse $lastVersion',
+        any,
+      )).thenReturn('zxy321');
       when(mockGit.getOutput('rev-parse HEAD', any)).thenReturn(commit);
       fakeArgResults = FakeArgResults(
         level: level,
         commit: commit,
         origin: origin,
-        justPrint: false,
-        autoApprove: true,
-        help: false,
       );
       expect(run(
         usage: usage,
@@ -180,7 +314,8 @@ void main() {
       ), true);
       verify(mockGit.run('fetch $origin', any));
       verify(mockGit.run('reset $commit --hard', any));
-      verify(mockGit.run('tag 1.2.0-1.0.pre', any));
+      verify(mockGit.run('tag $nextVersion', any));
+      verify(mockGit.run('push $origin $nextVersion', any));
       verify(mockGit.run('push $origin HEAD:dev', any));
     });
 
@@ -190,15 +325,16 @@ void main() {
       when(mockGit.getOutput(
         'describe --match *.*.*-*.*.pre --exact-match --tags refs/remotes/$origin/dev',
         any,
-      )).thenReturn('1.2.3-0.0.pre');
+      )).thenReturn(lastVersion);
+      when(mockGit.getOutput(
+        'rev-parse $lastVersion',
+        any,
+      )).thenReturn('zxy321');
       when(mockGit.getOutput('rev-parse HEAD', any)).thenReturn(commit);
       fakeArgResults = FakeArgResults(
         level: level,
         commit: commit,
         origin: origin,
-        justPrint: false,
-        autoApprove: true,
-        help: false,
         force: true,
       );
       expect(run(
@@ -208,7 +344,7 @@ void main() {
       ), true);
       verify(mockGit.run('fetch $origin', any));
       verify(mockGit.run('reset $commit --hard', any));
-      verify(mockGit.run('tag 1.2.0-1.0.pre', any));
+      verify(mockGit.run('tag $nextVersion', any));
       verify(mockGit.run('push --force $origin HEAD:dev', any));
     });
   });
@@ -268,14 +404,14 @@ void main() {
       String version = '1.0.0-0.0.pre-1-g$hash';
       expect(
         () => incrementLevel(version, level),
-        throwsException,
+        throwsExceptionWith('Git reported the latest version as "$version"'),
         reason: 'should throw because $version should be an exact tag',
       );
 
       version = '1.2.3';
       expect(
         () => incrementLevel(version, level),
-        throwsException,
+        throwsExceptionWith('Git reported the latest version as "$version"'),
         reason: 'should throw because $version should be a dev tag, not stable.'
       );
 
@@ -283,7 +419,7 @@ void main() {
       level = 'q';
       expect(
         () => incrementLevel(version, level),
-        throwsException,
+        throwsExceptionWith('Git reported the latest version as "$version"'),
         reason: 'should throw because $level is unsupported',
       );
     });
@@ -329,15 +465,26 @@ void main() {
   });
 }
 
+Matcher throwsExceptionWith(String messageSubString) {
+  return throwsA(
+      isA<Exception>().having(
+          (Exception e) => e.toString(),
+          'description',
+          contains(messageSubString),
+      ),
+  );
+}
+
 class FakeArgResults implements ArgResults {
   FakeArgResults({
     String level,
     String commit,
     String origin,
-    bool justPrint,
-    bool autoApprove,
-    bool help,
+    bool justPrint = false,
+    bool autoApprove = true, // so we don't have to mock stdin
+    bool help = false,
     bool force = false,
+    bool skipTagging = false,
   }) : _parsedArgs = <String, dynamic>{
     'increment': level,
     'commit': commit,
@@ -346,6 +493,7 @@ class FakeArgResults implements ArgResults {
     'yes': autoApprove,
     'help': help,
     'force': force,
+    'skip-tagging': skipTagging,
   };
 
   @override

packages/flutter_tools/test/general.shard/base/process_test.dart

@@ -247,7 +247,6 @@ void main() {
           timeout: timeout,
           timeoutRetries: 0,
         ), throwsA(isA<ProcessException>()));
-
         time.elapse(timeout);
       });
     });