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

- 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 and their author before signing off, and warn if
  there are none

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':