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

fix: respect existing trailers in commit messages #632

Merged
merged 1 commit into from
Jul 31, 2022
Merged
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
25 changes: 22 additions & 3 deletions lib/landing_session.js
Original file line number Diff line number Diff line change
@@ -306,9 +306,24 @@ export default class LandingSession extends Session {
const original = runSync('git', [
'show', 'HEAD', '-s', '--format=%B'
]).trim();

// git has very specific rules about what is a trailer and what is not.
// Instead of trying to implement those ourselves, let git parse the
// original commit message and see if it outputs any trailers.
const originalHasTrailers = runSync('git', [
'interpret-trailers', '--parse', '--no-divider'
], {
input: `${original}\n`
}).trim().length !== 0;

const metadata = this.metadata.trim().split('\n');
const amended = original.split('\n');
if (amended[amended.length - 1] !== '') {
Copy link
Member Author

@tniessen tniessen May 14, 2022

Choose a reason for hiding this comment

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

Comment: This condition appears to have always been false because the code above calls trim() on the commit message before splitting it at \n.


// If the original commit message already contains trailers (such as
// "Co-authored-by"), we simply add our own metadata after those. Otherwise,
// we have to add an empty line so that git recognizes our own metadata as
// trailers in the amended commit message.
if (!originalHasTrailers) {
amended.push('');
}

@@ -317,9 +332,13 @@ export default class LandingSession extends Session {
const REVIEW_RE = /Reviewed-By\s*:\s*(\S+)/i;

for (const line of metadata) {
if (original.includes(line)) {
if (line) {
if (line.length !== 0 && original.includes(line)) {
if (originalHasTrailers) {
cli.warn(`Found ${line}, skipping..`);
} else {
cli.error('Git found no trailers in the original commit message, ' +
`but '${line}' is present and should be a trailer.`);
process.exit(1); // make it work with git rebase -x
Copy link
Member Author

@tniessen tniessen May 14, 2022

Choose a reason for hiding this comment

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

Comment: It seems reasonable to abort in this case, but if anyone thinks it could be problematic, I'd be happy to revert this particular check.

}
} else {
if (line.match(BACKPORT_RE)) {