DRAFT: package changelog

[ABWassim]

I also chose to remove the get_changelog_at_tag function since it is only used in the context of cog changelog and it only makes a call to get_changelog. The --at and pattern of cog changelog are now processed in the "body" of cog changelog to directly compute the final pattern.

What do you think ? Gonna write tests + doc soon

[codecov]

Codecov Report

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

Project coverage is 86.17%. Comparing base (5021a13) to head (42c48fe).
Report is 10 commits behind head on main.

❗ Current head 42c48fe differs from pull request most recent head b5f59b4

Please upload reports for the commit b5f59b4 to get more accurate results.

Files	Patch %	Lines
src/command/changelog.rs	0.00%	10 Missing ⚠️
src/bin/cog/main.rs	50.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #380      +/-   ##
==========================================
- Coverage   86.38%   86.17%   -0.21%     
==========================================
  Files          49       49              
  Lines        7072     7081       +9     
==========================================
- Hits         6109     6102       -7     
- Misses        963      979      +16     

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

[ABWassim]

Hey @oknozor,

I noticed a weird behavior when a commit contains multiple tags (which can happen when bumping with multiple packages that were updated). Looks like when generating a Changelog for a package, it takes the latest created tag on the commit rather than the one that belongs to the package. It is illustrated by the test I added :

• Feat commits on the package one.
• Bump everything with the --auto option -> We get a 0.1.0 and a one-0.1.0 tag.
• Create a package called two.
• Commits on package one and two
• Bump everything with the --auto option -> We get a 0.2.0, a one-0.2.0 and a two-0.1.0 tag.
• Generated the Changelog for package one

## two-0.1.0 (should be one-0.2.0) - 2024-04-19
#### Features
- one feature - (17ef0a3) - Tom
#### Miscellaneous Chores
- **(version)** 0.2.0 - (2a6c0d2) - Tom

- - -
## one-0.1.0 - 2024-04-19
#### Features
- package one feature - (f270338) - Tom
#### Miscellaneous Chores
- **(version)** 0.1.0 - (9f00137) - Tom

Looks like the issue comes from the revwalk function where each oid is either a commit or the latest created tag on a commit, which doesn't allow us getting every tag of commit so far.

I need to figure out a clean solution for that issue but if you have some idea I'd like to hear it :)

[oknozor]

Hey I @ABWassim, I suspected this would be an issue at some point.
I think the solution is to change the layout of the cache:

cocogitto/src/git/rev/cache.rs

Line 9 in 23e4310

static REPO_CACHE: Lazy<Arc<Mutex<BTreeMap<String, OidOf>>>> =

Instead of BTreeMap<String, OidOf> we should have BTreeMap<String, Vec<OidOf>> to hold multiple tags for a single commit sha.

Once we have that we should be able to change the resolve_oid_of function (or create a dedicated one) to force the lookup of a specific package tag.

[ABWassim]

Hey @oknozor, thanks for the hint ! I'll try my best at it

[ABWassim]

Hey @oknozor :)

Finally have some more time to work on this. I changed the layout of the cache like you said and proceeded to change the implementation of resolve_oid_of() but I'm kind of struggling with a situation illustrated by the shoud_get_range_for_a_multiple_release() test.

If a tag exists on the HEAD commit (let's say 0.2.0 like in the failing test), the cache has a key/value pair in the form head_commit_sha -> ["0.2.0", "HEAD"]. My question is : which one do we pick ?

In the context of that test, we would need 0.2.0 cause here we're trying to create releases so we need to set release.version to 0.2.0, but is there other cases where we need to specifically pick either the tag or HEAD ?

I still left some messy debug with good old println!("") but you can replay the test if you want.

[oknozor]

Hello @ABWassim,

If a tag exists on the HEAD commit (let's say 0.2.0 like in the failing test), the cache has a key/value pair in the form head_commit_sha -> ["0.2.0", "HEAD"]. My question is : which one do we pick ?

This is context dependent. When generating a changelog, we don't want to use HEAD because the repo head change while the changelog will stay.

In the context of that test, we would need 0.2.0 cause here we're trying to create releases so we need to set release.version to 0.2.0, but is there other cases where we need to specifically pick either the tag or HEAD ?

I think the only use for HEAD is in the revspec pattern resolution (ex: cog changelog ..HEAD). This is not an issue because, it will always fallback to native git revparse_single.
Whenever we generate an output head could be ignored.

In other word I think your current PR would do the job for now :)

---

That said I think there is another possible approach.
I don't know if its worth trying though.

We could change the OidOf::Tag variant to hold multiple tags. This way there would be not need for a package specific revwalk and resolve_oid_of implementation.

That would mean a few addition to the OidOf impl, such as method to filter the desired tags at the last possible moment (during changelog generation). But it implies a lot of changes to everywhere OidOf is used. I am aware this is a big refactoring and not sure it's worth it.

#[derive(Debug, PartialEq, Eq, Clone)]
pub enum OidOf {
    Tag(Tags),
    Head(Oid),
    Other(Oid),
    FirstCommit(Oid),
}

#[derive(Debug, PartialEq, Eq, Clone)]
pub struct Tags {
    inner: Vec<Tag>,
}

impl Tags {
    /// Try to find a specific package name tag. The caller probably want to take the first one.
    fn filter_package(&self, package: &str) -> impl Iterator<Item=&Tag> {
        self.inner.iter()
            .filter(|t| t.package.as_ref().map(|p| p == package).unwrap_or_default())
    }

    /// Try to find a all package tag. 
    fn packages(&self) -> impl Iterator<Item=&Tag> {
        self.inner.iter()
            .filter(|t| t.package.is_some()))
    }

    /// Try to find non package tag. 
    fn global(&self) -> impl Iterator<Item=&Tag> {
        // probably requires to filter on the configured prefix
        self.inner.iter()
            .filter(|t| t.package.is_none()))
    }

   // etc
}

⚠️ There is a PR is progress to turn cocogitto into a cargo workspace (#392). I will freeze new feature until this is done

You probably want to change your PR to target that branch, since it splits cog into many package you will need to adapt a lot of your code I am afraid.

[ABWassim]

Hey @oknozor,

Again thanks for your help. I'll try changing the OidOf::Tag variant and compare both solutions :)