Skip to content

Conversation

@dslemusp
Copy link

@dslemusp dslemusp commented Jul 3, 2025

Description

Currently each commit in the commit list is processed before splitting. This causes an issue when split_commit is set to true. Commits in the description are not being processed if the commit message contains a skipped type set in the commit_parsers. For instance if the output of a squash and merge start with a chore: and the chore: is set to skip in the commit_parsers the description won't be parsed even if it contains feat: or fix: or any other non skipped field.

// Squash and merge Commit Sample
chore(ci): Update ci runners

fix(ci): a fix --> This is never parsed
chore(ci): another chore --> This is never parsed

Motivation and Context

When working with squash and merge it is useful to have the message of the commit as the overview of the changes and the description as the details of the changes. However it should be possible to skip parsing the message as the description of the commit contains all the info, so trying to avoid repetitions.

How Has This Been Tested?

I have adjusted the current unit test to reflect the scenario that I am describing

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.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dslemusp dslemusp requested a review from orhun as a code owner July 3, 2025 21:25
@welcome
Copy link

welcome bot commented Jul 3, 2025

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

@codecov-commenter
Copy link

codecov-commenter commented Jul 3, 2025

Codecov Report

❌ Patch coverage is 82.89474% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.40%. Comparing base (9857d86) to head (d1c1eb3).

Files with missing lines Patch % Lines
git-cliff-core/src/changelog.rs 82.44% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1201      +/-   ##
==========================================
+ Coverage   43.65%   44.40%   +0.76%     
==========================================
  Files          22       22              
  Lines        1982     2025      +43     
==========================================
+ Hits          865      899      +34     
- Misses       1117     1126       +9     
Flag Coverage Δ
unit-tests 44.40% <82.90%> (+0.76%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@orhun
Copy link
Owner

orhun commented Jul 6, 2025

This was changed in #556 to fix another issue and was reported again in #1093 - can you maybe take a look and double check this?

@dslemusp
Copy link
Author

dslemusp commented Jul 7, 2025

Yeah actually it seems to me that the initial behaviour and order is what I would expect to have. As I said, right now the commits are treated as one during preprocessing even if the split_commit flag is enabled. This results on description being skipped because the commit message is ignored in the preprocessor.

@orhun
Copy link
Owner

orhun commented Jul 7, 2025

Thanks for checking. I'd lean towards making this configurable instead of changing the behavior again. We could maybe add a new processing_order option like follows:

[git]
processing_order = ["preprocess", "split", "proces", "postprocess", "etc"]

So that you can configure the behavior. What do you think?

@dslemusp
Copy link
Author

dslemusp commented Jul 8, 2025

I like it!

@dslemusp
Copy link
Author

dslemusp commented Jul 8, 2025

How many processing steps do we have?. I see split, preprocess, commit_parsing, link_parsing, into_conventional

@ognis1205
Copy link
Contributor

@dslemusp @orhun

I assume the current step names like preprocess, process, and postprocess are just temporary placeholders for now, since this is still in a draft stage. That said, if we move forward with making the order configurable, I think it’d be good to avoid names like those — they imply a fixed sequence, which might be misleading if the steps can be reordered. It’d be great if we could find names that reflect the actual behavior more clearly.

@dslemusp
Copy link
Author

dslemusp commented Jul 8, 2025

I agree. I would suggest we use similar (if not same) names as they are defined in the cliff.toml (e.g. commit_preprocessor instead of preprocess ). I am working on updating the PR but it might take some time!. Feel free to think about the naming in the mean time ;P

@orhun
Copy link
Owner

orhun commented Jul 11, 2025

I would suggest we use similar (if not same) names as they are defined in the cliff.toml

Yes, very good point. I'd like that 👍🏼

I am working on updating the PR but it might take some time!

Great, thank you! Let me know if you need any help/guidance in the meantime. I think we should also update the title of this PR accordingly :D

@dslemusp dslemusp changed the title feat(changelog): flatten commit list before processing feat(changelog): implement commit processing order as set of steps Jul 11, 2025
@dslemusp
Copy link
Author

Ok I found some time today I came up with this. Maybe there is still some cleanup to do as this makes

pub fn process(&self, config: &GitConfig) -> Result<Self> {
not needed.

@dslemusp dslemusp force-pushed the feat/flatten_before_processing branch from 41e3e8a to 98fc267 Compare September 2, 2025 09:39
@dslemusp
Copy link
Author

dslemusp commented Sep 2, 2025

@orhun Any follow up on this?. I would like to have this functionality merged

Comment on lines +221 to +249
fn process_commit_list(commits: &mut Vec<Commit<'a>>, git_config: &GitConfig) -> Result<()> {
for step in &git_config.processing_order.order {
match step {
ProcessingStep::CommitParsers => {
debug!("Applying commit parsers...");
*commits = Self::apply_commit_parsers(commits, git_config);
}
ProcessingStep::CommitPreprocessors => {
debug!("Applying commit preprocessors...");
*commits = Self::apply_commit_preprocessors(commits, git_config);
}
ProcessingStep::IntoConventional => {
debug!("Converting commits to conventional format...");
*commits = Self::apply_into_conventional(commits, git_config);
}
ProcessingStep::LinkParsers => {
debug!("Applying link parsers...");
*commits = Self::apply_link_parsers(commits, git_config);
}
ProcessingStep::SplitCommits => {
debug!("Splitting commits...");
if git_config.split_commits {
*commits = Self::apply_split_commits(commits);
} else {
debug!("Split commits is disabled, skipping...");
}
}
}
}
Copy link
Author

@dslemusp dslemusp Sep 18, 2025

Choose a reason for hiding this comment

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

A thing to keep in mind is that if all the steps are not listed in the config they will be skipped. So maybe we need to check that the list always contains all the steps (no matter the order)

Copy link
Contributor

@ognis1205 ognis1205 left a comment

Choose a reason for hiding this comment

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

Another important point: since there are already issues arising from the interactions of repository and directory-related configs, I think we should be very careful about introducing new functionality into the config. Both the necessity and the exact specification should be carefully evaluated before adding more to this area.

/// Processing steps for the commits
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
pub enum ProcessingStep {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestions:

  1. Make the enum variant names and config keys consistent and explicit about what the slot does. Prefer either explicit Verb + Object action names or neutral stage names so the intent is clear. Example mappings:
  • CommitParsers -> ParseCommits
  • CommitPreprocessors -> ModifyCommits
  • IntoConventional -> ConventionalizeCommits or NormalizeCommits
  • LinkParsers -> ParseLinks
  • SplitCommits -> SplitCommits
  1. Rename CommitPreprocessors to a name that reflects it being an ordering slot rather than implying it always runs "before" something. Options:
  • If message transformations/modification are the main behavior: TransformCommits or ModifyCommits.

Copy link
Author

@dslemusp dslemusp Sep 25, 2025

Choose a reason for hiding this comment

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

Thx for the suggestions, I think it is indeed nice to have consistent naming. However I think the naming is more a fundamental change as I am using the current naming definitions from the configuration (see https://git-cliff.org/docs/configuration/git). I can indeed modify the names with the caveat of using multiple names for the same (e.g. ModifyCommits in place of CommitPreprocessors for this step https://git-cliff.org/docs/configuration/git#commit_preprocessors).
It seems that @orhun agreed on using the same names as in the config.toml (#1201 (comment)).

Copy link
Owner

Choose a reason for hiding this comment

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

Yup, I think it makes more sense to use the names that we are already familiar.

@dslemusp
Copy link
Author

dslemusp commented Sep 25, 2025

Another important point: since there are already issues arising from the interactions of repository and directory-related configs, I think we should be very careful about introducing new functionality into the config. Both the necessity and the exact specification should be carefully evaluated before adding more to this area.

I tend to agree, but in this case the idea is to keep feature parity and backwards comparability in case the processing_steps field is not present in the cliff.toml
When I have time I refactor the tests to leave them as they were on the base and add new ones for the feature. This to also check in a way that there are no regressions.

@orhun
Copy link
Owner

orhun commented Sep 30, 2025

Sorry for the delay on this... but most things look good already. Just need to find some time to do proper review.

Pretty excited for this one

Btw can you fix the conflicts with main?

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.

4 participants