Skip to content

feat: support skipping based on blame.ignoreRevsFile #1125

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

Closed
wants to merge 6 commits into from

Conversation

michen00
Copy link

@michen00 michen00 commented Apr 8, 2025

Description

Added support for:

  • excluding hashes listed in the blame ignore file from the changelog
  • excluding commits that only touch the blame ignore file from the changelog

Motivation and Context

This PR addresses #1084.

How Has This Been Tested?

I just ran cargo test.

Screenshots / Logs (if applicable)

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.

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2025

Codecov Report

Attention: Patch coverage is 41.66667% with 14 lines in your changes missing coverage. Please review.

Project coverage is 41.09%. Comparing base (aec41be) to head (8103008).

Files with missing lines Patch % Lines
git-cliff-core/src/changelog.rs 56.25% 7 Missing ⚠️
git-cliff/src/lib.rs 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1125      +/-   ##
==========================================
- Coverage   41.09%   41.09%   -0.00%     
==========================================
  Files          21       21              
  Lines        1845     1867      +22     
==========================================
+ Hits          758      767       +9     
- Misses       1087     1100      +13     
Flag Coverage Δ
unit-tests 41.09% <41.67%> (-<0.01%) ⬇️

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.

Comment on lines +103 to +132
### blame_ignore_revs_file

Path to a file containing a list of commit refs to ignore when generating blame information (configured with `git config blame.ignoreRevsFile`). The file should contain one commit ref per line. By convention, this file is named `.git-blame-ignore-revs` and is located in the root of the repository.

When used with `filter_blame_ignored_revs` set to `true`, commits that are listed in the file will be excluded from the changelog.

```toml
[git]
blame_ignore_revs_file = ".git-blame-ignore-revs"
```

### filter_blame_ignored_revs

If set to `true`, commits that are listed in the `blame_ignore_revs_file` will be excluded from the changelog.

```toml
[git]
blame_ignore_revs_file = ".git-blame-ignore-revs"
filter_blame_ignored_revs = false
```

### filter_mono_commits_to_blame_ignore_file

If set to `true`, commits that only modify the `blame_ignore_revs_file` will be excluded from the changelog.

```toml
[git]
blame_ignore_revs_file = ".git-blame-ignore-revs"
filter_mono_commits_to_blame_ignore_file = true
```
Copy link
Owner

Choose a reason for hiding this comment

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

Having 3 new options for handling a single functionality seems like an overkill. I recall we discussed having sane default for these and a single option for tweaking it, no?

Copy link
Author

Choose a reason for hiding this comment

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

We did discuss that, but perhaps I'm not understanding what you mean by sane defaults. I'll describe how I was thinking about it and maybe the gap in our understanding will be clear.

This draft PR introduces two new functionalities centered on the git blame ignoreRevs file:

  1. Allows exclusion of commits hashes in the blame ignore file from the change log (disabled by default)
  2. Allows exclusion of single-file commits that modify only that file from the change log (enabled by default)

There are three new options that configure these two new features. These have sensible default values that anticipate what most users of git cliff would prefer:

  • blame_ignore_revs_file: .git-blame-ignore-revs
    This file name is the most common, basically a de facto industry standard. It is configurable to support dev flows using some other naming convention.
  • filter_blame_ignored_revs: false
    Even style commits can introduce breaking changes or be the difference between CI pass/fail, so these shouldn't be ignored by default.
  • filter_mono_commits_to_blame_ignore_file: true
    These kinds of commits are typically isolated to documenting style commits. It's hard for me to see how these are useful info for a change log. (I just thought of an edge case for this one. A commit that only creates the blame ignoreRevs file should still be included.)

Do feel that my rationale for choosing the default values above could be improved? Or perhaps my code changes are not representing my intent well?

Or is there a better way of configuring that involves fewer options keys?

For instance, are you advocating that we implement only one of these two features? In this case, only the blame_ignore_revs_file config is needed. The default would probably be unset, and setting it would enable the feature.

We can also keep the single setting (blame_ignore_revs_file) config if providing the filename means both functionalities are enabled. Personally, I am interested in the second feature but not the first for my own stuff, so this wouldn't be very useful for me.

Another way of doing this is keeping only blame_ignore_revs_file and filter_blame_ignored_revs. Setting blame_ignore_revs_file would entail/imply that monofile commits to the blame ignore file get filtered from the changelog. Further setting filter_blame_ignored_revs to truthy then enables filtering the documented style commits. Setting only filter_blame_ignored_revs could use a default .git-blame-ignore-revs without the filter_mono_commits_to_blame_ignore_file behavior, but you wouldn't be able to use a different file name.

What do you think? I am most interested in filtering the monofile blame ignore commits, so I'm open to whichever path you feel is sensible to achieve that.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the detailed explanation! (and sorry for the late response)

What I meant with the sane defaults is that we shouldn't make these options configurable in the first place. In other words, I'm suggesting that we should not expose them in the config just yet - because they are very specific and I'm guessing if someone is using them they are just using the defaults :)

Adding 3 new options to the config for this might lead to clutter and confusion in my opinion. We can always add them later if anyone needs to tweak them. (That's what we did with some of the config options as well)

With that being said, I'm okay with the following defaults:

  • blame_ignore_revs_file = ".git-blame-ignore-revs"
  • filter_blame_ignored_revs = true
  • filter_mono_commits_to_blame_ignore_file = ? -> hmm, I'm not sure about this one

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your response; I think I am understanding a bit better. What I'm still not understanding is how many options you would actually like to add to the config. Are we going to expose any of these? If so, are we exposing just one or two of them?

As for the default values:

  • blame_ignore_revs_file = ".git-blame-ignore-revs" 👍
  • filter_blame_ignored_revs = true
    I'm not so keen on this default value. The hashes in the file are mostly style commits, but even these sometimes introduce breaking changes. I prefer having the style commits included in the changelog in their own section. If you'd prefer the default to be true, could we please expose this option to the config so I can disable it for my own workflows?
  • filter_mono_commits_to_blame_ignore_file = true is the default value I would propose. These kinds of commits typically have a summary that goes something like 'docs(blame): exclude a commit from blame' or 'chore: add a commit to .git-blame-ignore-revs'. I would like to exclude commits that do nothing but modify .git-blame-ignore-revs from the changelog.

And no worries on the late response. I'm only able to work on this in my spare time as well :)

Copy link
Owner

Choose a reason for hiding this comment

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

What I'm still not understanding is how many options you would actually like to add to the config. Are we going to expose any of these? If so, are we exposing just one or two of them?

I think zero for now. We can internally support this and expose these options if people request to configure them. My reasoning for that is I don't want to bother people with more options and make the configuration cumbersome :)

Regarding filter_blame_ignored_revs, I'm not sure if I understood its purpose fully and it's not very clear from the name itself. Can you maybe explain it a bit more and we can potentially expose it.

Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to explain it more and am open to a clearer, more descriptive, or concise name.

The .git-blame-ignore-revs file tells the git blame command to ignore certain commits, typically those that made large-scale, mechanical, or cosmetic changes — such as reformatting code, applying linter fixes, or renaming variables — that would otherwise obscure the true history of a line. Some developers may want to also ignore those same commits when creating a changelog. filter_blame_ignored_revs would allow those commits to be excluded, much like the .cliffignore file.

Although I could see both true/false as a suitable default value for filter_blame_ignored_revs, I would like this set to false for my own workflows. Here are two reasons why I prefer this personally:

  • style commits are the conventional category most often listed in .git-blame-ignore-revs. git cliff nicely categorizes all my other conventional commits, so I would like to treat these the same way.
  • Just because the devs chose to exclude some commit from blame by including it in .git-blame-ignore-revs does not entail an 'unimportant' change. Indeed, these changes often make the difference between a passing or failing CI run.

If we proceed with 0 exposed options and a default of true for filter_blame_ignored_revs, I would be first in line to request that it be exposed.

At any rate, it's sounding like maybe 2 PRs are appropriate here:

  1. blame_ignore_revs_file = ".git-blame-ignore-revs" + filter_mono_commits_to_blame_ignore_file = true (both unexposed defaults)
  2. filter_blame_ignored_revs = ? + one or zero exposed option (tbd)

If you're okay with the default values in the first one, I can get started re-drafting the PR the next time I can work on this. (Also, you or other contributors should feel free to implement this since I won't be able to do it soon.)

Copy link
Owner

Choose a reason for hiding this comment

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

Apologies for the long delay... I got sidetracked and this issue got lost in the pile of things to do.

I just got myself up-to-date with this conversation and your proposed solution seems like a good way to move forward with this. If you still have the interest and time, feel free to come back to this! Would love get this supported in git-cliff eventually.

Copy link
Author

Choose a reason for hiding this comment

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

No worries, life gets busy for anyone.

I'm glad we are aligned! To be honest, I don't know when I will get to this. But I do know that eventually I will want to produce a changelog for a project of mine using conventional commits and .git-blame-ignore-revs, so I am likely to return :)

I did do some vibe coding with Codex last week on this, but I'll admit I don't know enough rust yet to confidently review the code, so I was trying to find time to go through it line by line.

Let's keep this conversation open for now. We can resolve it when a PR is drafted for the first PR as discussed above.

Copy link
Owner

Choose a reason for hiding this comment

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

so I am likely to return :)

That's awesome to hear!

I did do some vibe coding with Codex last week on this, but I'll admit I don't know enough rust yet to confidently review the code, so I was trying to find time to go through it line by line.

One of my friends is an efficient user of Claude Code and he apparently getting good results when the enough context is provided. Either way, I think getting an implementation strategy from an LLM and doing the implementation yourself is a good workflow.

Let's keep this conversation open for now. We can resolve it when a PR is drafted for the first PR as discussed above.

Yup, sounds good. I think we can close the PR and keep the discussion open, or just create a new discussion which might be better.

Copy link
Author

Choose a reason for hiding this comment

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

As suggested, I have created a new discussion at #1222 and will close this PR for now.

@michen00 michen00 closed this Jul 19, 2025
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