Submodule recursion #1058

yudjinn · 2025-02-26T01:34:39Z

Description

Motivation and Context

How Has This Been Tested?

Screenshots / Logs (if applicable)

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.

welcome · 2025-02-26T01:34:43Z

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

orhun

Good stuff, I left some comments that I think help moving further.

Tested it quickly, but didn't see this in action yet. Can you also check again?

orhun · 2025-02-27T20:34:57Z

git-cliff-core/src/commit.rs

@@ -250,6 +252,29 @@ impl Commit<'_> {
 		}
 	}

+	/// Returns wether the commit changes the SHA of a submodule


Suggested change

/// Returns wether the commit changes the SHA of a submodule

/// Returns whether the commit changes the SHA of a submodule

orhun · 2025-02-27T20:37:25Z

git-cliff/src/lib.rs

-			}
-		}
-	}
+	parse_commits(args, &tags, &mut commit_range, repository)?;


Why this has been extracted to a new function?

interim derp, thought I might reuse it but I dont need to 😓

orhun · 2025-02-27T20:40:56Z

git-cliff/src/lib.rs

@@ -303,6 +279,36 @@ fn process_repository<'a>(
 			releases.push(Release::default());
 		}
 	}
+	if recurse_submodules {


I guess we are updating the releases vector here based on the commits in the submodules.

It would be nice to extract this to a function that takes &mut releases maybe.

orhun · 2025-02-27T20:46:37Z

git-cliff/src/lib.rs

+	if recurse_submodules {
+		for (_submodule, _commits) in submodule_commits.values() {
+			log::trace!("Recursing {:?}.", _submodule.path().to_str());
+			let _range = format!(


This doesn't seem to be used.

orhun · 2025-02-27T20:51:23Z

git-cliff/src/lib.rs

+				_commits.last().unwrap().id
+			);
+			let _repo_path = _submodule.path().to_path_buf();
+			let _repo = Repository::init(_repo_path.clone())?;


You can get rid of the compiler error by doing this for now:

let repo = Box::leak(Box::new(Repository::init(repo_path.clone())?));

It's a bit bad, but this is also what I'm doing for the other repositories 💀

lehmanju · 2025-02-28T11:49:48Z

Oh yes, great, that's a feature I am very interested in. I volunteer for testing, we use this at work for changelog generation with about 200 submodules. Right now we use the repository flag to generate a merged changelog for each submodule.
However, version 2.8 did somehow break it so I am looking forward to true submodule support.

There is one particular issue on CI environments I ran into. If you don't specify fetch-depth: 0 in Github Actions checkout then the submodule history is completely missing and it only contains the last commit. Maybe there is room for further improvement to fetch the necessary submodule commits on demand without fetching the complete history/refs.

orhun · 2025-02-28T17:19:38Z

It's great to hear there is some interest for this feature :)

However, version 2.8 did somehow break it so I am looking forward to true submodule support.

Ah, it's probably related to the monorepo related changes. Would love to take a look into it if you have time to submit an issue about this. But I agree that proper submodule support would be indeed better here.

Maybe there is room for further improvement to fetch the necessary submodule commits on demand without fetching the complete history/refs.

Interesting idea...

lehmanju · 2025-03-03T15:42:33Z

Couldn't help myself and started tinkering with this code ;). Here are some thoughts:

the idea does seem to be:
- iterate over all commits and save commits that change a submodule
- gather all submodule commits for these toplevel commits and append them to a release
this does not work because:
- only toplevel commit ids are gathered, but to iterate through submodule commits submodule commit ids need to be known. this requires reading the diff content, maybe commit_changed_submodules_no_cache .
- instead of doing these two steps separately, recursive commit explosion can be done directly while iterating through toplevel commits. submodule commits are then just appended to the same release tag/repository with an additional submodule value (optional)
- a template can then process a release grouping/filtering commits based on submodule.

orhun · 2025-03-03T22:58:45Z

Interesting... I didn't take a deeper look into this code but your take makes sense. I think we can wait for @yudjinn for incorporate some changes into this PR or feel free to put up a new draft based on those.

yudjinn · 2025-03-03T23:34:14Z

I poked at it some more, but in short, @lehmanju is correct that we do need to get submodule changes as well. I am currently planning on doing that as a Vec based on the new/old filename that the commit sees, and then just diffing releases based on the first and last in that vec, but if you have a better idea to do that explosion earlier in one shot, I'd love to see it!

lehmanju · 2025-03-04T07:09:30Z

I'd probably explode the commits between submodule before and after states somewhere in:

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

Lines 116 to 150 in 63129ce

    
           	pub fn commits( 
        
           		&self, 
        
           		range: Option<&str>, 
        
           		include_path: Option<Vec<Pattern>>, 
        
           		exclude_path: Option<Vec<Pattern>>, 
        
           	) -> Result<Vec<Commit>> { 
        
           		let mut revwalk = self.inner.revwalk()?; 
        
           		revwalk.set_sorting(Sort::TOPOLOGICAL)?; 
        
           		Self::set_commit_range(&mut revwalk, range).map_err(|e| { 
        
           			Error::SetCommitRangeError( 
        
           				range.map(String::from).unwrap_or_else(|| "?".to_string()), 
        
           				e, 
        
           			) 
        
           		})?; 
        
           		let mut commits: Vec<Commit> = revwalk 
        
           			.filter_map(|id| id.ok()) 
        
           			.filter_map(|id| self.inner.find_commit(id).ok()) 
        
           			.collect(); 
        
           		if include_path.is_some() || exclude_path.is_some() { 
        
           			let include_patterns = include_path.map(|patterns| { 
        
           				patterns.into_iter().map(Self::normalize_pattern).collect() 
        
           			}); 
        
           			let exclude_patterns = exclude_path.map(|patterns| { 
        
           				patterns.into_iter().map(Self::normalize_pattern).collect() 
        
           			}); 
        
           			commits.retain(|commit| { 
        
           				self.should_retain_commit( 
        
           					commit, 
        
           					&include_patterns, 
        
           					&exclude_patterns, 
        
           				) 
        
           			}); 
        
           		} 
        
           		Ok(commits) 
        
           	}

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

Lines 275 to 285 in 63129ce

    
           if let Ok(diff) = self.inner.diff_tree_to_tree( 
        
           	commit.tree().ok().as_ref(), 
        
           	prev_commit.tree().ok().as_ref(), 
        
           	None, 
        
           ) { 
        
           	changed_files.extend( 
        
           		diff.deltas().filter_map(|delta| { 
        
           			delta.new_file().path().map(PathBuf::from) 
        
           		}), 
        
           	); 
        
           }

And since this line already iterates through the diff, I'd extend it to look into the file contents to get the submodule commit id.

However, I'm stuck at how to ask libgit2 to get me the file contents if I have a diff?
DiffFile doesn't have a getter to get the contents.

orhun · 2025-03-05T14:18:35Z

And since this line already iterates through the diff, I'd extend it to look into the file contents to get the submodule commit id.
However, I'm stuck at how to ask libgit2 to get me the file contents if I have a diff?

Not sure how it would be possible to get the submodule information from a Diff but it is possible to get the file contents via calling Diff::print. Here you can also filter by DiffDelta (and DiffFile).

Here is an example from git2: https://github.com/rust-lang/git2-rs/blob/d1ae3b6c2d1200e7d82468af447fa66259225ecf/examples/log.rs#L201-L209

orhun · 2025-03-05T14:27:27Z

git-cliff/src/lib.rs

@@ -273,10 +281,34 @@ fn process_repository<'a>(
 	let mut previous_release = Release::default();
 	let mut first_processed_tag = None;
 	let repository_path = repository.path()?.to_string_lossy().into_owned();
+	let mut submodule_commits: HashMap<PathBuf, (&mut Repository, Vec<Commit>)> =


Why do we have this complex type? 🤔

So we don't have to reinit The repo every time we want to parse commits, but tbh I put some of that together after a very full day so I plan on redoing it a bit. The pathbuf being the key is just to avoid having to impl comparisons for submodules

Got it. it would be nice to refactor/simplify this a bit for sure :)

I think I might just swap it to be a tuple and replace the end commit instead. Plan is that for any given submodule, the releases we include should be inclusive and in aggregate. So if a single release has multiple commits bumping the submodule underneath it, just aggregate all of those changes

yudjinn · 2025-03-05T15:56:08Z

I'd probably explode the commits between submodule before and after states somewhere in:

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

Lines 116 to 150 in 63129ce

pub fn commits(

&self,

range: Option<&str>,

include_path: Option<Vec<Pattern>>,

exclude_path: Option<Vec<Pattern>>,

) -> Result<Vec<Commit>> {

let mut revwalk = self.inner.revwalk()?;

revwalk.set_sorting(Sort::TOPOLOGICAL)?;

Self::set_commit_range(&mut revwalk, range).map_err(|e| {

Error::SetCommitRangeError(

range.map(String::from).unwrap_or_else(|| "?".to_string()),

e,

)

})?;

let mut commits: Vec<Commit> = revwalk

.filter_map(|id| id.ok())

.filter_map(|id| self.inner.find_commit(id).ok())

.collect();

if include_path.is_some() || exclude_path.is_some() {

let include_patterns = include_path.map(|patterns| {

patterns.into_iter().map(Self::normalize_pattern).collect()

});

let exclude_patterns = exclude_path.map(|patterns| {

patterns.into_iter().map(Self::normalize_pattern).collect()

});

commits.retain(|commit| {

self.should_retain_commit(

commit,

&include_patterns,

&exclude_patterns,

)

});

}

Ok(commits)

}

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

Lines 275 to 285 in 63129ce

if let Ok(diff) = self.inner.diff_tree_to_tree(

commit.tree().ok().as_ref(),

prev_commit.tree().ok().as_ref(),

None,

) {

changed_files.extend(

diff.deltas().filter_map(|delta| {

delta.new_file().path().map(PathBuf::from)

}),

);

}

And since this line already iterates through the diff, I'd extend it to look into the file contents to get the submodule commit id.
However, I'm stuck at how to ask libgit2 to get me the file contents if I have a diff? DiffFile doesn't have a getter to get the contents.

the only way I was able to do it was checking the old_path and new_path against the paths from repo.submodules()

yudjinn added 3 commits February 24, 2025 10:47

testing submodule pt1

92e3924

update submodule

8b95ed2

first pass submodule usage

a807821

yudjinn requested a review from orhun as a code owner February 26, 2025 01:34

orhun reviewed Feb 27, 2025

View reviewed changes

wip submodule commit parsing

8f26221

orhun reviewed Mar 5, 2025

View reviewed changes

orhun changed the title ~~DRAFT: submodule recursion~~ Support submodule recursion Mar 5, 2025

orhun marked this pull request as draft March 5, 2025 14:27

orhun changed the title ~~Support submodule recursion~~ Submodule recursion Mar 5, 2025

lehmanju mentioned this pull request Mar 7, 2025

feat: Submodule commit processing #1082

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Submodule recursion #1058

Submodule recursion #1058

yudjinn commented Feb 26, 2025

welcome bot commented Feb 26, 2025

orhun left a comment

orhun Feb 27, 2025

orhun Feb 27, 2025

yudjinn Feb 27, 2025

orhun Feb 27, 2025

orhun Feb 27, 2025

orhun Feb 27, 2025

lehmanju commented Feb 28, 2025

orhun commented Feb 28, 2025

lehmanju commented Mar 3, 2025

orhun commented Mar 3, 2025

yudjinn commented Mar 3, 2025 •

edited

Loading

lehmanju commented Mar 4, 2025

orhun commented Mar 5, 2025

orhun Mar 5, 2025

yudjinn Mar 5, 2025

orhun Mar 5, 2025

yudjinn Mar 5, 2025

yudjinn commented Mar 5, 2025

	/// Returns wether the commit changes the SHA of a submodule
	/// Returns whether the commit changes the SHA of a submodule

Submodule recursion #1058

Are you sure you want to change the base?

Submodule recursion #1058

Conversation

yudjinn commented Feb 26, 2025

Description

Motivation and Context

How Has This Been Tested?

Screenshots / Logs (if applicable)

Types of Changes

Checklist:

welcome bot commented Feb 26, 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

lehmanju commented Feb 28, 2025

orhun commented Feb 28, 2025

lehmanju commented Mar 3, 2025

orhun commented Mar 3, 2025

yudjinn commented Mar 3, 2025 • edited Loading

lehmanju commented Mar 4, 2025

orhun commented Mar 5, 2025

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

yudjinn commented Mar 5, 2025

yudjinn commented Mar 3, 2025 •

edited

Loading