Merge #16223: devtools: Fetch and display ACKs at sign-off time in github-merge

0e01e4522e devtools: Fetch and display ACKs at sign-off time in github-merge (Wladimir J. van der Laan)

Pull request description:

  - Fetch the ACKs only at sign-off time. This makes sure that any last-minute ACKs are included (fixes #16200)
  - Show a list of ACKs that will be included and their author before signing off, and warn if there are none

  ![1](https://user-images.githubusercontent.com/126646/59605250-ad070980-910e-11e9-9f9a-d789c7f06ebb.png)
  ![2](https://user-images.githubusercontent.com/126646/59605255-b1332700-910e-11e9-80a5-d1e244f48264.png)

  There's a slight change to the merge commit format—before it was
  ```
      ACKs for commit 88884c:
  (list of ACKs, could be empty)
  ```
  now it is
  ```
  ACKs for top commit:
        jnewbery:
          ACK 5ebc6b0eb
      ... (list of ACKs cannot be empty)
  ```
  or
  ```
  Top commit has no ACKs.
  ```
  I don't think there's a reason to have the abbreviated commit ID there, after all the full commit id is already in the beginning of the merge commit message, and at least the abbreviated one is in every single ACK message.

ACKs for commit 0e01e4:
  fanquake:
    ACK 0e01e4522e0f9e2e994f80fc61f40d61a4a70d79

Tree-SHA512: 8576de016137d71cfc101747e9bb6779c13e0953cf2babee7afc9972bf2bd46f6912be4982b54fa5abf4d91e98e8fdae6b4ca3eef7d6892b7a5f04a7017b6882

1 files changed, 49 insertions, 19 deletions

diff --git a/contrib/devtools/github-merge.py b/contrib/devtools/github-merge.py
index 03179cec24..cd7a271e83 100755
--- a/contrib/devtools/github-merge.py
+++ b/contrib/devtools/github-merge.py
@@ -32,10 +32,14 @@ BASH = os.getenv('BASH','bash')
 # OS specific configuration for terminal attributes
 ATTR_RESET = ''
 ATTR_PR = ''
+ATTR_NAME = ''
+ATTR_WARN = ''
 COMMIT_FORMAT = '%H %s (%an)%d'
 if os.name == 'posix': # if posix, assume we can use basic terminal escapes
     ATTR_RESET = '\033[0m'
     ATTR_PR = '\033[1;36m'
+    ATTR_NAME = '\033[0;36m'
+    ATTR_WARN = '\033[1;31m'
     COMMIT_FORMAT = '%C(bold blue)%H%Creset %s %C(cyan)(%an)%Creset%C(green)%d%Creset'
 
 def git_config_get(option, default=None):
@@ -164,18 +168,36 @@ def tree_sha512sum(commit='HEAD'):
     return overall.hexdigest()
 
 def get_acks_from_comments(head_commit, comments):
-    assert len(head_commit) == 6
-    ack_str ='\n\nACKs for commit {}:\n'.format(head_commit)
+    # Look for abbreviated commit id, because not everyone wants to type/paste
+    # the whole thing and the chance of collisions within a PR is small enough
+    head_abbrev = head_commit[0:6]
+    acks = []
     for c in comments:
-        review = [l for l in c['body'].split('\r\n') if 'ACK' in l and head_commit in l]
+        review = [l for l in c['body'].split('\r\n') if 'ACK' in l and head_abbrev in l]
         if review:
-            ack_str += '  {}:\n'.format(c['user']['login'])
-            ack_str += '    {}\n'.format(review[0])
+            acks.append((c['user']['login'], review[0]))
+    return acks
+
+def make_acks_message(head_commit, acks):
+    if acks:
+        ack_str ='\n\nACKs for top commit:\n'.format(head_commit)
+        for name, msg in acks:
+            ack_str += '  {}:\n'.format(name)
+            ack_str += '    {}\n'.format(msg)
+    else:
+        ack_str ='\n\nTop commit has no ACKs.\n'
     return ack_str
 
-def print_merge_details(pull, title, branch, base_branch, head_branch):
+def print_merge_details(pull, title, branch, base_branch, head_branch, acks):
     print('%s#%s%s %s %sinto %s%s' % (ATTR_RESET+ATTR_PR,pull,ATTR_RESET,title,ATTR_RESET+ATTR_PR,branch,ATTR_RESET))
     subprocess.check_call([GIT,'log','--graph','--topo-order','--pretty=format:'+COMMIT_FORMAT,base_branch+'..'+head_branch])
+    if acks is not None:
+        if acks:
+            print('{}ACKs:{}'.format(ATTR_PR, ATTR_RESET))
+            for (name, message) in acks:
+                print('* {} {}({}){}'.format(message, ATTR_NAME, name, ATTR_RESET))
+        else:
+            print('{}Top commit has no ACKs!{}'.format(ATTR_WARN, ATTR_RESET))
 
 def parse_arguments():
     epilog = '''
@@ -225,9 +247,6 @@ def main():
     info = retrieve_pr_info(repo,pull,ghtoken)
     if info is None:
         sys.exit(1)
-    comments = retrieve_pr_comments(repo,pull,ghtoken) + retrieve_pr_reviews(repo,pull,ghtoken)
-    if comments is None:
-        sys.exit(1)
     title = info['title'].strip()
     body = info['body'].strip()
     # precedence order for destination branch argument:
@@ -257,6 +276,8 @@ def main():
         sys.exit(3)
     try:
         subprocess.check_call([GIT,'log','-q','-1','refs/heads/'+head_branch], stdout=devnull, stderr=stdout)
+        head_commit = subprocess.check_output([GIT,'log','-1','--pretty=format:%H',head_branch]).decode('utf-8')
+        assert len(head_commit) == 40
     except subprocess.CalledProcessError:
         print("ERROR: Cannot find head of pull request #%s on %s." % (pull,host_repo), file=stderr)
         sys.exit(3)
@@ -281,7 +302,6 @@ def main():
         message = firstline + '\n\n'
         message += subprocess.check_output([GIT,'log','--no-merges','--topo-order','--pretty=format:%H %s (%an)',base_branch+'..'+head_branch]).decode('utf-8')
         message += '\n\nPull request description:\n\n  ' + body.replace('\n', '\n  ') + '\n'
-        message += get_acks_from_comments(head_commit=subprocess.check_output([GIT,'log','-1','--pretty=format:%H',head_branch]).decode('utf-8')[:6], comments=comments)
         try:
             subprocess.check_call([GIT,'merge','-q','--commit','--no-edit','--no-ff','--no-gpg-sign','-m',message.encode('utf-8'),head_branch])
         except subprocess.CalledProcessError:
@@ -299,20 +319,14 @@ def main():
         if len(symlink_files) > 0:
             sys.exit(4)
 
-        # Put tree SHA512 into the message
+        # Compute SHA512 of git tree (to be able to detect changes before sign-off)
         try:
             first_sha512 = tree_sha512sum()
-            message += '\n\nTree-SHA512: ' + first_sha512
         except subprocess.CalledProcessError:
             print("ERROR: Unable to compute tree hash")
             sys.exit(4)
-        try:
-            subprocess.check_call([GIT,'commit','--amend','--no-gpg-sign','-m',message.encode('utf-8')])
-        except subprocess.CalledProcessError:
-            print("ERROR: Cannot update message.", file=stderr)
-            sys.exit(4)
 
-        print_merge_details(pull, title, branch, base_branch, head_branch)
+        print_merge_details(pull, title, branch, base_branch, head_branch, None)
         print()
 
         # Run test command if configured.
@@ -345,8 +359,24 @@ def main():
             print("ERROR: Tree hash changed unexpectedly",file=stderr)
             sys.exit(8)
 
+        # Retrieve PR comments and ACKs and add to commit message, store ACKs to print them with commit
+        # description
+        comments = retrieve_pr_comments(repo,pull,ghtoken) + retrieve_pr_reviews(repo,pull,ghtoken)
+        if comments is None:
+            print("ERROR: Could not fetch PR comments and reviews",file=stderr)
+            sys.exit(1)
+        acks = get_acks_from_comments(head_commit=head_commit, comments=comments)
+        message += make_acks_message(head_commit=head_commit, acks=acks)
+        # end message with SHA512 tree hash, then update message
+        message += '\n\nTree-SHA512: ' + first_sha512
+        try:
+            subprocess.check_call([GIT,'commit','--amend','--no-gpg-sign','-m',message.encode('utf-8')])
+        except subprocess.CalledProcessError:
+            print("ERROR: Cannot update message.", file=stderr)
+            sys.exit(4)
+
         # Sign the merge commit.
-        print_merge_details(pull, title, branch, base_branch, head_branch)
+        print_merge_details(pull, title, branch, base_branch, head_branch, acks)
         while True:
             reply = ask_prompt("Type 's' to sign off on the above merge, or 'x' to reject and exit.").lower()
             if reply == 's':