Skip to content

Conversation

@ikryukov
Copy link
Collaborator

What

Local Git Hooks - Developers can install hooks that validate commit messages before they're created
Shared Validation Script - Single source of truth for commit message rules
Easy Setup - Simple installation with .githooks/setup.sh
Complete Documentation - README files explaining usage and examples
Validation Rules Enforced

  • Title must be ≤ 50 characters (merge commits exempt)
  • Title must start with approved prefix (CI, TEST, BUILD, TL/CUDA, etc.)
  • Title must NOT end with a period

Why ?

  1. Catch Issues Before Push
    Developers get immediate feedback on commit message format locally, before pushing to remote. This prevents frustration of failed CI checks and having to rewrite commit history after the fact.
  2. Reduce CI Build Failures
    By validating commit messages locally, this prevents PRs from failing the codestyle workflow due to improperly formatted commit titles. This saves CI resources and reduces noise in PR checks.
  3. Single Source of Truth
    The shared validation script (.github/scripts/check-commit-msg.sh) is used by both local git hooks and GitHub Actions CI, ensuring 100% consistency between local validation and CI checks. No more "Commit title is too long: 54" scenarios.
  4. Improved Developer Experience
    The hook provides clear, actionable error messages with examples of valid commit formats. Developers learn the project's commit message standards through immediate feedback rather than reading documentation or learning from CI failures.
  5. Easy Maintenance
    When commit message standards need to be updated, changes only need to be made in one place (the shared script). Both local hooks and CI automatically use the updated validation logic, eliminating the risk of drift between environments.
    Bonus: The setup is completely optional and non-intrusive. Developers can choose to install the hooks with a single command (./.githooks/setup.sh), and the repository continues to work normally for those who don't install them, with CI still enforcing the standards.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Adds local git hooks to validate commit messages before creation, with a shared validation script intended to be used by both local hooks and CI.

Major Issues Found:

  • The shared validation script at .github/scripts/check-commit-msg.sh:50-51 has a bug that always prints "Commit title is too long" regardless of the actual validation error
  • The CI workflow at .github/workflows/codestyle.yaml:25-69 was not updated to use the new shared script - it still uses inline validation logic, which defeats the entire "single source of truth" purpose stated in the PR description

Positive Aspects:

  • Hook implementation correctly delegates to shared script
  • Setup script works properly and is easy to use
  • Documentation is comprehensive with clear examples
  • The architectural approach of using a shared script is sound (when properly implemented)

Confidence Score: 2/5

  • This PR has critical bugs and incomplete implementation that break its stated goals
  • The PR introduces two critical issues: (1) misleading error messages that will confuse developers, and (2) fails to deliver on its core promise of "single source of truth" by leaving the CI workflow with duplicated inline validation logic. The shared script was created but the CI wasn't migrated to use it.
  • .github/scripts/check-commit-msg.sh (fix error output bug) and .github/workflows/codestyle.yaml (migrate to shared script)

Important Files Changed

File Analysis

Filename Score Overview
.github/scripts/check-commit-msg.sh 2/5 Shared validation script with misleading error output bug - always prints "too long" message
.github/workflows/codestyle.yaml 2/5 CI workflow not updated to use shared validation script, breaking single source of truth goal
.githooks/commit-msg 5/5 Hook wrapper that correctly delegates to shared validation script

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant Git as Git Client
    participant Hook as .githooks/commit-msg
    participant Script as .github/scripts/check-commit-msg.sh
    participant CI as GitHub Actions CI
    
    Note over Dev,Script: Local Development Flow
    Dev->>Git: git commit -m "message"
    Git->>Hook: Execute commit-msg hook
    Hook->>Hook: Check if validation script exists
    Hook->>Script: Run check-commit-msg.sh --file
    Script->>Script: Parse commit message
    Script->>Script: Validate length (≤50 chars)
    Script->>Script: Validate prefix match
    Script->>Script: Check for trailing period
    alt Validation passes
        Script-->>Hook: Exit 0
        Hook-->>Git: Allow commit
        Git-->>Dev: Commit created
    else Validation fails
        Script-->>Hook: Exit 1 + error details
        Hook-->>Git: Reject commit
        Git-->>Dev: Display error + requirements
    end
    
    Note over Dev,CI: CI Workflow Flow
    Dev->>CI: Push PR to GitHub
    CI->>CI: Check commit title step (inline validation)
    Note right of CI: Currently NOT using<br/>shared script!
    CI->>CI: Iterate commits in PR
    alt All commits valid
        CI-->>Dev: CI check passes
    else Any commit invalid
        CI-->>Dev: CI check fails
    end
Loading

Additional Comments (1)

  1. .github/workflows/codestyle.yaml, line 25-69 (link)

    logic: CI workflow still uses inline validation instead of the new shared script .github/scripts/check-commit-msg.sh. This breaks the "single source of truth" promise from the PR description

    Replace the inline check_title function with calls to the shared script:

    - name: Check commit title
      run: |
        set -eE
        range="remotes/origin/$GITHUB_BASE_REF..HEAD"
        ok=1
        for sha1 in `git log $range --format="%h"`
        do
          title=`git log -1 --format="%s" $sha1`
          if ./.github/scripts/check-commit-msg.sh "$title"
          then
            echo "Good commit title: '$title'"
          else
            echo "Bad commit title: '$title'"
            ok=0
          fi
          echo "--------------------------------------------------"
        done
        if [ $ok -ne 1 ]
        then
          exit 1
        fi
    

5 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +50 to +51
if [ ${#errors[@]} -gt 0 ]; then
echo "Commit title is too long: ${#commit_title}"
Copy link

Choose a reason for hiding this comment

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

logic: always prints "Commit title is too long" regardless of actual error

Suggested change
if [ ${#errors[@]} -gt 0 ]; then
echo "Commit title is too long: ${#commit_title}"
if [ ${#errors[@]} -gt 0 ]; then
echo "Bad commit title: '$commit_title'"

@svcnbu-swx-hpcx
Copy link
Collaborator

Can one of the admins verify this patch?

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.

3 participants