feat(context): add release commit range #1123

jdrst · 2025-04-03T09:35:47Z

Description

Adds the used commit_range for each release.
commit_range includes the full length commit ids (commit_range.from and commit_range.to) and the short ids (commit_range.from_short and commit_range.to_short)
Also added related docs (and documentation for the sort_commits default)

Motivation and Context

See #1041

How Has This Been Tested?

Via integration_test and three new fixtures.

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.

/edit: added a third fixture. no more caveats + documentation and rustfmt/clippy

welcome · 2025-04-03T09:35:51Z

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

codecov-commenter · 2025-04-03T09:41:02Z

Codecov Report

Attention: Patch coverage is 40.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 41.08%. Comparing base (aec41be) to head (1b8ecf8).

Files with missing lines	Patch %	Lines
git-cliff/src/lib.rs	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1123      +/-   ##
==========================================
- Coverage   41.09%   41.08%   -0.00%     
==========================================
  Files          21       22       +1     
  Lines        1845     1860      +15     
==========================================
+ Hits          758      764       +6     
- Misses       1087     1096       +9

Flag	Coverage Δ
unit-tests	`41.08% <40.00%> (-<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.

jdrst · 2025-04-03T09:58:56Z

working on it :D

jdrst · 2025-04-03T11:07:31Z

i am unsure how to fix that timeout. is this flaky?

orhun · 2025-04-05T16:35:55Z

That links check should be fine... I'll take care of it later

orhun

Thanks for the implementation!

I had some comments and ideas :)

website/docs/usage/adding-commits.md

git-cliff-core/src/commit_range.rs

orhun · 2025-04-05T16:43:04Z

git-cliff-core/src/commit_range.rs

+			from_short: from.short_id.clone(),
+			to_short:   to.short_id.clone(),


I'm thinking maybe we can remove the short variants... the templating can be used to trim them anyways.

e.g.

{{ commit_range.to | truncate(length=7, end="") }}

Also I have another idea which might change the whole implementation but... maybe adding this to the commit object would be cleaner?

e.g.

{{ commit.range.to | truncate(length=7, end="") }}

What do you think?

I'm thinking maybe we can remove the short variants... the templating can be used to trim them anyways.

Short ids aren't necessarily unique when just truncated. I'd consider using the actual short id coming from git as more robust.

Also I have another idea which might change the whole implementation but... maybe adding this to the commit object would be cleaner?

I'm not sure i can follow? The commit has no idea about a range, or does it? Or do you mean, instead of having the range on the release, have it on every commit in the release? I think i'd consider that counterintuitive.

Oh yeah sorry, my bad. I was a bit confused there. It definitely does not belong to commit object :)

Short ids aren't necessarily unique when just truncated. I'd consider using the actual short id coming from git as more robust.

I'd like to double down on this though... We don't have 2 variants (short, long) in the commit object itself and it might lead to inconsistencies (in very rare cases) if we go with short-long here. So I'd prefer we only go with the long variant here.

Thoughts?

I'd also like to double down :D (one last try).
What kind of inconsistencies?
I'd rather change e.g. the commit in the context to be an object with long+short (that'd be breaking though).

What kind of inconsistencies?

You know, we'll be supporting short hashes in one variable (commit_range) but not for the other (commit) which is not great.

The reason why I don't like supporting short commit hashes is because 7 characters is not long enough and it's more probable that two entirely different commits might belong to the same short hash.

Even worse, you can override the first 7 digits of a commit hash with a tool like lucky-commit:

$ git log 1f6383a Some commit $ lucky_commit $ git log 0000000 Some commit

So you can have a repo with all 000000 ids, see this git history.

You know, we'll be supporting short hashes in one variable (commit_range) but not for the other (commit) which is not great.

That i absolutely understand, hence my proposition to also add it to the commit itself. But one could also argue that commit and commit_range are different use cases with different needs.

The reason why I don't like supporting short commit hashes is because 7 characters is not long enough and it's more probable that two entirely different commits might belong to the same short hash.

That is exactly my point, just from a different perspective :D. This PR does make use of git_object_short_id to circumvent that (see here)

Sorry for being so persistent. You can just put your foot down and say "Please remove the short id"!

I'll try playing tie-breaker ;)

I'd remove the short variants too. We are talking about configuration here. I don't see a point in ommitting a few bytes here. Putting in the long hash doesn't hurt and is more expressive.

Having only one set simplifies both the code and the documentation too.

Hey again, needed to take some break due to a small injury but I'm hoping to get this merged before the release soon :)

So I think we should go without short hashes here and rebase on main to fix the conflicts. The rest should be ready.

@jdrst do you mind incorporating these changes? 😊

orhun · 2025-04-12T09:55:09Z

We need to rebase on main and fix conflicts 😊

jdrst · 2025-04-20T20:28:42Z

Hey, no problem! I'm currently on vacation though. I could offer some time within the week after the next (so somewhere between 28.4 and 2.5). Does that still fit your timeframe? If not, i think you have editing rights, so if it's really pressing you could do it yourself. The rebase was uncomplicated, i just didn't push it yet :/

…

On April 20, 2025 9:48:24 PM GMT+02:00, "Orhun Parmaksız" ***@***.***> wrote: @orhun commented on this pull request. > + from_short: from.short_id.clone(), + to_short: to.short_id.clone(), Hey again, needed to take some break due to a small injury but I'm hoping to get this merged before the release soon :) So I think we should go without short hashes here and rebase on `main` to fix the conflicts. The rest should be ready. @jdrst do you mind incorporating these changes? 😊 -- Reply to this email directly or view it on GitHub: #1123 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

orhun · 2025-04-22T20:14:32Z

Hey that works, I also tried to do changes here but I can't push to your branch :/

 ! [remote rejected]   jdrst/main -> jdrst/main (permission denied)
error: failed to push some refs to 'https://github.com/jdrst/git-cliff.git'

orhun · 2025-04-22T20:15:30Z

git-cliff-core/src/range.rs

We can move the entirety to the commit module I think.

orhun · 2025-04-22T20:16:31Z

git-cliff/src/lib.rs

+	// Set the commit ranges for all releases
+	for release in &mut releases {


Is it possible that we use an already existing loop for this instead of looping through releases again?

e.g. we can set this in

git-cliff/git-cliff/src/lib.rs

Line 302 in aec41be

for git_commit in commits.iter().rev() {

Maybe

Will have a look. From what i remember it would've needed a lot of branching/complicated logic because we don't know all of the commits (or the release) we're in until everything is added (and then we can only add it before we switch to the previous release).

I think i opted for this because it is O(n) with n being the number of releases. That should still be fine even with lots of releases.

/edit: formatting

Have a look at process_submodules. It does the same thing because it also needs to have the first and last commit of a release. Maybe you can merge the two loops?

jdrst · 2025-04-22T20:22:37Z

Hey that works, I also tried to do changes here but I can't push to your branch :/
 ! [remote rejected]   jdrst/main -> jdrst/main (permission denied)
error: failed to push some refs to 'https://github.com/jdrst/git-cliff.git'

That is weird..

I just turned it off and on again. Maybe it broke because i accidentally force pushed no changes in between and this PR got closed automatically?

orhun · 2025-04-26T14:28:30Z

No it didn't work :/ Maybe you have branch protection on on main or something, I don't really know...

jdrst · 2025-04-26T15:02:00Z

No it didn't work :/ Maybe you have branch protection on on main or something, I don't really know...

no idea.. i just forked. no branch protection active. the hint even states

If checked, users with write access to orhun/git-cliff can add new commits to your main branch.
You can always change this setting later.

i just added you as contributor..

orhun · 2025-04-26T15:28:45Z

Ok I just pushed a commit to there... it doesn't show up in this PR though. What is going on...

orhun · 2025-04-26T15:30:31Z

Ok I messed up something

orhun · 2025-04-26T15:31:48Z

Can we maybe create a new branch and open a new PR from there? Creating a PR from the main branch makes things super confusing...

jdrst · 2025-04-26T16:29:58Z

Can we maybe create a new branch and open a new PR from there? Creating a PR from the main branch makes things super confusing...

aight, will do next week. you could upload a patch here or smth.

orhun · 2025-04-28T10:59:44Z

The changes are in the main branch here: https://github.com/jdrst/git-cliff/tree/main

jdrst · 2025-04-28T13:56:19Z

The changes are in the main branch here: https://github.com/jdrst/git-cliff/tree/main

currently rebasing, cleaning up and incorporating requested changes, will pr soon.
i squashed your commit into mine and added you as co-author. i hope that's okay?

orhun · 2025-04-28T13:57:15Z

yup! thanks

jdrst requested a review from orhun as a code owner April 3, 2025 09:35

jdrst force-pushed the main branch 3 times, most recently from 0c861bc to 132a847 Compare April 3, 2025 10:53

orhun reviewed Apr 5, 2025

View reviewed changes

jdrst force-pushed the main branch from 132a847 to 13598fb Compare April 7, 2025 09:59

jdrst closed this Apr 7, 2025

jdrst force-pushed the main branch from 13598fb to aec41be Compare April 7, 2025 10:00

jdrst reopened this Apr 7, 2025

jdrst force-pushed the main branch 4 times, most recently from e22b038 to 1b8ecf8 Compare April 7, 2025 10:47

orhun self-requested a review April 12, 2025 09:55

orhun reviewed Apr 22, 2025

View reviewed changes

orhun closed this Apr 26, 2025

orhun force-pushed the main branch from 1b8ecf8 to 28f0c0e Compare April 26, 2025 15:29

		from_short: from.short_id.clone(),
		to_short: to.short_id.clone(),

		// Set the commit ranges for all releases
		for release in &mut releases {

Uh oh!

feat(context): add release commit range #1123

feat(context): add release commit range #1123

Uh oh!

Conversation

jdrst commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of Changes

Checklist:

Uh oh!

welcome bot commented Apr 3, 2025

Uh oh!

codecov-commenter commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jdrst commented Apr 3, 2025

Uh oh!

jdrst commented Apr 3, 2025

Uh oh!

orhun commented Apr 5, 2025

Uh oh!

orhun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orhun Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orhun commented Apr 12, 2025

Uh oh!

jdrst commented Apr 20, 2025 via email

Uh oh!

orhun commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdrst Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdrst commented Apr 22, 2025

Uh oh!

orhun commented Apr 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdrst commented Apr 26, 2025

Uh oh!

orhun commented Apr 26, 2025

Uh oh!

orhun commented Apr 26, 2025

Uh oh!

orhun commented Apr 26, 2025

jdrst commented Apr 3, 2025 •

edited

Loading

codecov-commenter commented Apr 3, 2025 •

edited

Loading

orhun Apr 13, 2025 •

edited

Loading

jdrst Apr 22, 2025 •

edited

Loading

orhun commented Apr 26, 2025 •

edited

Loading