From 607c5d3c24d1ed36a5b31d8fae5723b05c8b1d2d Mon Sep 17 00:00:00 2001
From: Chris Kay <chris.kay@arm.com>
Date: Wed, 28 Apr 2021 10:43:37 +0100
Subject: [PATCH] fix!: fix signed-off-by detection

The signed-off-by rule does not correctly detect `Signed-off-by:`
trailers if they are not the final non-comment line of the commit. This
change ensures that sign-off trailers are correctly detected according
to the formatting rules Git uses to insert them.

The change replaces the manual parsing of the commit message with an
invocation of `git interpret-trailers`, which parses the trailers of
the commit message passed via stdin.

One known issue with this approach is that `BREAKING CHANGE` - if it
occurs grouped alongside the sign-off trailers - will break detection.
This appears to be due to the fact that Git does not accept trailers
with spaces, instead treating them as part of the body (this seems to
be an issue of the Conventional Commits specification more than
anything). This does not occur with `BREAKING-CHANGE`, which is
considered a valid trailer.

BREAKING CHANGE: `BREAKING CHANGE` in the footers block will break `Signed-off-by:` detection.

Signed-off-by: Chris Kay <chris.kay@arm.com>
---
 @commitlint/rules/package.json              |  3 ++-
 @commitlint/rules/src/signed-off-by.test.ts | 18 ++++++++++--------
 @commitlint/rules/src/signed-off-by.ts      | 16 +++++++---------
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/@commitlint/rules/package.json b/@commitlint/rules/package.json
index dc450eb291..f8686e284c 100644
--- a/@commitlint/rules/package.json
+++ b/@commitlint/rules/package.json
@@ -44,7 +44,8 @@
     "@commitlint/ensure": "^12.1.1",
     "@commitlint/message": "^12.1.1",
     "@commitlint/to-lines": "^12.1.1",
-    "@commitlint/types": "^12.1.1"
+    "@commitlint/types": "^12.1.1",
+    "execa": "^5.0.0"
   },
   "gitHead": "70f7f4688b51774e7ac5e40e896cdaa3f132b2bc"
 }
diff --git a/@commitlint/rules/src/signed-off-by.test.ts b/@commitlint/rules/src/signed-off-by.test.ts
index 29fc665501..ba83237a49 100644
--- a/@commitlint/rules/src/signed-off-by.test.ts
+++ b/@commitlint/rules/src/signed-off-by.test.ts
@@ -3,15 +3,17 @@ import {signedOffBy} from './signed-off-by';
 
 const messages = {
 	empty: 'test:\n',
-	with: `test: subject\nbody\nfooter\nSigned-off-by:\n\n`,
-	without: `test: subject\nbody\nfooter\n\n`,
-	inSubject: `test: subject Signed-off-by:\nbody\nfooter\n\n`,
-	inBody: `test: subject\nbody Signed-off-by:\nfooter\n\n`,
-	withSignoffAndComments: `test: subject
+	with: `test: subject\n\nbody\n\nfooter\n\nSigned-off-by:\n\n`,
+	without: `test: subject\n\nbody\n\nfooter\n\n`,
+	inSubject: `test: subject Signed-off-by:\n\nbody\n\nfooter\n\n`,
+	inBody: `test: subject\n\nbody Signed-off-by:\n\nfooter\n\n`,
+	withSignoffAndNoise: `test: subject
 
 message body
 
+Arbitrary-trailer:
 Signed-off-by:
+Another-arbitrary-trailer:
 
 # Please enter the commit message for your changes. Lines starting
 # with '#' will be ignored, and an empty message aborts the commit.
@@ -24,7 +26,7 @@ const parsed = {
 	without: parse(messages.without),
 	inSubject: parse(messages.inSubject),
 	inBody: parse(messages.inBody),
-	withSignoffAndComments: parse(messages.withSignoffAndComments),
+	withSignoffAndNoise: parse(messages.withSignoffAndNoise),
 };
 
 test('empty against "always signed-off-by" should fail', async () => {
@@ -67,9 +69,9 @@ test('without against "never signed-off-by" should succeed', async () => {
 	expect(actual).toEqual(expected);
 });
 
-test('trailing comments should be ignored', async () => {
+test('comments and other trailers should be ignored', async () => {
 	const [actual] = signedOffBy(
-		await parsed.withSignoffAndComments,
+		await parsed.withSignoffAndNoise,
 		'always',
 		'Signed-off-by:'
 	);
diff --git a/@commitlint/rules/src/signed-off-by.ts b/@commitlint/rules/src/signed-off-by.ts
index 674a206197..d213b64e41 100644
--- a/@commitlint/rules/src/signed-off-by.ts
+++ b/@commitlint/rules/src/signed-off-by.ts
@@ -1,3 +1,4 @@
+import execa from 'execa';
 import message from '@commitlint/message';
 import toLines from '@commitlint/to-lines';
 import {SyncRule} from '@commitlint/types';
@@ -7,18 +8,15 @@ export const signedOffBy: SyncRule<string> = (
 	when = 'always',
 	value = ''
 ) => {
-	const lines = toLines(parsed.raw).filter(
-		(ln) =>
-			// skip comments
-			!ln.startsWith('#') &&
-			// ignore empty lines
-			Boolean(ln)
-	);
+	const trailers = execa.sync('git', ['interpret-trailers', '--parse'], {
+		input: parsed.raw,
+	}).stdout;
 
-	const last = lines[lines.length - 1];
+	const signoffs = toLines(trailers).filter((ln) => ln.startsWith(value))
+		.length;
 
 	const negated = when === 'never';
-	const hasSignedOffBy = last.startsWith(value);
+	const hasSignedOffBy = signoffs > 0;
 
 	return [
 		negated ? !hasSignedOffBy : hasSignedOffBy,