Merge a clean revert without waiting for checks #1498

keyonghan · 2021-12-06T19:33:00Z

keyonghan · 2021-12-06T19:58:01Z

app_dart/lib/src/request_handlers/check_for_waiting_pull_requests.dart

+      final RegExp commitRegExp = RegExp(_regExpCommitMessage);
+      final RegExpMatch? match = commitRegExp.firstMatch(queryResult.message);
+      String? revertCommitSha;
+      if (match != null) {
+        revertCommitSha = match.namedGroup('revertCommitSha');
+      }


Github doesn't treat revert PR/commit separately, and there is not a simple mapping between the revert and the to-be-reverted commit.

Checked recent reverts, and all are with the default commit message which includes the to-be-reverted commit sha info. (this is what is proposed here).

Another more stable way is use git diff between the current revert commit and the second TOT commit: if diff is zero, then we can conclude the current revert is based on the TOT commit. Filed SpinlockLabs/github.dart#283 to request support on git diff comparison.

Please advise if any better ways.

With flutter/flutter@86cb259 as an example,

Cocoon could grab

The proposed revert PR diff: https://github.com/flutter/flutter/commit/86cb2598a3b683ebb7bdb9ed4ba6bb0b57424d05.patch

The tip of tree commit diff: https://github.com/flutter/flutter/commit/9412edf1ca69121fe733160e22bc48131e7a3fcd.patch

It can then split the lines of each, and for each line that starts with + or - add it to a map. When going through the second diff, it can reverse the operator and remove it from the map. If the map has any keys remaining, it's not a pure revert and should be skipped.

https://docs.github.com/en/rest/reference/commits#compare-two-commits does exactly the same thing. It sounds a re-implementation of this API. Submitted SpinlockLabs/github.dart#285 to support the files field so that we can use this API.

This is the diff result of a revert commit: flutter/flutter@9d2bf91 and the second tot commit: 86d229dc399136aa7a0b3935f723ffa1289217f8
https://api.github.com/repos/flutter/flutter/compare/86d229dc399136aa7a0b3935f723ffa1289217f8...9d2bf9140499984aff7d78145029cfc2a39a762d, where the empty files field validates this is a clean revert.

CaseyHillers · 2022-01-10T17:59:49Z

app_dart/lib/src/request_handlers/check_for_waiting_pull_requests.dart

@@ -90,6 +91,23 @@ class CheckForWaitingPullRequests extends ApiRequestHandler<Body> {
    }
  }

+  /// Check if the `commitSha` is a clean revert of TOT commit.


Suggested change

/// Check if the `commitSha` is a clean revert of TOT commit.

/// Check if the `commitSha` is a clean revert of Tip Of Tree commit (TOT).

CaseyHillers · 2022-01-10T18:02:05Z

app_dart/lib/src/request_handlers/check_for_waiting_pull_requests.dart

@@ -67,7 +67,8 @@ class CheckForWaitingPullRequests extends ApiRequestHandler<Body> {
    );
    final List<_AutoMergeQueryResult> queryResults = await _parseQueryData(data, slug.name);
    for (_AutoMergeQueryResult queryResult in queryResults) {
-      if (mergeCount < _kMergeCountPerCycle && queryResult.shouldMerge) {
+      // If the PR is a revert of the tot commit, merge without waiting for checks passing.
+      if (mergeCount < _kMergeCountPerCycle && queryResult.shouldMerge || await isTOTRevert(queryResult.sha, slug)) {


Could we simplify this? Maybe exit early by checking _kMergeCountPerCycle < mergeCount and add a helper function

Added a helper function to make it clear.

CaseyHillers · 2022-01-10T18:03:08Z

app_dart/lib/src/request_handlers/check_for_waiting_pull_requests.dart

+    final RepositoryCommit secondTotCommit = await github.repositories.getCommit(slug, 'HEAD~');
+    log.info('Current commit is: $commitSha');
+    log.info('Second TOT commit is: ${secondTotCommit.sha}');
+    final GitHubComparison gitHubComparison =


nit: gitHub -> github

CaseyHillers · 2022-01-10T18:03:23Z

app_dart/pubspec.yaml

@@ -19,7 +19,7 @@ dependencies:
  file: ^6.1.2
  fixnum: ^1.0.0
  gcloud: ^0.8.4
-  github: ^8.2.4
+  github: ^8.3.0


Nice! Thanks for upstreaming this logic

CaseyHillers · 2022-01-10T18:03:47Z

app_dart/test/request_handlers/check_for_waiting_pull_requests_test.dart

@@ -362,6 +387,34 @@ void main() {
      ]);
    });

+    test('Merge a clean revert PR with in progress tests', () async {


Can we add a test for a non-clean revert?

Please see githubComparison = GitHubComparison('abc', 'def', 0, 0, 0, <CommitFile>[CommitFile(name: 'test')]); in other tests, which is mocking the non-clean revert cases.

CaseyHillers

google-cla bot added the cla: yes label Dec 6, 2021

keyonghan commented Dec 6, 2021

View reviewed changes

keyonghan requested review from CaseyHillers and godofredoc December 6, 2021 20:37

keyonghan force-pushed the ignore_checks_for_clean_revert branch 2 times, most recently from c3a495d to f710c3b Compare December 21, 2021 01:01

keyonghan requested a review from yusuf-goog January 4, 2022 20:09

CaseyHillers reviewed Jan 10, 2022

View reviewed changes

keyonghan added 5 commits January 10, 2022 15:46

merge clean revert

7236700

format

417b176

github api

41b093c

remove message

d76a886

add helper function

e1980d4

keyonghan force-pushed the ignore_checks_for_clean_revert branch from f710c3b to e1980d4 Compare January 11, 2022 00:15

CaseyHillers approved these changes Jan 11, 2022

View reviewed changes

keyonghan added the waiting for tree to go green Merge PR when tree becomes green via fluttergithubbot label Jan 11, 2022

fluttergithubbot merged commit 2d9f276 into flutter:main Jan 11, 2022

keyonghan mentioned this pull request Jan 11, 2022

Cocoon merges Engine PR with failed builds flutter/flutter#96478

Closed

keyonghan mentioned this pull request Jul 26, 2022

Land ToT revert immediately without waiting for validations #2009

Closed

keyonghan deleted the ignore_checks_for_clean_revert branch March 13, 2024 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge a clean revert without waiting for checks #1498

Merge a clean revert without waiting for checks #1498

keyonghan commented Dec 6, 2021

keyonghan Dec 6, 2021 •

edited

Loading

CaseyHillers Dec 6, 2021

keyonghan Dec 15, 2021

CaseyHillers Jan 10, 2022

keyonghan Jan 11, 2022

CaseyHillers Jan 10, 2022

keyonghan Jan 11, 2022

CaseyHillers Jan 10, 2022

keyonghan Jan 11, 2022

CaseyHillers Jan 10, 2022

CaseyHillers Jan 10, 2022

keyonghan Jan 11, 2022

CaseyHillers left a comment

	/// Check if the `commitSha` is a clean revert of TOT commit.
	/// Check if the `commitSha` is a clean revert of Tip Of Tree commit (TOT).

Merge a clean revert without waiting for checks #1498

Merge a clean revert without waiting for checks #1498

Conversation

keyonghan commented Dec 6, 2021

keyonghan Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaseyHillers left a comment

Choose a reason for hiding this comment

keyonghan Dec 6, 2021 •

edited

Loading