[pulseaudio-discuss] scripts: Add a pre-receive hook to catch invalid merge and WIP commits

Submitted by Arun Raghavan on April 7, 2017, 8:39 a.m.

Details

Message ID 20170407083952.32390-1-arun@arunraghavan.net
State New
Headers show
Series "scripts: Add a pre-receive hook to catch invalid merge and WIP commits" ( rev: 2 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Arun Raghavan April 7, 2017, 8:39 a.m.
This should make sure we avoid merge commits from branches outside of PA
(we don't really have any, so this should avoid all merge commits). This
also catches "WIP" and such in the title to prevent accidental pushing
of those.
---
 scripts/pre-receive.hook | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100755 scripts/pre-receive.hook

Patch hide | download patch | download mbox

diff --git a/scripts/pre-receive.hook b/scripts/pre-receive.hook
new file mode 100755
index 0000000..7240c27
--- /dev/null
+++ b/scripts/pre-receive.hook
@@ -0,0 +1,89 @@ 
+#!/usr/bin/env python2
+import sys
+import os
+import subprocess
+
+# Based on the same file in: https://cgit.freedesktop.org/gstreamer/common
+# Manually deployed to /srv/git.freedesktop.org/git/pulseaudio/pulseaudio.git/hooks
+
+# checks whether the push contains merge commits and if so, makes sure that
+# the merge is only between branches present on the repository.
+def contains_valid_merge(old, new, ref):
+    sub = subprocess.Popen(["git", "rev-list", "--parents", "%s..%s" % (old, new, )],
+                           stdout=subprocess.PIPE)
+    stdout, stderr = sub.communicate()
+
+    res = True
+
+    print "=> Checking for merge commits"
+
+    for line in stdout.split('\n'):
+        if res == False:
+            break
+        splits = line.strip().split()
+        if len(splits) > 2:
+            # the commit has more than one parent => it's a merge
+            commit = splits[0]
+            for parent in splits[1:]:
+                if not commit_in_valid_branch(parent):
+                    print
+                    print "    /!\\ Commit %s" % commit
+                    print "    /!\\ is a merge commit from/to a branch not"
+                    print "    /!\\ present on this repository"
+                    print
+                    print "    Make sure you are not attempting to push a merge"
+                    print "    from a 3rd party repository"
+                    print
+                    print "    Make sure you have properly rebased your commits against"
+                    print "    the current official branches"
+                    print
+                    res = False
+                    break
+
+    if res:
+            print "   [OK]"
+            print
+    return res
+
+# checks whether the first line of any commit message contains a marker
+# that indicates that it's work-in-progress that shouldn't be pushed
+def contains_wip_commits(old, new):
+    # Get the commit message header
+    sub = subprocess.Popen(["git", "log", "--format=%s", "%s..%s" % (old, new, )], stdout=subprocess.PIPE)
+    stdout, stderr = sub.communicate()
+    if stdout != "":
+        for line in stdout.strip().rsplit('\n',1):
+            if line.startswith("!"):
+                return True
+            if line.startswith("WIP:") or line.startswith("wip:"):
+                return True
+    return False
+
+pushvalid = True
+error_badcommit = False
+
+for line in sys.stdin.readlines():
+    # ref is the ref to be modified
+    # old is the old commit it was pointing to
+    # new is the new commit it was pointing to
+    old, new, ref = line.split(' ', 2)
+
+    pushvalid = contains_valid_merge(old, new, ref)
+    if pushvalid == False:
+        break
+    if contains_wip_commits(old, new):
+        error_badcommit = True
+        pushvalid = False
+        break
+
+if pushvalid:
+    print "   Incoming packet valid, proceeding to actual commit"
+    print
+    sys.exit(0)
+else:
+    if error_badcommit:
+        print "   Attempting to push commits with commit messages that start"
+        print "   with '!' or 'WIP:'. Such commit messages are reserved for"
+        print "   private branches and work in progress to prevent the commits"
+        print "   from accidentally being pushed to the main repository."
+    sys.exit(-1)

Comments

On Fri, 2017-04-07 at 14:09 +0530, Arun Raghavan wrote:
> This should make sure we avoid merge commits from branches outside of PA
> (we don't really have any, so this should avoid all merge commits). This
> also catches "WIP" and such in the title to prevent accidental pushing
> of those.
> ---
>  scripts/pre-receive.hook | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100755 scripts/pre-receive.hook

I tested the script. The "wip" detection seems to work as advertised,
but I found a couple of other issues, see below.

> diff --git a/scripts/pre-receive.hook b/scripts/pre-receive.hook
> new file mode 100755
> index 0000000..7240c27
> --- /dev/null
> +++ b/scripts/pre-receive.hook
> @@ -0,0 +1,89 @@
> +#!/usr/bin/env python2
> +import sys
> +import os
> +import subprocess
> +
> +# Based on the same file in: https://cgit.freedesktop.org/gstreamer/common
> +# Manually deployed to /srv/git.freedesktop.org/git/pulseaudio/pulseaudio.git/hooks

This should mention that when deploying, the .hook suffix needs to be
dropped. Or maybe the suffix shouldn't be in the file name in the first
place?

> +
> +# checks whether the push contains merge commits and if so, makes sure that
> +# the merge is only between branches present on the repository.
> +def contains_valid_merge(old, new, ref):
> +    sub = subprocess.Popen(["git", "rev-list", "--parents", "%s..%s" % (old, new, )],
> +                           stdout=subprocess.PIPE)
> +    stdout, stderr = sub.communicate()
> +
> +    res = True
> +
> +    print "=> Checking for merge commits"
> +
> +    for line in stdout.split('\n'):
> +        if res == False:
> +            break
> +        splits = line.strip().split()
> +        if len(splits) > 2:
> +            # the commit has more than one parent => it's a merge
> +            commit = splits[0]
> +            for parent in splits[1:]:
> +                if not commit_in_valid_branch(parent):

remote: => Checking for merge commits
remote: Traceback (most recent call last):
remote:   File "hooks/pre-receive", line 71, in <module>
remote:     pushvalid = contains_valid_merge(old, new, ref)
remote:   File "hooks/pre-receive", line 28, in contains_valid_merge
remote:     if not commit_in_valid_branch(parent):
remote: NameError: global name 'commit_in_valid_branch' is not defined