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.

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

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

Closed
wants to merge 4 commits into from

Conversation

VallariAg
Copy link
Member

@VallariAg VallariAg commented Feb 2, 2022

Resolves https://tracker.ceph.com/issues/51537

For files changes in src/abc/xyz/file.ext, abcd/file2.txt, and file3.txt, this would create a template with the file path (and remove the redundant "/src" part):

# Please enter the commit message for your changes.
# abc/xyz: <commit_message>
# abcd: <commit_message>
# file3.txt: <commit_message>

The developer can simply uncomment the most relevant lines and replace <commit_message> with their message text.

To use git hooks, configure git to use githooks directory instead of .git/hooks/ (reason: maintaining git hooks in repository) by running:

git config core.hooksPath githooks

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox

@ideepika
Copy link
Member

ideepika commented Feb 7, 2022

@VallariAg this is awesome, thanks for the contribution!

I was just curious why didn't you use .git/hooks directory itself?

I tried your PR locally, we are not retaining default github comments, can we do a append to commit message defaults that github has, instead of complete template replacing?
Another thing, I'd like to see if possible would be Signed-off-by auto append, for example please read about git commit signing.
We'd need to amend the commit in this PR as well with a git commit sign.

@VallariAg
Copy link
Member Author

Hey @ideepika, thank you!

I was just curious why didn't you use .git/hooks directory itself?

Scripts inside.git/hooks/ are not version controlled so they cannot be commited to the repo. So, I came across this solution, to store these scripts in a directory which can be tracked by git.
I think, we would need to add command git config core.hooksPath githooks to the installation script for these hooks to be used by everyone.

can we do a append to commit message defaults that github has, instead of complete template replacing?
I'd like to see if possible would be Signed-off-by auto append,

Awesome, I'll make these changes!

We'd need to amend the commit in this PR as well with a git commit sign.

My bad, I'll edit my commit message with the changes you mentioned above.

Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
@VallariAg
Copy link
Member Author

Added the "signed-off-by" and the default commit message text.
For changes on a file /test.txt, it creates the following commit message:

# Possible commit messages: 
# test.txt: <commit_message>

# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch test
# Changes to be committed:
#       new file:   test.txt
#
# Changes not staged for commit:
#       modified:   githooks/prepare-commit-msg
#       modified:   install-deps.sh
#
# Untracked files:
#       # Possible commit messages: 
#       gpg-key.asc
#       gpg-key.pub
#

Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>

@ideepika
Copy link
Member

@VallariAg thanks for the update, added minor changes needs, looks good overall!

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!

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

@ideepika ideepika Feb 15, 2022

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.

Copy link
Member

@epuertat epuertat left a comment

Choose a reason for hiding this comment

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

Thanks a lot @VallariAg for your magnificent contribution here! 🥳 I left a few suggestions over there.

@@ -0,0 +1,43 @@
#!/usr/bin/sh
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#!/usr/bin/sh
#!/usr/bin/env sh

Or bash

Comment on lines 12 to 14
COMMIT_MSG_FILE=$1
COMMIT_SOURCE=$2
SHA1=$3
Copy link
Member

Choose a reason for hiding this comment

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

Have you tested this with a commit template? I'm asking because many people use that and I tried and it bypasses this. It might be interesting to check if there's a way to make both things work together.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't tried using commit templates, I'll try and test that!

# src/<project>/file.txt - "<project>:"
# file.txt - "file.txt:"
# <project>/file.txt - "<project>:"
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.

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.

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).

# the commit is aborted.
#
# 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

@epuertat
Copy link
Member

@VallariAg regarding the file location, if you assume that users might have their own hooks in .git/hooks then the idea of changing the config is less feasible. So what I usually do here is creating symlinks from the hooks to files in the repo, and to make things more user-friendly you can provide a script (or document the command) for automating that.

@VallariAg
Copy link
Member Author

@VallariAg regarding the file location, if you assume that users might have their own hooks in .git/hooks then the idea of changing the config is less feasible. So what I usually do here is creating symlinks from the hooks to files in the repo, and to make things more user-friendly you can provide a script (or document the command) for automating that.

Oh, yes, that makes sense! symlinks it is, then.

@djgalloway djgalloway changed the base branch from master to main May 25, 2022 20:03
To use these scripts, run:
- chmod +x githooks/prepare-commit-msg
- ln -s githooks/* .git/hooks/

Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Aug 25, 2022
@VallariAg VallariAg marked this pull request as draft August 25, 2022 23:11
@VallariAg
Copy link
Member Author

I've pushed some of the work I did on this, using python scripts to achieve the suggested results.

Though while working on this, I realized I need to re-approach a part of how commit-msgs are editing messages because of unexpected results in few cases (example: using "--amend" doesn't show the commit message template and use of these hooks with "-m" needs more customization).
I'll get back to this soon! This would really be a convenient feature to have. Sorry for the lack of activity on this one!
When it's ready for review, I'll change it back to open PR from draft PR.

@github-actions github-actions bot removed the stale label Aug 26, 2022
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Jan 27, 2023
@github-actions
Copy link

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

@github-actions github-actions bot closed this Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants