-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
fix: assign commits to closest tag #711
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #711 +/- ##
==========================================
- Coverage 43.08% 42.46% -0.62%
==========================================
Files 21 22 +1
Lines 1718 1795 +77
==========================================
+ Hits 740 762 +22
- Misses 978 1033 +55
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Keeping as draft as existing tests pass, but I still have to add new ones from the linked issue and some more edge cases I have in mind |
@@ -206,11 +206,139 @@ impl Repository { | |||
} | |||
} | |||
|
|||
/// Stores which commits are tagged with which tags. | |||
#[derive(Debug)] | |||
pub struct TaggedCommits<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to its own module (tagged.rs
or tagged_commit.rs
) and add unit tests! 🐻
git-cliff/src/lib.rs
Outdated
@@ -324,6 +306,42 @@ fn process_repository<'a>( | |||
Ok(releases) | |||
} | |||
|
|||
fn fill_release( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need separate functions for these? I feel like they are additional complexity.
I'm okay with it if we test them though.
Hey @DaniPopes are you still interested in working on this? |
Hey, sorry I haven't been able to allocate more time on this, and I'll be off for a few weeks. |
Sounds good :) |
Description
Previously, when creating releases (assigning commits to tags), a new release was considered only when a tagged commit is encountered. This works fine in most cases, however, when used together with
--include-path
/--exclude-path
, which may filter out one of these tagged commits, the entire tag would be skipped too.This PR makes sure tags are never skipped by looking up the closest tag instead of trying to find an exact match for a given commit when creating new releases.
Additionally, this PR also fixes the issue where the first (root) commit of the repository was never included in the output when
--include-path
/--include-path
was used.Both of these issues are described in #208.
Based on #709 to minimize conflicts.
Motivation and Context
Fixes #208.
How Has This Been Tested?
cargo test
+ manual testing on repositories I use git-cliff on with--include-path
, and with the script provided in the issue.Screenshots / Logs (if applicable)
Types of Changes
Checklist: