feat: Submodule commit processing #1082

lehmanju · 2025-03-07T09:29:57Z

So #1058 inspired me to try submodule support myself. It is not intended to be "better" or "replace" @yudjinn 's efforts. Since this feature is a blocker for me anyway I might as well put some time into it.

welcome · 2025-03-07T09:30:00Z

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

codecov-commenter · 2025-03-07T09:36:12Z

Codecov Report

Attention: Patch coverage is 25.27473% with 68 lines in your changes missing coverage. Please review.

Project coverage is 40.67%. Comparing base (729aa47) to head (9e8b649).

Files with missing lines	Patch %	Lines
git-cliff/src/lib.rs	0.00%	26 Missing ⚠️
git-cliff-core/src/repo.rs	0.00%	23 Missing ⚠️
git-cliff-core/src/changelog.rs	53.66%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1082      +/-   ##
==========================================
- Coverage   41.42%   40.67%   -0.74%     
==========================================
  Files          21       21              
  Lines        1811     1859      +48     
==========================================
+ Hits          750      756       +6     
- Misses       1061     1103      +42

Flag	Coverage Δ
unit-tests	`40.67% <25.28%> (-0.74%)`	⬇️

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.

yudjinn · 2025-03-07T14:54:13Z

Nice! So for a given commit range of the containing repo, this returns all submodules that changed within those commits, and their deltas between them?

yudjinn · 2025-03-07T14:56:24Z

This is actually an improvement and "replaces" some of what I was trying to do, and I'm super happy to see it ☺️ if I have time this weekend I'll try to use this as the basis for the releases instead of my other way

lehmanju · 2025-03-07T19:00:11Z

Well, this iteration sort of actually works! However, the submodule path is not correct ...
context.json

I used openstack/openstack for testing btw, that repository does seem like the ultimate stresstest.

lehmanju · 2025-03-07T19:17:20Z

So that is fixed as well, only config option, documentation and code cleanup/review are missing.

orhun · 2025-03-07T19:21:23Z

This is some good stuff. Let me know when this is ready for review :)

lehmanju

not sure about some naming conventions and what should be tested/where. other than that its finished but still needs some testing with a real changelog template.

lehmanju · 2025-03-07T21:42:11Z

git-cliff-core/Cargo.toml

@@ -55,7 +55,7 @@ serde_json = "1.0.137"
 bincode = "2.0.0-rc.3"
 serde_regex = "1.1.0"
 tera = "1.20.0"
-indexmap = { version = "2.7.1", optional = true }
+indexmap = { version = "2.7.1", features = ["serde"] }


this isn't optional anymore since Release uses it for submodule commits. maybe use hashmap instead? my reasoning was that its used in Repository as well, so keep it consistent

If we use hashmap then the items are not ordered which is probably not something that we want.

Also why enable the serde feature?

serde is needed now because Release needs to be serializable and as a consequence indexmap .

git-cliff-core/src/changelog.rs

git-cliff-core/src/commit.rs

git-cliff-core/src/release.rs

git-cliff-core/src/repo.rs

git-cliff/src/lib.rs

orhun

Looks nice!

Some things to do:

Add fixture tests to ensure the functionality
Update documentation about the new configuration option

git-cliff-core/src/release.rs

git-cliff-core/src/repo.rs

git-cliff/Cargo.toml

git-cliff/src/lib.rs

orhun · 2025-03-09T19:38:18Z

git-cliff-core/Cargo.toml

@@ -55,7 +55,7 @@ serde_json = "1.0.137"
 bincode = "2.0.0-rc.3"
 serde_regex = "1.1.0"
 tera = "1.20.0"
-indexmap = { version = "2.7.1", optional = true }
+indexmap = { version = "2.7.1", features = ["serde"] }


If we use hashmap then the items are not ordered which is probably not something that we want.

Also why enable the serde feature?

git-cliff-core/src/changelog.rs

lehmanju · 2025-03-11T20:04:27Z

Thanks for the review! I'll have time to address your comments in about a week.

Co-authored-by: Orhun Parmaksız <[email protected]>

lehmanju

Did a bunch of fixes:

added fixture test
fixed first/last commit bug (submodule commit range now correct)
fixed commit preprocessing for submodule commits

@orhun would appreciate another review.

lehmanju · 2025-03-17T19:06:16Z

git-cliff/src/lib.rs

+		.commit_id
+		.clone()
+		.and_then(|commit_id| repository.find_commit(&commit_id));
+	let commit_range = first_commit.zip(last_commit);


This commit range generation was initially wrong, I needed to use the tag commit id from the previous and current release.

lehmanju · 2025-03-17T19:06:58Z

git-cliff/src/lib.rs

@@ -300,6 +355,9 @@ fn process_repository<'a>(
 			previous_release.previous = None;
 			release.previous = Some(Box::new(previous_release));
 			previous_release = release.clone();
+			if recurse_submodules {


Because previous release is needed, I moved this to the bottom of the for loop. Else it wouldn't be initialized.

lehmanju · 2025-03-17T19:07:52Z

git-cliff-core/src/release.rs

+	///
+	/// Maps submodule path to a list of commits.
+	#[serde(rename = "submodule_commits")]
+	pub submodule_commits: Option<HashMap<String, Vec<Commit<'a>>>>,


Changed to Hashmap because indexmap serde implementation doesn't preserve order anyway and order doesn't matter.

removed the Option<> wrapping and used an empty hashmap instead. results in a cleaner API during templating.

lehmanju · 2025-03-17T19:08:44Z

git-cliff-core/src/changelog.rs

@@ -186,6 +145,58 @@ impl<'a> Changelog<'a> {
 		Ok(())
 	}

+	fn process_commit_list(


This is new, because commit preprocessing now needs to be done for release commits AND submodule commits.

lehmanju · 2025-03-17T20:56:57Z

There does seem to be something wrong with the fixture tests in general: https://github.com/orhun/git-cliff/actions/runs/13907737767/job/38914649566#step:3:913

lehmanju · 2025-03-19T15:46:02Z

The more I test this in a real scenario the more corner cases I find:
unreleased and first release don't contain any submodule commits although they should

orhun

Looking good! Left some comments that I believe will make the implementation/documentation better.

The fixtures should be fixed in #1099 - ignore the ones that are failing for now.

orhun · 2025-03-19T20:16:04Z

.github/fixtures/test-submodules/cliff.toml

+{% for submodule_path, commits in submodule_commits %}
+    ### {{ submodule_path | upper_first }}
+    {% for group, commits in commits | group_by(attribute="group") %}
+        #### {{ group | upper_first }}
+        {% for commit in commits %}
+            - {{ commit.message | upper_first }}\
+        {% endfor %}
+    {% endfor %}
+{% endfor %}\n


Nice! Can you create a new documentation section (Usage > Submodules) and mention this snippet along with the configuration option? I think doing that would help with the visibility of this new template value.

orhun · 2025-03-19T20:22:56Z

.github/fixtures/test-submodules/commit.sh

This would be more readable if you can add newlines between submodule commits, etc. And maybe consider using pushd & popd

orhun · 2025-03-19T20:24:24Z

website/docs/templating/context.md

@@ -64,6 +64,15 @@ following context is generated to use for templating:
  "commit_id": "a440c6eb26404be4877b7e3ad592bfaa5d4eb210 (release commit)",
  "timestamp": 1625169301,
  "repository": "/path/to/repository",
+  "submodule_commits": {
+    "<relative/submodule_path/in/repository>": [


Have you checked how this path is affected e.g. when running inside another directory in a monorepo?

orhun · 2025-03-19T20:24:48Z

website/docs/configuration/git.md

+
+### recurse_submodules
+
+`recurse_submodules` is an **optional** boolean value that indicates whether git-cliff should read and process commits of submodules. This only considers submodules at the toplevel (depth 1). These commits can then be accessed by the variable `submodule_commits` during [templating](/docs/templating/context).


Suggested change

`recurse_submodules` is an **optional** boolean value that indicates whether git-cliff should read and process commits of submodules. This only considers submodules at the toplevel (depth 1). These commits can then be accessed by the variable `submodule_commits` during [templating](/docs/templating/context).

`recurse_submodules` is an _optional_ boolean value that indicates whether **git-cliff** should read and process commits of submodules. This only considers submodules at the toplevel (depth 1). These commits can then be accessed by the variable `submodule_commits` during [templating](/docs/templating/context).

orhun · 2025-03-19T20:25:43Z

git-cliff/src/lib.rs

+					.map(|commits| commits.iter().map(Commit::from).collect());
+
+				let submodule_path = sub_repo.path().to_string_lossy().into_owned();
+				// (submodule_path, Vec<Commit>)


Suggested change

// (submodule_path, Vec<Commit>)

// Returns a (submodule_path, Vec<Commit>)

orhun · 2025-03-19T20:26:08Z

git-cliff-core/src/repo.rs

+	/// Returns submodule repositories for a given commit range.
+	///
+	/// Returns submodule repositories for a given commit range.


Suggested change

/// Returns submodule repositories for a given commit range.

///

/// Returns submodule repositories for a given commit range.

/// Returns submodule repositories for a given commit range.

orhun · 2025-03-19T20:27:22Z

git-cliff-core/src/repo.rs

+					.zip(Some(range))
+			}
+		});
+		// (repository, commit_range)


Not sure if these comments are useful. You can also derive these types via your LSP most of the time. Maybe remove them or add a sentence for explanation, if necessary.

orhun · 2025-03-19T20:27:58Z

git-cliff-core/src/repo.rs

@@ -46,6 +49,15 @@ pub struct Repository {
 	changed_files_cache_path: PathBuf,
 }

+/// Range of commits in a submodule.
+pub struct SubmoduleRange {


Derive Debug etc. if possible.

orhun · 2025-03-19T20:28:21Z

git-cliff-core/src/changelog.rs

+		debug!("Processing the commits...");
+		for release in self.releases.iter_mut() {
+			Self::process_commit_list(&mut release.commits, &self.config.git)?;
+			for submodule_commit_vec in release.submodule_commits.values_mut() {


Suggested change

for submodule_commit_vec in release.submodule_commits.values_mut() {

for submodule_commits in release.submodule_commits.values_mut() {

lehmanju force-pushed the submodules-2 branch from d13315a to 1939b58 Compare March 7, 2025 09:31

lehmanju force-pushed the submodules-2 branch from 1939b58 to aba15af Compare March 7, 2025 09:44

lehmanju commented Mar 7, 2025

View reviewed changes

lehmanju changed the title ~~feat(repo): get submodule repositories with commit ranges~~ feat: Submodule commit processing Mar 7, 2025

lehmanju marked this pull request as ready for review March 7, 2025 21:48

lehmanju requested a review from orhun as a code owner March 7, 2025 21:48

lehmanju commented Mar 8, 2025

View reviewed changes

git-cliff/src/lib.rs Outdated Show resolved Hide resolved

orhun reviewed Mar 9, 2025

View reviewed changes

lehmanju and others added 14 commits March 17, 2025 18:55

feat(repo): get submodule repositories with commit ranges

2d86ab6

more submodule work on releases

9b136a9

its working, yay

9320311

fix tests and other stuff

f51d465

rename some stuff and add if gate to submodule processing

094adce

format

ef5c006

update cargo.toml

feabec9

Update git-cliff-core/src/repo.rs

d92a4f0

Co-authored-by: Orhun Parmaksız <[email protected]>

add submodule_commits comment

1ad06e4

rename path fn

1ca62e4

revert formatting

229e007

more doc

4db1481

format

5156b85

rename serde submodule_commits

777d3cb

lehmanju added 4 commits March 17, 2025 19:02

use hashmap, do preprocessing

88dc46e

revert cargo.toml changes

66f19cf

initial submodule test fixture

e816ec9

fix first/last commit

5d2dda1

lehmanju force-pushed the submodules-2 branch from 8e5f3a8 to 5d2dda1 Compare March 17, 2025 19:03

lehmanju commented Mar 17, 2025

View reviewed changes

lehmanju added 5 commits March 18, 2025 07:54

fix syntax error in action

c318056

removed option from submodule commits -> cleaner templating

7a1f23f

add doc about submodules

2d9773b

fix integration test

8d4c04b

revert action changes

9e8b649

orhun reviewed Mar 19, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Submodule commit processing #1082

feat: Submodule commit processing #1082

lehmanju commented Mar 7, 2025

welcome bot commented Mar 7, 2025

codecov-commenter commented Mar 7, 2025 •

edited

Loading

yudjinn commented Mar 7, 2025

yudjinn commented Mar 7, 2025 •

edited

Loading

lehmanju commented Mar 7, 2025

lehmanju commented Mar 7, 2025

orhun commented Mar 7, 2025

lehmanju left a comment •

edited

Loading

lehmanju Mar 7, 2025

orhun Mar 9, 2025

lehmanju Mar 17, 2025

orhun left a comment

orhun Mar 9, 2025

lehmanju commented Mar 11, 2025

lehmanju left a comment

lehmanju Mar 17, 2025

lehmanju Mar 17, 2025

lehmanju Mar 17, 2025

lehmanju Mar 18, 2025

lehmanju Mar 17, 2025

lehmanju commented Mar 17, 2025

lehmanju commented Mar 19, 2025

orhun left a comment

orhun Mar 19, 2025

orhun Mar 19, 2025

orhun Mar 19, 2025

orhun Mar 19, 2025

orhun Mar 19, 2025

orhun Mar 19, 2025

orhun Mar 19, 2025

orhun Mar 19, 2025

orhun Mar 19, 2025


		### recurse_submodules

		`recurse_submodules` is an optional boolean value that indicates whether git-cliff should read and process commits of submodules. This only considers submodules at the toplevel (depth 1). These commits can then be accessed by the variable `submodule_commits` during [templating](/docs/templating/context).

	`recurse_submodules` is an optional boolean value that indicates whether git-cliff should read and process commits of submodules. This only considers submodules at the toplevel (depth 1). These commits can then be accessed by the variable `submodule_commits` during [templating](/docs/templating/context).
	`recurse_submodules` is an _optional_ boolean value that indicates whether git-cliff should read and process commits of submodules. This only considers submodules at the toplevel (depth 1). These commits can then be accessed by the variable `submodule_commits` during [templating](/docs/templating/context).

	// (submodule_path, Vec<Commit>)
	// Returns a (submodule_path, Vec<Commit>)

	for submodule_commit_vec in release.submodule_commits.values_mut() {
	for submodule_commits in release.submodule_commits.values_mut() {

feat: Submodule commit processing #1082

Are you sure you want to change the base?

feat: Submodule commit processing #1082

Conversation

lehmanju commented Mar 7, 2025

welcome bot commented Mar 7, 2025

codecov-commenter commented Mar 7, 2025 • edited Loading

Codecov Report

yudjinn commented Mar 7, 2025

yudjinn commented Mar 7, 2025 • edited Loading

lehmanju commented Mar 7, 2025

lehmanju commented Mar 7, 2025

orhun commented Mar 7, 2025

lehmanju left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orhun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lehmanju commented Mar 11, 2025

lehmanju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lehmanju commented Mar 17, 2025

lehmanju commented Mar 19, 2025

orhun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 7, 2025 •

edited

Loading

yudjinn commented Mar 7, 2025 •

edited

Loading

lehmanju left a comment •

edited

Loading