Merge ~sbeattie/ubuntu-cve-tracker/+git/ubuntu-cve-tracker:git-commit-hooks-pre-merge_hook into ubuntu-cve-tracker:master

[Steve Beattie]

git-hooks: use pre-merge-commit over prepare-commit-msg

Replace/remove the prepare-commit-msg with a pre-merge-commit hook that
invokes the pre-commit infrastructure we already have.

When we originally started using git hooks to perform check-syntax
against UCT, the prepare-commit-msg hook was added (in 2019) because
git's pre-commit hook does not get performed on git merges, and there
were times when we would want to merge branches from people who do
not have enough of a setup for check-syntax to work.

However, git has had a pre-merge-commit hook since git 2.24 [1], and
present since at least Ubuntu 20.04 LTS, so we should make use of that.
Additionally, the prepare-commit-msg hook attempted to duplicate the
pre-commit hook but was never kept in sync, so choose here to use
the default upstream pre-merge-commit hook, which just invokes the
pre-commit hook.

[1] https://github.com/git/git/blob/da72936f544fec5a335e66432610e4cef4430991/Documentation/RelNotes/2.24.0.txt#L38

Thinking about git commit hooks from Amir's merge request reminded me that we were using the prepare-commit-msg hook to work around git's failure to use the pre-commit hook for merges. Git 2.24 added support for a pre-merge-commit hook, so this merge proposal converts UCT to use that, and set it up via 'make dev-setup'.

One thing this change does NOT do, however, is remove an existing prepare-commit-msg hook if it exists (which it should, because make dev-setup would install it into place).

[Alex Murray (alexmurray)]

LGTM except I wonder if we should make this get rid of the old prepare-commit-msg hook as well to keep things cleaner.

review: Approve

[Eduardo Barretto (ebarretto)]

hey Steve, is there anything else for this PR?

review: Needs Information

[Mark Esler (eslerm)]

LGTM. Just commenting.

Not sure if this is related: currently if you recieve a warning and fix the issue, a subsequent git-commit will complete even without git-add'ing the new changes first. As in, git-hooks inspect the present state of a file, not the form they are staged as.

Unmerged commits

2963895... by Steve Beattie on 2023-12-12

git-hooks: use pre-merge-commit over prepare-commit-msg

Replace/remove the prepare-commit-msg with a pre-merge-commit hook that
invokes the pre-commit infrastructure we already have.

When we originally started using git hooks to perform check-syntax
against UCT, the prepare-commit-msg hook was added (in 2019) because
git's pre-commit hook does not get performed on git merges, and there
were times when we would want to merge branches from people who do
not have enough of a setup for check-syntax to work.

However, git has had a pre-merge-commit hook since git 2.24 [1], and
present since at least Ubuntu 20.04 LTS, so we should make use of that.
Additionally, the prepare-commit-msg hook attempted to duplicate the
pre-commit hook but was never kept in sync, so choose here to use
the default upstream pre-merge-commit hook, which just invokes the
pre-commit hook.

[1] https://github.com/git/git/blob/da72936f544fec5a335e66432610e4cef4430991/Documentation/RelNotes/2.24.0.txt#L38

Signed-off-by: Steve Beattie <email address hidden>

	unit-tests:0 (build)
	check-cves:0 (build)

1 → 2 of 2 results

First • Previous • Next • Last

diff --git a/Makefile b/Makefile
index 2c0abb7..bfbafc3 100644
--- a/Makefile
+++ b/Makefile
@@ -61,8 +61,8 @@ dev_setup:
 		# install git prepare message hook; this is used to do a \
 		# check-syntax run for merge commits, which the regular git \
 		# commit hook mind-bogglingly does not get run on \
-		echo install -m 755 -b -S .backup scripts/git-hooks/prepare-commit-msg .git/hooks ; \
-		install -m 755 -b -S .backup scripts/git-hooks/prepare-commit-msg .git/hooks ; \
+		echo install -m 755 -b -S .backup scripts/git-hooks/pre-merge-commit .git/hooks ; \
+		install -m 755 -b -S .backup scripts/git-hooks/pre-merge-commit .git/hooks ; \
 		# install git commit hooks in UST repo if configured \
 		if [ -n "$${UST}" ] ; then \
 			echo install -m 755 -b -S .backup scripts/git-hooks/pre-commit-pyflakes3 "$${UST}/.git/hooks/pre-commit" ; \
diff --git a/scripts/git-hooks/pre-merge-commit b/scripts/git-hooks/pre-merge-commit
 new file mode 100755
index 0000000..399eab1
--- /dev/null
+++ b/scripts/git-hooks/pre-merge-commit
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# An example hook script to verify what is about to be committed.
+# Called by "git merge" with no arguments.  The hook should
+# exit with non-zero status after issuing an appropriate message to
+# stderr if it wants to stop the merge commit.
+#
+# To enable this hook, rename this file to "pre-merge-commit".
+
+. git-sh-setup
+test -x "$GIT_DIR/hooks/pre-commit" &&
+        exec "$GIT_DIR/hooks/pre-commit"
+:
diff --git a/scripts/git-hooks/prepare-commit-msg b/scripts/git-hooks/prepare-commit-msg
 deleted file mode 100755
index 629035d..0000000
--- a/scripts/git-hooks/prepare-commit-msg
+++ /dev/null
@@ -1,51 +0,0 @@
-#!/bin/sh
-#
-# git prepare-commit-msg hook
-# Sadly, on git merge, the pre-commit hook is not invoked. So we use
-# this hook instead to check if a merge has borked cve syntax.
-# Unfortunately, if the hook fails, then you are left in an incomplete
-# merge state.
-#
-
-if ! [ "$1" = ".git/MERGE_MSG" ] ; then
-    # this was not triggered by a git merge, so skip check (we do it
-    # elsewhere for regular commits *and* it's handled more cleanly
-    # there.
-    exit 0
-fi
-
-if git rev-parse --verify HEAD >/dev/null 2>&1
-then
-    against=HEAD
-else
-    # Initial commit: diff against an empty tree object
-    against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
-fi
-
-# Redirect output to stderr.
-exec 1>&2
-
-cleanup() {
-    if [ -n "${modified_files}" ] && [ -f "${modified_files}" ]  ; then
-        rm "${modified_files}"
-    fi
-}
-
-set -e
-
-trap cleanup EXIT INT HUP
-
-if [ -z "${UCT_IGNORE_CHECK_SYNTAX}" ] ; then
-    modified_files="$(mktemp -t ucthook-XXXXXXXXXX)"
-    # check-syntax needs full path to files so prepend it via sed
-    dir=$(git rev-parse --show-toplevel)
-    git diff --cached --name-only --diff-filter=AM $against | sed "s|^|${dir}/|" > "${modified_files}"
-
-    if ! "${UCT}/scripts/check-syntax" --verbose --filelist "${modified_files}" ; then
-        read -p 'Found syntax errors. Enter "ignore" to continue or anything else to exit: ' answer < /dev/tty
-        if ! [ "${answer}" = "ignore" ] ; then
-            echo 'Aborting, use git merge --abort to undo the merge action'
-            exit 1
-        fi
-    fi
-fi