Add a git hook script that can be manually setup to run some checks on every push

[mehdi_amini]

Summary

Right now it just catches arcanist noisy tags, and include a script to
automatically clean these.

Follow up on http://lists.llvm.org/pipermail/llvm-dev/2019-December/137848.html

[MaskRay]

llvm/utils/git/arcfilter.sh

6

This version handles both Summay: ... (on the same line) and Summary:\nmulti lines

git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -

In addition, I added --date=now to reset author date to committer date.

llvm/utils/git/pre-push.py

1

I think we can just use python3 and drop Python 2.7 support.

[MaskRay]

Thanks for the patch. git pre-hook seems to be a clever trick. I need to play with this.

[mehdi_amini]

llvm/utils/git/arcfilter.sh

6

I'll update the awk, why should we add --date=now ?

[Higuoxing]

Thanks! I always forget to use arcfilter before pushing the changes.

[mehdi_amini]

Limit to python3 and use the new "awk" script

[mehdi_amini]

llvm/utils/git/arcfilter.sh

6

Done, but still ping @MaskRay in case you missed my question? (I'm curious)

[MaskRay]

llvm/utils/git/arcfilter.sh

6

Author dates (git log default) seem to be random. They are oftentimes misleading. (For example, it is difficult to tell whether a commit was really made yesterday) I changed my git log config to display committer dates by default, but many don't. Resetting my commits will make others easier when reading my commits.

[mehdi_amini]

llvm/utils/git/arcfilter.sh

6

OK, does it LGTY now?

[MaskRay]

llvm/utils/git/pre-push.py

92

Are print_raw_stderr and ignore_errors and probably other arguments really used?

117

text is always True. The other code path can be dropped.

138

shutil.which

147

return reversed(...)

202

typo: find

214

No need to wrap lines.

227

spaces around =

[mehdi_amini]

llvm/utils/git/pre-push.py

92

I forked the whole file from the former git-llvm script we had in tree :)

117

this function as-is is reusable: I could strip it down the bare minimum needed in this script, but since it already existed before...

138

Does it handle the .exe suffix addition here?

[jdoerfert]

I haven't reviewed the code but I like this a lot, thx! 👍

[mehdi_amini]

Address some comments on the python script

[mehdi_amini]

(mark some comments as done)

[mehdi_amini]

llvm/utils/git/pre-push.py

138

Seems like it would work, but requires 3.3, not sure if we should require this?

[ldionne]

This is nice! I'm pretty tired of seeing Arcanist boilerplate in commits. Let's do this!

[mehdi_amini]

@MaskRay any major left or can we go one with this?

[MaskRay]

llvm/utils/git/pre-push.py

41

Not used in Python 3

50

space around =

77

space after comma

123

Quotes are very inconsistent in this file. Please stick with '

138

Python 3.4 was released 6 years ago. I think we don't need to care about 3.3 on Windows.

185

Single quotes and add spaces after commas

225

No trailing empty line

[mehdi_amini]

@MaskRay anything more than cosmetic left?

[mehdi_amini]

llvm/utils/git/pre-push.py

147

Actually I need to revert back to commits.reverse() here:

File ".git/hooks/pre-push", line 165, in handle_push
    if len(revs) > 1:
TypeError: object of type 'list_reverseiterator' has no len()

[mehdi_amini]

Mostly cosmetic

[MaskRay]

In D80978#2102557, @mehdi_amini wrote:

@MaskRay anything more than cosmetic left?

Style-wise I cannot find more issues now. Honestly I tried this script just now.

Without git pull --rebase origin master, my local master branch was stale. This script said:

% git push origin HEAD:master                                                                                                                   
`git rev-list 4171f80d5416eccbeebe8864410d576d7dc61eaa..bb67f1a4400a19318713375655999f733418a33b` returned 128    
fatal: Invalid revision range 4171f80d5416eccbeebe8864410d576d7dc61eaa..bb67f1a4400a19318713375655999f733418a33b

The diagnostic can probably be made more helpful.

After git pull --rebase origin master:

% git push origin HEAD:master                                                                                                                     
Pushing to "refs/heads/master" on remote "[e-mail]:llvm/llvm-project.git"
 - 8ffb2097cc1 [ELF] Refine LMA offset propagation rule in D76995          
Enumerating objects: 22, done.                                             
Counting objects: 100% (22/22), done.                                      
Delta compression using up to 72 threads                             
Compressing objects: 100% (11/11), done.                             
Writing objects: 100% (12/12), 2.04 KiB | 2.04 MiB/s, done.          
Total 12 (delta 9), reused 0 (delta 0), pack-reused 0                
remote: Resolving deltas: 100% (9/9), completed with 9 local objects.
To github.com:llvm/llvm-project.git
   4171f80d541..8ffb2097cc1  HEAD -> master

The first two lines are added by this script. They look good to me.

[mehdi_amini]

Detect when the remote revision isn't available locally

[mehdi_amini]

In D80978#2103899, @MaskRay wrote:

Without git pull --rebase origin master, my local master branch was stale. This script said:

% git push origin HEAD:master                                                                                                                   
`git rev-list 4171f80d5416eccbeebe8864410d576d7dc61eaa..bb67f1a4400a19318713375655999f733418a33b` returned 128    
fatal: Invalid revision range 4171f80d5416eccbeebe8864410d576d7dc61eaa..bb67f1a4400a19318713375655999f733418a33b

Thanks for catching this! I think the script should just ignore this case and let git fails. I added the check.

llvm/utils/git/pre-push.py

138

Note: this isn't about windows, using this puts the requirement everywhere

[MaskRay]

LGTM.

[mehdi_amini]

Thanks for the reviews @MaskRay !

llvm/docs/GettingStarted.rst

.. code-block:: console
   % git push origin HEAD:master
 
 LLVM currently has a linear-history policy, which means that merge commits are
 not allowed. The `llvm-project` repo on github is configured to reject pushes
 that include merges, so the `git rebase` step above is required.
 
 Please ask for help if you're having trouble with your particular git workflow.
 
+Git pre-push hook
+^^^^^^^^^^^^^^^^^
+
+We include an optional pre-push hook that run some sanity checks on the revisions
+you are about to push and ask confirmation if you push multiple commits at once.
+You can set it up (on Unix systems) by running from the repository root:
+
+.. code-block:: console
+
+  % ln -sf ../../llvm/utils/git/pre-push.py .git/hooks/pre-push
+
 Bisecting commits
 ^^^^^^^^^^^^^^^^^
 
 See `Bisecting LLVM code <GitBisecting.html>`_ for how to use ``git bisect``
 on LLVM.
 
 Reverting a change
 ^^^^^^^^^^^^^^^^^^

llvm/docs/Phabricator.rst

 guide <commit_from_git>`.
 
 Note that if you commit the change without using Arcanist and forget to add the
 ``Differential Revision`` line to your commit message then it is recommended
 that you close the review manually. In the web UI, under "Leap Into Action" put
 the git revision number in the Comment, set the Action to "Close Revision" and
 click Submit.  Note the review must have been Accepted first.
 
+Arcanist also adds extra tags that are mostly noise in the commit message, for
+this reason avoid using `arc land` and push commits to master directly with git
+after removing tags other than "Reviewed by" and "Differential Revision".
+You can run `llvm/utils/git/arcfilter.sh` to clean the commit message of the
+current "HEAD" commit automatically. You can also setup a git hook to catch this
+for you (see `Getting Started <GettingStarted.html#git-pre-push-hook>`).
+
 
 Committing someone's change from Phabricator
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 On a clean Git repository on an up to date ``master`` branch run the
 following (where ``<Revision>`` is the Phabricator review number):
 
 ::

 
 ::
 
   git pull --rebase origin master
   git show # Ensure the patch looks correct.
   ninja check-$whatever # Rerun the appropriate tests if needed.
   git push
 
-Or
-
-::
-
-  arc land D<Revision>
-
 
 Abandoning a change
 -------------------
 
 If you decide you should not commit the patch, you should explicitly abandon
 the review so that reviewers don't think it is still open. In the web UI,
 scroll to the bottom of the page where normally you would enter an overall
 comment. In the drop-down Action list, which defaults to "Comment," you should

llvm/utils/git/arcfilter.sh

+#!/bin/sh
+
+# Remove the phabricator tags from the commit at HEAD
+git log -1 --pretty=%B | \
+  sed  's/^Summary://'  | \
+  awk '/Reviewers: /{p=1; sub(/Reviewers: .*Differential Revision: /, "")}; /Differential Revision: /{p=0;}; !p' | \
+  git commit --amend -F - ; }
+

llvm/utils/git/pre-push.py

+#!/usr/bin/env python
+#
+# ======- git-llvm - LLVM Git Help Integration ---------*- python -*--========#
+#
+# Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+# See https://llvm.org/LICENSE.txt for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+#
+# ==------------------------------------------------------------------------==#
+
+"""
+git-llvm integration
+====================
+
+This file provides integration for git.
+
+The git llvm push sub-command can be used to push changes to GitHub.  It is
+designed to be a thin wrapper around git, and its main purpose is to
+detect and prevent merge commits from being pushed to the main repository.
+
+Usage:
+
+git-llvm push <upstream-branch>
+
+This will push changes from the current HEAD to the branch <upstream-branch>.
+
+"""
+
+from __future__ import print_function
+import argparse
+import collections
+import os
+import re
+import shutil
+import subprocess
+import sys
+import time
+import getpass
+assert sys.version_info >= (2, 7)
+
+try:
+    dict.iteritems
+except AttributeError:
+    # Python 3
+    def iteritems(d):
+        return iter(d.items())
+else:
+    # Python 2
+    def iteritems(d):
+        return d.iteritems()
+
+try:
+    # Python 3
+    from shlex import quote
+except ImportError:
+    # Python 2
+    from pipes import quote
+
+VERBOSE = False
+QUIET = False
+dev_null_fd = None
+z40="0000000000000000000000000000000000000000"
+
+
+def eprint(*args, **kwargs):
+    print(*args, file=sys.stderr, **kwargs)
+
+
+def log(*args, **kwargs):
+    if QUIET:
+        return
+    print(*args, **kwargs)
+
+
+def log_verbose(*args, **kwargs):
+    if not VERBOSE:
+        return
+    print(*args, **kwargs)
+
+
+def die(msg):
+    eprint(msg)
+    sys.exit(1)
+
+
+def ask_confirm(prompt):
+    # Python 2/3 compatibility
+    try:
+        read_input = raw_input
+    except NameError:
+        read_input = input
+
+    while True:
+        query = read_input('%s (y/N): ' % (prompt))
+        if query.lower() not in ['y','n', '']:
+           print('Expect y or n!')
+           continue
+        return query.lower() == 'y'
+
+
+def get_dev_null():
+    """Lazily create a /dev/null fd for use in shell()"""
+    global dev_null_fd
+    if dev_null_fd is None:
+        dev_null_fd = open(os.devnull, 'w')
+    return dev_null_fd
+
+
+def shell(cmd, strip=True, cwd=None, stdin=None, die_on_failure=True,
+          ignore_errors=False, text=True, print_raw_stderr=False):
+    # Escape args when logging for easy repro.
+    quoted_cmd = [quote(arg) for arg in cmd]
+    log_verbose('Running in %s: %s' % (cwd, ' '.join(quoted_cmd)))
+
+    err_pipe = subprocess.PIPE
+    if ignore_errors:
+        # Silence errors if requested.
+        err_pipe = get_dev_null()
+
+    start = time.time()
+    p = subprocess.Popen(cmd, cwd=cwd, stdout=subprocess.PIPE, stderr=err_pipe,
+                         stdin=subprocess.PIPE,
+                         universal_newlines=text)
+    stdout, stderr = p.communicate(input=stdin)
+    elapsed = time.time() - start
+
+    log_verbose('Command took %0.1fs' % elapsed)
+
+    if p.returncode == 0 or ignore_errors:
+        if stderr and not ignore_errors:
+            if not print_raw_stderr:
+                eprint('`%s` printed to stderr:' % ' '.join(quoted_cmd))
+            eprint(stderr.rstrip())
+        if strip:
+            if text:
+                stdout = stdout.rstrip('\r\n')
+            else:
+                stdout = stdout.rstrip(b'\r\n')
+        if VERBOSE:
+            for l in stdout.splitlines():
+                log_verbose("STDOUT: %s" % l)
+        return stdout
+    err_msg = '`%s` returned %s' % (' '.join(quoted_cmd), p.returncode)
+    eprint(err_msg)
+    if stderr:
+        eprint(stderr.rstrip())
+    if die_on_failure:
+        sys.exit(2)
+    raise RuntimeError(err_msg)
+
+
+def git(*cmd, **kwargs):
+    return shell(['git'] + list(cmd), **kwargs)
+
+
+def program_exists(cmd):
+    if sys.platform == 'win32' and not cmd.endswith('.exe'):
+        cmd += '.exe'
+    for path in os.environ["PATH"].split(os.pathsep):
+        if os.access(os.path.join(path, cmd), os.X_OK):
+            return True
+    return False
+
+def get_revs_to_push(range):
+    commits = git('rev-list', range).splitlines()
+    # Reverse the order so we print the oldest commit first
+    commits.reverse()
+    return commits
+
+def handle_push(args, local_ref, local_sha, remote_ref, remote_sha):
+    '''Check a single push request (which can include multiple revisions)'''
+    # Handle request to delete
+    if local_sha == z40:
+        if not ask_confirm("Are you sure you want to delete '%s' on remote '%s'?" % (remote_ref, args.url)):
+            die("Aborting")
+        return
+
+    # Push a new branch
+    if remote_sha == z40:
+      if not ask_confirm("Are you sure you want to push a new branch/tag '%s' on remote '%s'?" % (remote_ref, args.url)):
+        die("Aborting")
+      range=local_sha
+      return
+    else:
+      # Update to existing branch, examine new commits
+      range="%s..%s" % (remote_sha, local_sha)
+
+    revs = get_revs_to_push(range)
+    if not revs:
+        # This can happen if someone is force pushing an older revision to a branch
+        return
+
+    # Print the revision about to be pushed commits
+    print("Pushing to '%s' on remote '%s'" % (remote_ref, args.url))
+    for sha in revs:
+      print(" - " + git("show", "--oneline", "--quiet", sha))
+
+    if len(revs) > 1:
+      if not ask_confirm("Are you sure you want to push %d commits'?" % len(revs)):
+          die("Aborting")
+
+
+    for sha in revs:
+      msg = git("log","--format=%B","-n1", sha)
+      if "Differential Revision" not in msg:
+          continue
+      for line in msg.splitlines():
+          for tag in ["Summary", "Reviewers", "Subscribers", "Tags"]:
+            if line.startswith(tag + ":"):
+              eprint("Please remove arcanist tags from the commit message (found '%s' tag in %s)" % (tag, sha[:12]))
+              if len(revs) == 1:
+                  eprint("Try running: llvm/utils/git/arcfilter.sh")
+              die("Aborting (force push by adding '--no-verify')")
+
+    return
+
+
+if __name__ == '__main__':
+    if not program_exists('git'):
+        die('error: cannot fine git command')
+
+    argv = sys.argv[1:]
+    p = argparse.ArgumentParser(
+        prog='pre-push', formatter_class=argparse.RawDescriptionHelpFormatter,
+        description=__doc__)
+    verbosity_group = p.add_mutually_exclusive_group()
+    verbosity_group.add_argument('-q', '--quiet', action='store_true',
+                                 help='print less information')
+    verbosity_group.add_argument('-v', '--verbose', action='store_true',
+                                 help='print more information')
+
+    p.add_argument(
+        'remote',
+        type=str,
+        help="Name of the remote")
+    p.add_argument(
+        'url',
+        type=str,
+        help="URL for the remote")
+
+    args = p.parse_args(argv)
+    VERBOSE = args.verbose
+    QUIET = args.quiet
+
+    lines=sys.stdin.readlines()
+    sys.stdin = open("/dev/tty", "r")
+    for line in lines:
+      local_ref, local_sha, remote_ref, remote_sha = line.split()
+      handle_push(args, local_ref, local_sha, remote_ref, remote_sha)
+