Skip to content

fix(parser): require all commit parser fields to match - #1616

Open
sisp wants to merge 1 commit into
orhun:mainfrom
sisp:fix/require-all-commit-parser-fields-to-match
Open

fix(parser): require all commit parser fields to match#1616
sisp wants to merge 1 commit into
orhun:mainfrom
sisp:fix/require-all-commit-parser-fields-to-match

Conversation

@sisp

@sisp sisp commented Aug 24, 2026

Copy link
Copy Markdown

Description

Previously, a commit_parsers entry with multiple fields set (e.g. message and footer) matched a commit if any one field matched, instead of requiring all of them to match. This caused commits to be grouped incorrectly when parsers combined several conditions to narrow down a match.

Commit parsers with multiple fields now only match when every specified field matches.

The fix changes Commit::parse, so each specified matcher field (message, body, footer, field/pattern) is checked immediately and aborts the parser via continue 'parsers if it doesn't match, rather than collecting all fields into a list and matching if any one of them matched. Only sha retains its original independent fast path, matching unconditionally regardless of other fields set on the same parser; that's because a commit SHA is a unique identifier.

Motivation and Context

Fixes #1612.

How Has This Been Tested?

I've added a few test cases and run the test suite.

Screenshots / Logs (if applicable)

n/a

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly (if applicable).
  • I have formatted the code with rustfmt.
    • cargo +nightly fmt --all
  • I checked the lints with clippy.
    • cargo clippy --tests --verbose -- -D warnings
  • I have added tests to cover my changes.
  • All new and existing tests passed.
    • cargo test

@sisp

sisp commented Aug 24, 2026

Copy link
Copy Markdown
Author

I think the two CI job failures are inherited from main.

@orhun orhun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hey hey!
Thanks for the PR!

I found two issues:

  • A configured footer should fail when the commit has no conventional footer data. Currently, when self.conv is None, this entire condition is skipped, so another matching condition such as message can still make the parser match.
  • { sha = "abc123...", message = "^feat", skip = true } -> will no longer match commit abc123... if its message starts with "fix", because the message check executes continue 'parsers before reaching the SHA comparison.

Can you fix these issues and add tests for them as well?

Previously, a `commit_parsers` entry with multiple fields set (e.g.
`message` and `footer`) matched a commit if any one field matched,
instead of requiring all of them to match. This caused commits to be
grouped incorrectly when parsers combined several conditions to narrow
down a match.

Commit parsers with multiple fields now only match when every specified
field matches.
@sisp
sisp force-pushed the fix/require-all-commit-parser-fields-to-match branch from 4d70838 to da3f30d Compare August 31, 2026 08:52
@sisp

sisp commented Aug 31, 2026

Copy link
Copy Markdown
Author

Thanks for your diligent review and feedback! 🙇 I've updated the PR, hopefully addressed your remarks.

Regarding the SHA matching behavior: I've relaxed it to be a regular matcher that can be combined with other matchers like message instead of treating it special. Does this align with your expected behavior?

@sisp
sisp requested a review from orhun August 31, 2026 09:00
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.

Commit parser field regexes should have conjunctive relationship

2 participants