Skip to content

Commit

Permalink
fix use of span to get number of days
Browse files Browse the repository at this point in the history
This commit revises the change in Byron#28 to be a bit more robust.

Originally, before Byron#28, the code was buggy because, I think, `Span` was
being used like a normal "absolute" duration. But a `Span` keeps track
of values for each individual unit. It isn't just a single number of
nanoseconds like, e.g., `std::time::Duration` is. It's "smarter" than
that. It deals with non-uniform units like days, months and years. But
to do it correctly, you need a reference date.

What this means is that when you get a `Span` by subtracting two `Zoned`
values, you can't just ask the `Span` for the total number of days via
`get_days()`. It has to be *computed*. In Byron#28, this was done for one
case but not the other via the `Span::total` API. While this works
today, the code was not providing a reference date, which means days are
silently treated as always being 24 hours long.
See BurntSushi/jiff#48 for more details where
it's likely that this sort of usage will return an error in `jiff 0.2`.

The main gotcha here is that since this is using `gix::date`, the
`Zoned` values that are created are just "fixed offset" datetimes. They
don't actually have a time zone. So in practice, such datetimes will
always have all days be 24 hours long. This is not correct, but it's not
clear to me that this is fixable inside the context of `git`. But at
least with this patch, if you do ever end up using true time zones, then
this code will be robust to that.
  • Loading branch information
BurntSushi committed Aug 31, 2024
1 parent 44f2371 commit 9309196
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 17 deletions.
28 changes: 23 additions & 5 deletions src/changelog/section/from_history.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{collections::BTreeMap, ops::Sub};
use std::collections::BTreeMap;

use cargo_metadata::Package;
use gix::prelude::ObjectIdExt;
Expand Down Expand Up @@ -88,14 +88,32 @@ impl Section {
.as_ref()
.filter(|_| selection.contains(Selection::COMMIT_STATISTICS))
{
let duration = history
.last()
.map(|last| date_time.sub(&time_to_zoned_time(last.commit_time).expect("valid time")));
let duration = history.last().and_then(|last| {
let first_commit_time = time_to_zoned_time(last.commit_time).expect("valid time");
let span = date_time
.since(
jiff::ZonedDifference::new(&first_commit_time)
.smallest(jiff::Unit::Day)
.largest(jiff::Unit::Day),
)
.ok()?;
Some(span.get_days())
});
let time_passed_since_last_release = prev_date_time.and_then(|prev_time| {
let span = date_time
.since(
jiff::ZonedDifference::new(&prev_time)
.smallest(jiff::Unit::Day)
.largest(jiff::Unit::Day),
)
.ok()?;
Some(span.get_days())
});
segments.push(Segment::Statistics(section::Data::Generated(
section::segment::CommitStatistics {
count: history.len(),
duration,
time_passed_since_last_release: prev_date_time.map(|prev_time| date_time.sub(&prev_time)),
time_passed_since_last_release,
conventional_count: history.iter().filter(|item| item.message.kind.is_some()).count(),
unique_issues: {
let mut v = commits_by_category
Expand Down
8 changes: 4 additions & 4 deletions src/changelog/section/segment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,14 +109,14 @@ impl Details {
pub struct CommitStatistics {
/// Amount of commits that contributed to the release
pub count: usize,
/// The time span from first to last commit, if there is more than one.
pub duration: Option<jiff::Span>,
/// The time span, in days, from first to last commit, if there is more than one.
pub duration: Option<i32>,
/// Amount of commits that could be parsed as git-conventional
pub conventional_count: usize,
/// The issue numbers that were referenced in commit messages
pub unique_issues: Vec<details::Category>,
/// The duration from the release before this one, if this isn't the first release.
pub time_passed_since_last_release: Option<jiff::Span>,
/// The duration, in days, from the release before this one, if this isn't the first release.
pub time_passed_since_last_release: Option<i32>,
}

impl CommitStatistics {
Expand Down
12 changes: 4 additions & 8 deletions src/changelog/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,19 +320,15 @@ impl section::Segment {
count,
if *count == 1 { "commit" } else { "commits" },
match duration {
Some(duration) if duration.get_days() > 0 => format!(
&Some(duration) if duration > 0 => format!(
" over the course of {} calendar {}.",
duration.get_days(),
if duration.get_days() == 1 { "day" } else { "days" }
duration,
if duration == 1 { "day" } else { "days" }
),
_ => ".".into(),
}
)?;
if let Some(days_between_releases) = time_passed_since_last_release
.and_then(|d| d.total(jiff::Unit::Day).ok())
.map(|d| d.floor() as u64)
.filter(|d| *d > 0)
{
if let Some(days_between_releases) = time_passed_since_last_release.filter(|d| *d > 0) {
writeln!(
out,
" - {} {} passed between releases.",
Expand Down

0 comments on commit 9309196

Please sign in to comment.