Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

githooks: create commit message template with prepare-commit-msg #44872

Closed
wants to merge 4 commits into from
Closed

githooks: create commit message template with prepare-commit-msg #44872

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab7c7df
githooks: Create commit message template with prepare-commit-msg
VallariAg Feb 2, 2022
f951d75
githooks: add SOB and default commit message text
VallariAg Feb 9, 2022
028c3f9
githooks: Add prepare-commit-msg hook in python script
VallariAg Aug 4, 2022
83b0c48
githook: Add commit-msg hook
VallariAg Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 15 additions & 13 deletions githooks/prepare-commit-msg
View file
Open in desktop
Expand Up @@ -9,33 +9,35 @@
#
# To enable this hook, rename this file to "prepare-commit-msg".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always better to write safer shell scripts:

Suggested change
set -euxo pipefail

# This hook includes three examples. The first one removes the
# "# Please enter the commit message..." help message.
#
# The second includes the output of "git diff --name-status -r"
# into the message, just before the "git status" output. It is
# commented because it doesn't cope with --amend or with squashed
# commits.
#
# The third example adds a Signed-off-by line to the message, that can
# still be edited. This is rarely a good idea.

COMMIT_MSG_FILE=$1
COMMIT_SOURCE=$2
SHA1=$3


# Removes the "# Please enter the commit message..." help message.
# /usr/bin/perl -i.bak -ne 'print unless(m/^. Please enter the commit message/..m/^#$/)' "$COMMIT_MSG_FILE"

if [[ -z "$COMMIT_SOURCE" ]]
then
echo "# Please enter the commit message for your changes. " > "$COMMIT_MSG_FILE"
COMMIT_MSG_HEADERS="# Possible commit messages: "
for FILE in $(git diff --cached --name-only -r)
do
# According to files changed the commit message would start "<project_name>: <commit_message>"
# src/<project>/file.txt - "<project>:"
# file.txt - "file.txt:"
# <project>/file.txt - "<project>:"
echo "# $FILE: <commit_message>" | sed -E 's|src/+||g' | sed -r 's/(.*)(\/[a-z0-9.-]+)/\1 /' >> "$COMMIT_MSG_FILE"
COMMIT_MSG_HEADERS+="\n$(echo "# $FILE: <commit_message>" | sed -E 's|src/+||g' | sed -r 's/(.*)(\/[a-z0-9.-]+)/\1 /')"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a couple of files where we map paths to Github teams, and to Github labels.

I'm not advocating for a third file mapping paths to components, but... yeah, on a second thought, it makes all the sense, right? These files are not changed that often to become tedious to keep them in sync.

While sed supports for script files, at this point in complexity I'd consider rewriting this in Python. With a CSV reader and a file pattern matching library, you could easily read the mappings from a file:

doc/*                      doc
src/mon/*                  mon
...
src/pybind/mgr/dashboard/* mgr/dashboard

BONUS 1: Additionally it'd be great not to show the same component multiple times:

# Possible commit messages:
# pybind/mgr/dashboard : <commit_message>
# pybind/mgr/dashboard : <commit_message>
# mon:  <commit_message>

If you implement this in Python it's quite easy to perform the deduplication and only show 1 line per component.

BONUS 2: If you count the number of occurrences per component, you can also suggest first the component with most files changed. Let's say a commit changed "src/mon/file1, src/cephfs/file2 and src/cephfs/file3", so we display first the component with more files modified (cephfs):

# Possible commit messages:
# cephfs : <commit_message>
# mon:  <commit_message>

Python 3.6 and above brings in a new class collections.Counter exactly for this purpose.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a file for mapping paths to component sounds like a great idea! Yeah, keeping them in sync should not be a problem.
Sounds awesome, doing this in python makes sense.

done
sed -i "1s;^;$COMMIT_MSG_HEADERS\n;" $COMMIT_MSG_FILE
Copy link
Member

@ideepika ideepika Feb 15, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have the commit message headers after signed-off by, a developer would be able to then identify more clearly if the commit is signed or not, and will not have to check after text the git default template.

fi


# Adds a Signed-off-by line to the message, that can still be edited (This is rarely a good idea).
SOB=$(git var GIT_COMMITTER_IDENT | sed -n 's/^\(.*>\).*$/Signed-off-by: \1/p')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

users most times use git commit -s to add signed off by line, can add a check for if signed-off-by is specified/present already in the commit message file; then do not add new.

Another thing I noticed was, doing a git amend adds a duplicate signed-off by as well; the above approach should also resolve that issue as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @ideepika, most users alreay include the -s or alias the commit -s command.

However, what we can do here is, apart from this hook, create a complementary pre-commit hook (if you write both in Python you can share most of the code from a third file) that basically detects:

  • the commit message follows the format: component: subject (and component has to match a list of predefined components obtained from the path-component mapping file)
  • check the commit title length (<72 chars).
  • Basically everything else required here.
  • Fixes: (not mandatory)
  • Signed-off-by:.

There are tools that already do this, but they are mostly for semantic commits, a convention we are not adhered to (it's configurable though).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, adding a complimentary hook to check the commit message is an awesome idea!
I think we would be going for the commit-msg hook, as they run in order: pre-commit -> prepare-commit-msg -> commit-msg.

Both hooks in python, got it!

printf "\n$SOB" >> "$COMMIT_MSG_FILE"

# Adds a newline at the start of the message
# if test -z "$COMMIT_SOURCE"
# then
# /usr/bin/perl -i.bak -pe 'print "\n" if !$first_line++' "$COMMIT_MSG_FILE"
# fi