Repository: orhun/git-cliff (rust)

Issue:
Commit footers have values but no names
**Describe the bug**

When a conventional commit has footers (such as `Signed-off-by: Someone <email>`), the `footers` array provided to the changelog template contains the value (the part *after* the `:`) but not the name of the footer.  This makes it difficult to implement any kind of special templating based on the name of the footer. For example, if I want to add `Signed-off-by` emails to a changelog entry, but not `Reviewed-by`, it's currently impossible to determine what commit footer that value came from.

**To Reproduce**

In a repository where commits contain multiple footers with different names, create a template with `{{ commit.footers }}` in it somewhere. Note that only the values of the footer are output.

**Expected behavior**
 
The entire footer (including the name) should be exposed to the template. This could either be by making each entry in the `commit.footers` array be the _entire_ footer value, including both the name and the value after the colon, or by changing `commit.footers` from an array to a map of footer names to footer values.

**System**
 - OS Information: 
   ```
   :; uname -a
   Linux noctis 5.17.3 #1-NixOS SMP PREEMPT Wed Apr 13 17:27:43 UTC 2022 x86_64 GNU/Linux
   ```

 - Project Version:
   ```
   :; git-cliff --version
   git-cliff 0.7.0
   ```

**Additional context**

While looking at the code for serializing commits as JSON, I noticed that the `Serialize` impl maps over the list of footers and calls `f.value()`:
https://github.com/orhun/git-cliff/blob/7d0786ca55423950f0779de4e6a907fc25ae203a/git-cliff-core/src/commit.rs#L221-L226

Although I'm not familiar with the `git_conventional` crate's API, my assumption is that this is discarding the footer's name. :)

I'm happy to open a PR changing this behavior, which I imagine would be fairly straightforward. The one design decision is what we should output instead: is it preferable to emit the footers as a map of footer name to footer value (which could break existing templates, but seems nicer), or as an array of the footer's name and value as a single string? Or, is there some other approach that I haven't thought of?


Pull request description:
feat(commit)!: pass footer token and separator to template
## Description

This branch changes the `Serialize` impl for `Commit` to output conventional
commit footers as structured JSON objects, rather than as just the footer's
value string.

See hawkw/git-cliff@93779b21667fd5f7ec28635e36e428035d69afb5 for a discussion of
the implementation details of this change.

## Motivation and Context

As discussed in issue #96, using footers in changelog templates is currently not
practical in most cases, since there is currently no way to determine *which*
footer a value belongs to, and no way to access the name of the footer.

This PR implements the approach suggested by @orhun in https://github.com/orhun/git-cliff/issues/96#issuecomment-1164883227.
Additionally, I added a section to the README documentation discussing the
structure of the footers passed to templates.

Fixes #96

## How Has This Been Tested?

I added a new test in `commit.rs` that conventional commits with footers have a
`footers` iterator containing the correct footers. In addition, I did some
manual testing to test the use of footers in templates --- see the next section
for details.

## Screenshots / Output (if appropriate):

To manually verify that everthing works end-to-end, I temporarily modified
`examples/detailed.toml` to output footers in the template:

```patch
diff --git a/examples/detailed.toml b/examples/detailed.toml
index b6b57fd..6fc2252 100644
--- a/examples/detailed.toml
+++ b/examples/detailed.toml
@@ -24,6 +24,9 @@ body = """
     ### {{ group | upper_first }}
     {% for commit in commits %}
         - {{ commit.message | upper_first }} ([{{ commit.id | truncate(length=7, end="") }}]({{ commit.id }}))\
+          {% for footer in commit.footers -%}
+            , {{ footer.token }}{{ footer.separator }} {{ footer.value }}\
+          {% endfor %}
     {% endfor %}
 {% endfor %}\n
 """
```

Generating a test changelog with that template now outputs footers, as expected:

```markdown
# Changelog

All notable changes to this project will be documented in this file.

## [unreleased]

### Documentation

- Clarify that `--tag` argument can be an unexisting tag ([d540f5d](d540f5d8938bc84b01b4fafaa69c3290eb72cd08))

- Discuss footers in README ([a54bca5](a54bca57acc91a886d52bb9d3d87866eb5c18f32)), Signed-off-by: Eliza Weisman <eliza@buoyant.io>


### Features

- Support external commands for commit preprocessors (#86) ([7d0786c](7d0786ca55423950f0779de4e6a907fc25ae203a))

- Support changing commit scope with `commit_parsers` (#94) ([e220768](e22076843b91be3680617db5686e070dedcfef29)), Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>

...
```

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Documentation (no code change)
- [ ] Refactor (refactoring production code)
- [ ] Other <!--- (provide information) -->

## Checklist:
- [x] My code follows the code style of this project.
- [x] I have updated the documentation accordingly.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.


Patch:
diff --git a/README.md b/README.md
index cba03a7..6934e9f 100644
--- a/README.md
+++ b/README.md
@@ -581,7 +581,7 @@ While `skip_tags` drop commits from the changelog, `ignore_tags` include ignored
 
 #### date_order
 
-If set to `true`, tags are processed in topological order instead of chronological.
+If set to `true`, tags are processed in chronological order instead of topological.
 
 This can also be achieved by using the `--date-order` command line flag.
 
@@ -669,7 +669,14 @@ following context is generated to use for templating:
       "scope": "[scope]",
       "message": "<description>",
       "body": "[body]",
-      "footers": ["[footer]", "[footer]"],
+      "footers": [
+        {
+          "token": "<name of the footer, such as 'Signed-off-by'>",
+          "separator": "<the separator between the token and value, such as ':'>",
+          "value": "<the value following the separator",
+          "breaking": false
+        }
+      ],
       "breaking_description": "<description>",
       "breaking": false,
       "conventional": true,
@@ -684,6 +691,24 @@ following context is generated to use for templating:
 }
 ```
 
+##### Footers
+
+A conventional commit's body may end with any number of structured key-value pairs known as [_footers_](https://www.conventionalcommits.org/en/v1.0.0/#specification). These consist of a string token naming the footer, a separator (which is either `: ` or ` #`), and a value, similar to [the git trailers convention](https://git-scm.com/docs/git-interpret-trailers).
+
+For example:
+
+- `Signed-off-by: User Name <user.email@example.com>`
+- `Reviewed-by: User Name <user.email@example.com>`
+- `Fixes #1234`
+- `BREAKING CHANGE: breaking change description`
+
+When a conventional commit contains footers, the footers are passed to the template in a `footers` array in the commit object. Each footer is represented by an object with the following fields:
+
+- `"token"`, the name of the footer (preceeding the separator character)
+- `separator`, the footer's separator string (either `: ` or ` #`)
+- `value`, the value following the separator character
+- `breaking`, which is `true` if this is a `BREAKING CHANGE:` footer, and `false` otherwise
+
 ##### Breaking Changes
 
 `breaking` flag is set to `true` when the commit has an exclamation mark after the commit type and scope, e.g.:
@@ -702,6 +727,9 @@ BREAKING CHANGE: this is a breaking change
 
 `breaking_description` is set to the explanation of the breaking change. This description is expected to be present in the `BREAKING CHANGE` footer. However, if it's not provided, the `message` is expected to describe the breaking change.
 
+If the `BREAKING CHANGE:` footer is present, the footer will also be included in
+`commit.footers`.
+
 #### Non-Conventional Commits
 
 > conventional_commits = **false**
diff --git a/examples/detailed.toml b/examples/detailed.toml
index b6b57fd..bd355c1 100644
--- a/examples/detailed.toml
+++ b/examples/detailed.toml
@@ -24,6 +24,9 @@ body = """
     ### {{ group | upper_first }}
     {% for commit in commits %}
         - {{ commit.message | upper_first }} ([{{ commit.id | truncate(length=7, end="") }}]({{ commit.id }}))\
+          {% for footer in commit.footers -%}
+            , {{ footer.token }}{{ footer.separator }}{{ footer.value }}\
+          {% endfor %}\
     {% endfor %}
 {% endfor %}\n
 """
diff --git a/git-cliff-core/src/commit.rs b/git-cliff-core/src/commit.rs
index adb192f..be87679 100644
--- a/git-cliff-core/src/commit.rs
+++ b/git-cliff-core/src/commit.rs
@@ -49,6 +49,35 @@ pub struct Link {
 	pub href: String,
 }
 
+/// A conventional commit footer.
+#[derive(Debug, Clone, Eq, PartialEq, serde::Deserialize, serde::Serialize)]
+struct Footer<'a> {
+	/// Token of the footer.
+	///
+	/// This is the part of the footer preceding the separator. For example, for
+	/// the `Signed-off-by: <user.name>` footer, this would be `Signed-off-by`.
+	token:     &'a str,
+	/// The separator between the footer token and its value.
+	///
+	/// This is typically either `:` or `#`.
+	separator: &'a str,
+	/// The value of the footer.
+	value:     &'a str,
+	/// A flag to signal that the footer describes a breaking change.
+	breaking:  bool,
+}
+
+impl<'a> From<&'a git_conventional::Footer<'a>> for Footer<'a> {
+	fn from(footer: &'a git_conventional::Footer<'a>) -> Self {
+		Self {
+			token:     footer.token().as_str(),
+			separator: footer.separator().as_str(),
+			value:     footer.value(),
+			breaking:  footer.breaking(),
+		}
+	}
+}
+
 impl<'a> From<&GitCommit<'a>> for Commit<'a> {
 	fn from(commit: &GitCommit<'a>) -> Self {
 		Self::new(
@@ -208,6 +237,16 @@ impl Commit<'_> {
 		}
 		Ok(self)
 	}
+
+	/// Returns an iterator over this commit's [`Footer`]s, if this is a
+	/// conventional commit.
+	///
+	/// If this commit is not conventional, the returned iterator will be empty.
+	fn footers(&self) -> impl Iterator<Item = Footer<'_>> {
+		self.conv
+			.iter()
+			.flat_map(|conv| conv.footers().iter().map(Footer::from))
+	}
 }
 
 impl Serialize for Commit<'_> {
@@ -215,21 +254,29 @@ impl Serialize for Commit<'_> {
 	where
 		S: Serializer,
 	{
+		/// A wrapper to serialize commit footers from an iterator using
+		/// `Serializer::collect_seq` without having to allocate in order to
+		/// `collect` the footers  into a new to `Vec`.
+		struct SerializeFooters<'a>(&'a Commit<'a>);
+		impl Serialize for SerializeFooters<'_> {
+			fn serialize<S>(
+				&self,
+				serializer: S,
+			) -> std::result::Result<S::Ok, S::Error>
+			where
+				S: Serializer,
+			{
+				serializer.collect_seq(self.0.footers())
+			}
+		}
+
 		let mut commit = serializer.serialize_struct("Commit", 9)?;
 		commit.serialize_field("id", &self.id)?;
 		match &self.conv {
 			Some(conv) => {
 				commit.serialize_field("message", conv.description())?;
 				commit.serialize_field("body", &conv.body())?;
-				commit.serialize_field(
-					"footers",
-					&conv
-						.footers()
-						.to_vec()
-						.iter()
-						.map(|f| f.value())
-						.collect::<Vec<&str>>(),
-				)?;
+				commit.serialize_field("footers", &SerializeFooters(self))?;
 				commit.serialize_field(
 					"group",
 					self.group.as_ref().unwrap_or(&conv.type_().to_string()),
@@ -304,6 +351,58 @@ mod test {
 		assert_eq!(Some(String::from("test_scope")), commit.default_scope);
 	}
 
+	#[test]
+	fn conventional_footers() {
+		let cfg = crate::config::GitConfig {
+			conventional_commits: Some(true),
+			..Default::default()
+		};
+		let test_cases = vec![
+			(
+				Commit::new(
+					String::from("123123"),
+					String::from(
+						"test(commit): add test\n\nSigned-off-by: Test User \
+						 <test@example.com>",
+					),
+				),
+				vec![Footer {
+					token:     "Signed-off-by",
+					separator: ":",
+					value:     "Test User <test@example.com>",
+					breaking:  false,
+				}],
+			),
+			(
+				Commit::new(
+					String::from("123124"),
+					String::from(
+						"fix(commit): break stuff\n\nBREAKING CHANGE: This commit \
+						 breaks stuff\nSigned-off-by: Test User <test@example.com>",
+					),
+				),
+				vec![
+					Footer {
+						token:     "BREAKING CHANGE",
+						separator: ":",
+						value:     "This commit breaks stuff",
+						breaking:  true,
+					},
+					Footer {
+						token:     "Signed-off-by",
+						separator: ":",
+						value:     "Test User <test@example.com>",
+						breaking:  false,
+					},
+				],
+			),
+		];
+		for (commit, footers) in &test_cases {
+			let commit = commit.process(&cfg).expect("commit should process");
+			assert_eq!(&commit.footers().collect::<Vec<_>>(), footers);
+		}
+	}
+
 	#[test]
 	fn parse_link() {
 		let test_cases = vec![
diff --git a/git-cliff-core/src/repo.rs b/git-cliff-core/src/repo.rs
index 347542a..159cc75 100644
--- a/git-cliff-core/src/repo.rs
+++ b/git-cliff-core/src/repo.rs
@@ -56,37 +56,33 @@ impl Repository {
 			.filter_map(|id| self.inner.find_commit(id).ok())
 			.collect();
 		if include_path.is_some() || exclude_path.is_some() {
-			commits = commits
-				.into_iter()
-				.filter(|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
-									}
-								});
-						}
+			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
-				})
-				.collect()
+				}
+				false
+			});
 		}
 		Ok(commits)
 	}


Test patch:
diff --git a/.github/fixtures/test-commit-footers/cliff.toml b/.github/fixtures/test-commit-footers/cliff.toml
new file mode 100644
index 00000000..1cd862a
--- /dev/null
+++ b/.github/fixtures/test-commit-footers/cliff.toml
@@ -0,0 +1,30 @@
+[changelog]
+# changelog header
+header = """
+# Changelog\n
+All notable changes to this project will be documented in this file.\n
+"""
+# template for the changelog body
+# https://tera.netlify.app/docs/#introduction
+body = """
+{% if version %}\
+    ## [{{ version | trim_start_matches(pat="v") }}] - {{ timestamp | date(format="%Y-%m-%d") }}
+{% else %}\
+    ## [unreleased]
+{% endif %}\
+{% for group, commits in commits | group_by(attribute="group") %}
+    ### {{ group | upper_first }}
+    {% for commit in commits %}
+        - {{ commit.message | upper_first }} ([{{ commit.id | truncate(length=7, end="") }}]({{ commit.id }}))\
+          {% for footer in commit.footers -%}
+            , {{ footer.token }}{{ footer.separator }}{{ footer.value }}\
+          {% endfor %}\
+    {% endfor %}
+{% endfor %}\n
+"""
+# remove the leading and trailing whitespace from the template
+trim = true
+# changelog footer
+footer = """
+<!-- generated by git-cliff -->
+"""
diff --git a/.github/fixtures/test-commit-footers/commit.sh b/.github/fixtures/test-commit-footers/commit.sh
new file mode 100755
index 00000000..d19473d
--- /dev/null
+++ b/.github/fixtures/test-commit-footers/commit.sh
@@ -0,0 +1,9 @@
+#!/usr/bin/env bash
+set -e
+
+GIT_COMMITTER_DATE="2021-01-23 01:23:45" git commit --allow-empty -m "feat: add feature 1" -m "footer: test"
+
+GIT_COMMITTER_DATE="2021-01-23 01:23:46" git commit --allow-empty -m "feat: add feature 2" -m "Signed-off-by: bot"
+git tag v0.1.0
+
+GIT_COMMITTER_DATE="2021-01-23 01:23:47" git commit --allow-empty -m "fix: fix feature 1" -m "footer1: xyz" -m "footer2: abc"
diff --git a/.github/fixtures/test-commit-footers/expected.md b/.github/fixtures/test-commit-footers/expected.md
new file mode 100644
index 00000000..6a08cda
--- /dev/null
+++ b/.github/fixtures/test-commit-footers/expected.md
@@ -0,0 +1,18 @@
+# Changelog
+
+All notable changes to this project will be documented in this file.
+
+## [unreleased]
+
+### Fix
+
+- Fix feature 1 ([540f28b](540f28b88861802ca6c196482c5c70933593561b)), footer1:xyz, footer2:abc
+
+## [0.1.0] - 2021-01-23
+
+### Feat
+
+- Add feature 1 ([376fd60](376fd6043cb27af83973f31dd6aab87486d8e554)), footer:test
+- Add feature 2 ([fc086fa](fc086faec7a5bd4429f62f01c4a871631f63be68)), Signed-off-by:bot
+
+<!-- generated by git-cliff -->