Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Submodule recursion #1058

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "sources/ratatui"]
path = sources/ratatui
url = [email protected]:ratatui/ratatui.git
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions cliff.toml
Original file line number Diff line number Diff line change
Expand Up @@ -124,3 +124,5 @@ ignore_tags = "rc|v2.1.0|v2.1.1"
topo_order = false
# sort the commits inside sections by oldest/newest order
sort_commits = "newest"

recurse_submodules = true
1 change: 1 addition & 0 deletions git-cliff-core/src/changelog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,7 @@ mod test {
)),
replace_command: None,
}]),
recurse_submodules: Some(false),
commit_parsers: Some(vec![
CommitParser {
sha: Some(String::from("tea")),
Expand Down
2 changes: 1 addition & 1 deletion git-cliff-core/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ pub fn run(

#[cfg(test)]
mod test {
use super::*;


#[test]
#[cfg(target_family = "unix")]
Expand Down
62 changes: 62 additions & 0 deletions git-cliff-core/src/commit.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
use std::collections::HashMap;
use std::path::PathBuf;

use crate::config::{
CommitParser,
GitConfig,
Expand All @@ -8,6 +11,8 @@ use crate::error::{
Error as AppError,
Result,
};
use crate::repo::Repository;
use git2::Submodule;
#[cfg(feature = "repo")]
use git2::{
Commit as GitCommit,
Expand Down Expand Up @@ -106,6 +111,8 @@ pub struct Commit<'a> {
pub id: String,
/// Commit message including title, description and summary.
pub message: String,
/// Parent commit ID
pub parent: String,
/// Conventional commit.
#[serde(skip_deserializing)]
pub conv: Option<ConventionalCommit<'a>>,
Expand Down Expand Up @@ -179,12 +186,17 @@ impl From<String> for Commit<'_> {
#[cfg(feature = "repo")]
impl<'a> From<&GitCommit<'a>> for Commit<'a> {
fn from(commit: &GitCommit<'a>) -> Self {
let parent = match commit.parent_id(0) {
Ok(id) => id.to_string(),
Err(_) => String::new(),
};
Commit {
id: commit.id().to_string(),
message: commit.message().unwrap_or_default().trim_end().to_string(),
author: commit.author().into(),
committer: commit.committer().into(),
merge_commit: commit.parent_count() > 1,
parent,
..Default::default()
}
}
Expand Down Expand Up @@ -250,6 +262,56 @@ impl Commit<'_> {
}
}

/// Returns whether the commit changes the SHA of a submodule
pub fn get_submodule_updates<'a>(
&self,
repo: &'a Repository,
) -> Option<HashMap<PathBuf, (&mut &Submodule<'a>, git2::Oid, git2::Oid)>> {
let parent = match repo.commits(Some(&self.parent), None, None) {
Ok(it) => it,
Err(_) => {
return None;
}
};
let commit = match repo.commits(Some(&self.id), None, None) {
Ok(it) => it,
Err(_) => {
return None;
}
};
// let mut output: HashMap<Submodule, (GitCommit, GitCommit)> =
// HashMap::new();
let mut output = HashMap::new();
// get all submodule changes in repo
let binding = Box::leak(Box::new(repo.submodules().unwrap_or_default()));
let submodules: HashMap<PathBuf, &Submodule> = binding
.iter()
.map(|s| (s.path().to_path_buf(), s))
.collect();
if let Ok(diff) = repo.inner.diff_tree_to_tree(
Some(&parent[0].tree().expect("Parent commit has no tree.")),
Some(&commit[0].tree().expect("Commit has no tree.")),
None,
) {
for d in diff.deltas() {
if let Some(p) = d.old_file().path() {
if let Some(s) = submodules.get(&p.to_path_buf()) {
// if the path change matches the submodule path, add it
output.insert(
p.to_path_buf(),
(
Box::leak(Box::new(*s)),
d.old_file().id(),
d.new_file().id(),
),
);
}
}
}
}
Some(output)
}

/// Preprocesses the commit using [`TextProcessor`]s.
///
/// Modifies the commit [`message`] using regex or custom OS command.
Expand Down
2 changes: 2 additions & 0 deletions git-cliff-core/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ pub struct GitConfig {
pub sort_commits: Option<String>,
/// Limit the number of commits included in the changelog.
pub limit_commits: Option<usize>,
/// Recurse submodules for changes
pub recurse_submodules: Option<bool>,
}

/// Remote configuration.
Expand Down
15 changes: 13 additions & 2 deletions git-cliff-core/src/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::error::{
Result,
};
use crate::tag::Tag;
pub use git2::Submodule as GitSubmodule;
use git2::{
BranchType,
Commit,
Expand Down Expand Up @@ -39,7 +40,7 @@ const CHANGED_FILES_CACHE: &str = "changed_files_cache";
///
/// [`Repository`]: GitRepository
pub struct Repository {
inner: GitRepository,
pub inner: GitRepository,
/// Repository path.
path: PathBuf,
/// Cache path for the changed files of the commits.
Expand Down Expand Up @@ -265,7 +266,7 @@ impl Repository {
/// Calculate the changed files of the commit.
///
/// This function does not use the cache (directly calls git2).
fn commit_changed_files_no_cache(&self, commit: &Commit) -> Vec<PathBuf> {
pub fn commit_changed_files_no_cache(&self, commit: &Commit) -> Vec<PathBuf> {
let mut changed_files = Vec::new();
if let Ok(prev_commit) = commit.parent(0) {
// Compare the current commit with the previous commit to get the
Expand All @@ -277,6 +278,9 @@ impl Repository {
prev_commit.tree().ok().as_ref(),
None,
) {
for d in diff.deltas() {
println!("{:?}", d);
}
changed_files.extend(
diff.deltas().filter_map(|delta| {
delta.new_file().path().map(PathBuf::from)
Expand Down Expand Up @@ -473,6 +477,13 @@ impl Repository {
"no remotes configured or HEAD is detached",
)))
}

/// Load all submodules for this repository and return them.
pub fn submodules(&self) -> Result<Vec<GitSubmodule<'_>>> {
self.inner
.submodules()
.map_err(|e| Error::RepoError(String::from(e.message())))
}
}

fn find_remote(url: &str) -> Result<Remote> {
Expand Down
1 change: 1 addition & 0 deletions git-cliff-core/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ fn generate_changelog() -> Result<()> {
replace: Some(String::from("[closes Issue${1}]")),
replace_command: None,
}]),
recurse_submodules: Some(false),
commit_parsers: Some(vec![
CommitParser {
sha: Some(String::from("coffee")),
Expand Down
3 changes: 2 additions & 1 deletion git-cliff/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ path = "src/bin/mangen.rs"

[features]
# check for new versions
default = ["update-informer", "github", "gitlab", "gitea", "bitbucket"]
default = ["update-informer", "github", "gitlab", "gitea", "bitbucket", "dep:indexmap"]
# inform about new releases
update-informer = ["dep:update-informer"]
# enable remote repository integration
Expand Down Expand Up @@ -53,6 +53,7 @@ clap_mangen = "0.2.26"
shellexpand = "3.1.0"
update-informer = { version = "1.2.0", optional = true }
indicatif = { version = "0.17.9", optional = true }
indexmap = { version = "2.7.1", optional = true }
env_logger = "=0.10.2"
pprof = { version = "0.14", optional = true }
rand = { version = "0.8.4", optional = true }
Expand Down
79 changes: 77 additions & 2 deletions git-cliff/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,17 @@ use git_cliff_core::error::{
Result,
};
use git_cliff_core::release::Release;
use git_cliff_core::repo::Repository;
use git_cliff_core::repo::{
GitSubmodule,
Repository,
};
use git_cliff_core::{
DEFAULT_CONFIG,
IGNORE_FILE,
};
use glob::Pattern;
use std::borrow::BorrowMut;
use std::collections::HashMap;
use std::env;
use std::fs::{
self,
Expand Down Expand Up @@ -95,6 +100,8 @@ fn process_repository<'a>(
let skip_regex = config.git.skip_tags.as_ref();
let ignore_regex = config.git.ignore_tags.as_ref();
let count_tags = config.git.count_tags.as_ref();
let recurse_submodules =
*config.git.recurse_submodules.as_ref().unwrap_or(&false);
tags.retain(|_, tag| {
let name = &tag.name;

Expand Down Expand Up @@ -167,6 +174,7 @@ fn process_repository<'a>(

// Parse commits.
let mut commit_range = args.range.clone();

if args.unreleased {
if let Some(last_tag) = tags.last().map(|(k, _)| k) {
commit_range = Some(format!("{last_tag}..HEAD"));
Expand Down Expand Up @@ -216,7 +224,7 @@ fn process_repository<'a>(
commit_range = Some(format!("{tag1}..{tag2}"));
}
}
}
};

// Include only the current directory if not running from the root repository
let mut include_path = args.include_path.clone();
Expand Down Expand Up @@ -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>)> =
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have this complex type? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

HashMap::new();
for git_commit in commits.iter().rev() {
let release = releases.last_mut().unwrap();
let commit = Commit::from(git_commit);
let commit_id = commit.id.to_string();
if recurse_submodules {
if let Some(submodule_deltas) = commit.get_submodule_updates(repository)
{
for (path, (_submodule, old, new)) in submodule_deltas {
submodule_commits
.entry(path.clone())
.and_modify(|e| {
let c = e.0.find_commit(&new.to_string()).unwrap();
e.1.push(Commit::from(&c));
})
.or_insert_with(|| {
let _repo = Box::leak(Box::new(
Repository::init(path.clone()).unwrap(),
));
let v = vec![Commit::from(
&_repo.find_commit(&old.to_string()).unwrap(),
)];
(_repo.borrow_mut(), v)
});
}
}
}
release.commits.push(commit);
release.repository = Some(repository_path.clone());
if let Some(tag) = tags.get(&commit_id) {
Expand All @@ -303,6 +335,14 @@ fn process_repository<'a>(
releases.push(Release::default());
}
}
if recurse_submodules {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

process_submodule_releases(
submodule_commits,
config,
&mut releases,
&mut previous_release,
)?;
}

debug_assert!(!releases.is_empty());

Expand Down Expand Up @@ -374,6 +414,41 @@ fn process_repository<'a>(
Ok(releases)
}

// Loop through submodule commit changes and create release
fn process_submodule_releases<'a>(
submodule_commits: HashMap<PathBuf, (&'a mut Repository, Vec<Commit>)>,
config: &mut Config,
releases: &mut Vec<Release<'a>>,
previous_release: &mut Release<'a>,
) -> Result<()> {
for (path, (submodule, commits)) in submodule_commits {
log::trace!("Recursing {:?}.", submodule.path());
let commit_range = format!(
"{}..{}",
commits.first().unwrap().id,
commits.last().unwrap().id
);
let mut _internal_commits =
submodule.commits(Some(&commit_range), None, None)?;
if let Some(commit_limit_value) = config.git.limit_commits {
_internal_commits.truncate(commit_limit_value);
}
for _commit in _internal_commits.iter() {
let release = releases.last_mut().unwrap();
let commit = Commit::from(_commit);
let commit_id = commit.id.to_string();
release.commits.push(commit);
release.repository = Some(path.to_string_lossy().into_owned());
release.commit_id = Some(commit_id);
previous_release.previous = None;
release.previous = Some(Box::new(previous_release.clone()));
*previous_release = release.clone();
releases.push(Release::default());
}
}
Ok(())
}

/// Runs `git-cliff`.
///
/// # Example
Expand Down
1 change: 1 addition & 0 deletions sources/ratatui
Submodule ratatui added at 5aca11
Loading