refactor(lib): clean up some code (#709)

DaniPopes · orhun · web-flow · commit 4b0c0eb09abf · 2024-08-04T00:05:26.000+03:00
* refactor: clean up some code

* Update git-cliff/src/lib.rs

Co-authored-by: Orhun Parmaksız &lt;orhunparmaksiz@gmail.com&gt;

* review

* fmt

* fix: read before opening the file to prepend

* style: update formatting

---------

Co-authored-by: Orhun Parmaksız &lt;orhunparmaksiz@gmail.com&gt;
diff --git a/git-cliff-core/src/changelog.rs b/git-cliff-core/src/changelog.rs
@@ -499,7 +499,7 @@ impl<'a> Changelog<'a> {
 	}
 
 	/// Generates the changelog and writes it to the given output.
-	pub fn generate<W: Write>(&self, out: &mut W) -> Result<()> {
+	pub fn generate<W: Write + ?Sized>(&self, out: &mut W) -> Result<()> {
 		debug!("Generating changelog...");
 		let postprocessors = self
 			.config
@@ -567,7 +567,7 @@ impl<'a> Changelog<'a> {
 	}
 
 	/// Generates a changelog and prepends it to the given changelog.
-	pub fn prepend<W: Write>(
+	pub fn prepend<W: Write + ?Sized>(
 		&self,
 		mut changelog: String,
 		out: &mut W,
@@ -582,7 +582,7 @@ impl<'a> Changelog<'a> {
 	}
 
 	/// Prints the changelog context to the given output.
-	pub fn write_context<W: Write>(&self, out: &mut W) -> Result<()> {
+	pub fn write_context<W: Write + ?Sized>(&self, out: &mut W) -> Result<()> {
 		let output = Releases {
 			releases: &self.releases,
 		}
diff --git a/git-cliff-core/src/repo.rs b/git-cliff-core/src/repo.rs
@@ -59,14 +59,14 @@ impl Repository {
 	/// Sorts the commits by their time.
 	pub fn commits(
 		&self,
-		range: Option<String>,
-		include_path: Option<Vec<Pattern>>,
-		exclude_path: Option<Vec<Pattern>>,
+		range: Option<&str>,
+		include_path: Option<&[Pattern]>,
+		exclude_path: Option<&[Pattern]>,
 	) -> Result<Vec<Commit>> {
 		let mut revwalk = self.inner.revwalk()?;
 		revwalk.set_sorting(Sort::TOPOLOGICAL)?;
 		if let Some(range) = range {
-			revwalk.push_range(&range)?;
+			revwalk.push_range(range)?;
 		} else {
 			revwalk.push_head()?;
 		}
@@ -76,31 +76,31 @@ impl Repository {
 			.collect();
 		if include_path.is_some() || exclude_path.is_some() {
 			commits.retain(|commit| {
-				if let Ok(prev_commit) = commit.parent(0) {
-					if let Ok(diff) = self.inner.diff_tree_to_tree(
-						commit.tree().ok().as_ref(),
-						prev_commit.tree().ok().as_ref(),
-						None,
-					) {
-						return diff
-							.deltas()
-							.filter_map(|delta| delta.new_file().path())
-							.any(|new_file_path| {
-								if let Some(include_path) = &include_path {
-									include_path
-										.iter()
-										.any(|glob| glob.matches_path(new_file_path))
-								} else if let Some(exclude_path) = &exclude_path {
-									!exclude_path
-										.iter()
-										.any(|glob| glob.matches_path(new_file_path))
-								} else {
-									false
-								}
-							});
-					}
-				}
-				false
+				let Ok(prev_commit) = commit.parent(0) else {
+					return false;
+				};
+				let Ok(diff) = self.inner.diff_tree_to_tree(
+					commit.tree().ok().as_ref(),
+					prev_commit.tree().ok().as_ref(),
+					None,
+				) else {
+					return false;
+				};
+				diff.deltas()
+					.filter_map(|delta| delta.new_file().path())
+					.any(|new_file_path| {
+						if let Some(include_path) = include_path {
+							return include_path
+								.iter()
+								.any(|glob| glob.matches_path(new_file_path));
+						}
+						if let Some(exclude_path) = exclude_path {
+							return !exclude_path
+								.iter()
+								.any(|glob| glob.matches_path(new_file_path));
+						}
+						unreachable!()
+					})
 			});
 		}
 		Ok(commits)
diff --git a/git-cliff/src/lib.rs b/git-cliff/src/lib.rs
@@ -45,11 +45,8 @@ use std::fs::{
 	self,
 	File,
 };
-use std::io::{
-	self,
-	Write,
-};
-use std::path::PathBuf;
+use std::io;
+use std::path::Path;
 use std::time::{
 	SystemTime,
 	UNIX_EPOCH,
@@ -89,31 +86,28 @@ fn process_repository<'a>(
 	let mut tags = repository.tags(&config.git.tag_pattern, args.topo_order)?;
 	let skip_regex = config.git.skip_tags.as_ref();
 	let ignore_regex = config.git.ignore_tags.as_ref();
-	tags = tags
-		.into_iter()
-		.filter(|(_, tag)| {
-			let name = &tag.name;
-
-			// Keep skip tags to drop commits in the later stage.
-			let skip = skip_regex.map(|r| r.is_match(name)).unwrap_or_default();
-
-			let ignore = ignore_regex
-				.map(|r| {
-					if r.as_str().trim().is_empty() {
-						return false;
-					}
+	tags.retain(|_, tag| {
+		let name = &tag.name;
 
-					let ignore_tag = r.is_match(name);
-					if ignore_tag {
-						trace!("Ignoring release: {}", name)
-					}
-					ignore_tag
-				})
-				.unwrap_or_default();
+		// Keep skip tags to drop commits in the later stage.
+		let skip = skip_regex.is_some_and(|r| r.is_match(name));
+		if skip {
+			return true;
+		}
 
-			skip || !ignore
-		})
-		.collect();
+		let ignore = ignore_regex.is_some_and(|r| {
+			if r.as_str().trim().is_empty() {
+				return false;
+			}
+
+			let ignore_tag = r.is_match(name);
+			if ignore_tag {
+				trace!("Ignoring release: {}", name)
+			}
+			ignore_tag
+		});
+		!ignore
+	});
 
 	if !config.remote.github.is_set() {
 		match repository.upstream_remote() {
@@ -214,14 +208,12 @@ fn process_repository<'a>(
 		}
 	}
 	let mut commits = repository.commits(
-		commit_range,
-		args.include_path.clone(),
-		args.exclude_path.clone(),
+		commit_range.as_deref(),
+		args.include_path.as_deref(),
+		args.exclude_path.as_deref(),
 	)?;
 	if let Some(commit_limit_value) = config.git.limit_commits {
-		commits = commits
-			.drain(..commits.len().min(commit_limit_value))
-			.collect();
+		commits.truncate(commit_limit_value);
 	}
 
 	// Update tags.
@@ -240,27 +232,19 @@ fn process_repository<'a>(
 
 	// Process releases.
 	let mut releases = vec![Release::default()];
-	let mut release_index = 0;
 	let mut previous_release = Release::default();
 	let mut first_processed_tag = None;
 	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();
-		releases[release_index].repository =
-			Some(repository.path.to_string_lossy().to_string());
-		if args.sort == Sort::Newest {
-			releases[release_index].commits.insert(0, commit);
-		} else {
-			releases[release_index].commits.push(commit);
-		}
+		release.commits.push(commit);
+		release.repository = Some(repository.path.to_string_lossy().into_owned());
 		if let Some(tag) = tags.get(&commit_id) {
-			let tag_name = &tag.name;
-
-			releases[release_index].version = Some(tag_name.clone());
-			releases[release_index].message = tag.message.clone();
-			releases[release_index].commit_id = Some(commit_id);
-			releases[release_index].timestamp = if args.tag == Some(tag_name.clone())
-			{
+			release.version = Some(tag.name.to_string());
+			release.message = tag.message.clone();
+			release.commit_id = Some(commit_id);
+			release.timestamp = if args.tag.as_deref() == Some(tag.name.as_str()) {
 				SystemTime::now()
 					.duration_since(UNIX_EPOCH)?
 					.as_secs()
@@ -272,27 +256,32 @@ fn process_repository<'a>(
 				first_processed_tag = Some(tag);
 			}
 			previous_release.previous = None;
-			releases[release_index].previous = Some(Box::new(previous_release));
-			previous_release = releases[release_index].clone();
+			release.previous = Some(Box::new(previous_release));
+			previous_release = release.clone();
 			releases.push(Release::default());
-			release_index += 1;
 		}
 	}
 
-	if release_index > 0 {
+	debug_assert!(!releases.is_empty());
+
+	if releases.len() > 1 {
 		previous_release.previous = None;
-		releases[release_index].previous = Some(Box::new(previous_release));
+		releases.last_mut().unwrap().previous = Some(Box::new(previous_release));
+	}
+
+	if args.sort == Sort::Newest {
+		for release in &mut releases {
+			release.commits.reverse();
+		}
 	}
 
 	// Add custom commit messages to the latest release.
 	if let Some(custom_commits) = &args.with_commit {
-		if let Some(latest_release) = releases.iter_mut().last() {
-			custom_commits.iter().for_each(|message| {
-				latest_release
-					.commits
-					.push(Commit::from(message.to_string()))
-			});
-		}
+		releases
+			.last_mut()
+			.unwrap()
+			.commits
+			.extend(custom_commits.iter().cloned().map(Commit::from));
 	}
 
 	// Set custom message for the latest release.
@@ -303,12 +292,11 @@ fn process_repository<'a>(
 	}
 
 	// Set the previous release if the first release does not have one set.
-	if !releases.is_empty() &&
-		releases
-			.first()
-			.and_then(|r| r.previous.as_ref())
-			.and_then(|p| p.version.as_ref())
-			.is_none()
+	if releases[0]
+		.previous
+		.as_ref()
+		.and_then(|p| p.version.as_ref())
+		.is_none()
 	{
 		// Get the previous tag of the first processed tag in the release loop.
 		let first_tag = first_processed_tag
@@ -553,6 +541,15 @@ pub fn run(mut args: Opt) -> Result<()> {
 	let mut changelog = Changelog::new(releases, &config)?;
 
 	// Print the result.
+	let mut out: Box<dyn io::Write> = if let Some(path) = &args.output {
+		if path == Path::new("-") {
+			Box::new(io::stdout())
+		} else {
+			Box::new(io::BufWriter::new(File::create(path)?))
+		}
+	} else {
+		Box::new(io::stdout())
+	};
 	if args.bump.is_some() || args.bumped_version {
 		let next_version = if let Some(next_version) = changelog.bump_version()? {
 			next_version
@@ -565,40 +562,22 @@ pub fn run(mut args: Opt) -> Result<()> {
 			return Ok(());
 		};
 		if args.bumped_version {
-			if let Some(path) = args.output {
-				let mut output = File::create(path)?;
-				output.write_all(next_version.as_bytes())?;
-			} else {
-				println!("{next_version}");
-			}
+			writeln!(out, "{next_version}")?;
 			return Ok(());
 		}
 	}
 	if args.context {
-		return if let Some(path) = args.output {
-			let mut output = File::create(path)?;
-			changelog.write_context(&mut output)
-		} else {
-			changelog.write_context(&mut io::stdout())
-		};
+		changelog.write_context(&mut out)?;
+		return Ok(());
 	}
-	if let Some(ref path) = args.prepend {
-		changelog.prepend(fs::read_to_string(path)?, &mut File::create(path)?)?;
+	if let Some(path) = &args.prepend {
+		let changelog_before = fs::read_to_string(path)?;
+		let mut out = io::BufWriter::new(File::create(path)?);
+		changelog.prepend(changelog_before, &mut out)?;
 	}
-	if let Some(path) = args.output {
-		let mut output: Box<dyn Write> = if path == PathBuf::from("-") {
-			Box::new(io::stdout())
-		} else {
-			Box::new(File::create(path)?)
-		};
-		if args.context {
-			changelog.write_context(&mut output)
-		} else {
-			changelog.generate(&mut output)
-		}
-	} else if args.prepend.is_none() {
-		changelog.generate(&mut io::stdout())
-	} else {
-		Ok(())
+	if args.output.is_some() || args.prepend.is_none() {
+		changelog.generate(&mut out)?;
 	}
+
+	Ok(())
 }

Original file line number	Diff line number	Diff line change
`@@ -499,7 +499,7 @@ impl<'a> Changelog<'a> {`
`499`	`499`	`}`
`500`	`500`
`501`	`501`	`/// Generates the changelog and writes it to the given output.`
`502`		`- pub fn generate<W: Write>(&self, out: &mut W) -> Result<()> {`
	`502`	`+ pub fn generate<W: Write + ?Sized>(&self, out: &mut W) -> Result<()> {`
`503`	`503`	`debug!("Generating changelog...");`
`504`	`504`	`let postprocessors = self`
`505`	`505`	`.config`
`@@ -567,7 +567,7 @@ impl<'a> Changelog<'a> {`
`567`	`567`	`}`
`568`	`568`
`569`	`569`	`/// Generates a changelog and prepends it to the given changelog.`
`570`		`- pub fn prepend<W: Write>(`
	`570`	`+ pub fn prepend<W: Write + ?Sized>(`
`571`	`571`	`&self,`
`572`	`572`	`mut changelog: String,`
`573`	`573`	`out: &mut W,`
`@@ -582,7 +582,7 @@ impl<'a> Changelog<'a> {`
`582`	`582`	`}`
`583`	`583`
`584`	`584`	`/// Prints the changelog context to the given output.`
`585`		`- pub fn write_context<W: Write>(&self, out: &mut W) -> Result<()> {`
	`585`	`+ pub fn write_context<W: Write + ?Sized>(&self, out: &mut W) -> Result<()> {`
`586`	`586`	`let output = Releases {`
`587`	`587`	`releases: &self.releases,`
`588`	`588`	`}`