Skip to content

fix: ignore reverted commit pairs #2873

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.

Sign up for GitHub

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

Open
iamhaseebn wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: ignore reverted commit pairs #2873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
109 changes: 99 additions & 10 deletions src/commit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,9 @@ function toConventionalChangelogFormat(
if (parent.type === 'token') {
parent = ancestors.pop();
let footerText = '';
const semanticFooter = node.value.toLowerCase() === 'release-as';
const footerType = node.value.toLowerCase();
const semanticFooter =
footerType === 'release-as' || footerType === 'revert';
visit(
parent,
['type', 'scope', 'breaking-change', 'separator', 'text', 'newline'],
Expand All @@ -281,7 +283,7 @@ function toConventionalChangelogFormat(
);
// Any footers that carry semantic meaning, e.g., Release-As, should
// be added to the footer field, for the benefits of post-processing:
if (semanticFooter) {
if (footerType === 'release-as') {
let releaseAs = '';
visit(parent, ['text'], (node: parser.Text) => {
releaseAs = node.value;
Expand All @@ -294,14 +296,16 @@ function toConventionalChangelogFormat(
if (!headerCommit.footer) headerCommit.footer = '';
headerCommit.footer += `\n${footerText.toLowerCase()}`.trimStart();
}
try {
for (const commit of toConventionalChangelogFormat(
parser.parser(footerText)
)) {
commits.push(commit);
if (!semanticFooter) {
try {
for (const commit of toConventionalChangelogFormat(
parser.parser(footerText)
)) {
commits.push(commit);
}
} catch (err) {
// Footer does not appear to be an additional commit.
}
} catch (err) {
// Footer does not appear to be an additional commit.
}
}
}
Expand Down Expand Up @@ -402,6 +406,91 @@ function splitMessages(message: string): string[] {
return [...conventionalCommits, ...messages.slice(1)];
}

const REVERT_TARGET_PATTERNS = [
/(?:^|\r?\n)This reverts commit ([0-9a-z]+)\.?(?=\r?(?:\n|$))/gi,
/(?:^|\r?\n)Revert:\s*([0-9a-z]+)\.?(?=\r?(?:\n|$))/gi,
];

function revertTargets(message: string): string[] {
const targets = new Set<string>();
for (const pattern of REVERT_TARGET_PATTERNS) {
pattern.lastIndex = 0;
let match: RegExpExecArray | null;
while ((match = pattern.exec(message)) !== null) {
targets.add(match[1].toLowerCase());
}
}
return [...targets];
}

/**
* Removes revert pairs whose original and reverting commits both occur in the
* current commit range. Reverts of commits outside the range remain visible,
* since they describe a change relative to the previous release.
*/
function filterRevertedCommits(commits: Commit[]): Commit[] {
const revertTargetByCommit = new Map<number, number>();
const revertersByTarget = new Map<number, number[]>();

for (let commitIndex = 0; commitIndex < commits.length; commitIndex++) {
const targets = revertTargets(commits[commitIndex].message);
if (targets.length === 0) continue;

let resolvedTarget: number | undefined;
let canResolve = true;
for (const target of targets) {
const matches: number[] = [];
for (
let candidateIndex = 0;
candidateIndex < commits.length;
candidateIndex++
) {
if (candidateIndex === commitIndex) continue;
const candidateSha = commits[candidateIndex].sha.toLowerCase();
if (
candidateSha.startsWith(target) ||
target.startsWith(candidateSha)
) {
matches.push(candidateIndex);
}
}

if (matches.length !== 1) {
canResolve = false;
break;
}
if (resolvedTarget !== undefined && resolvedTarget !== matches[0]) {
canResolve = false;
break;
}
resolvedTarget = matches[0];
}

if (!canResolve || resolvedTarget === undefined) continue;
revertTargetByCommit.set(commitIndex, resolvedTarget);
const reverters = revertersByTarget.get(resolvedTarget) ?? [];
reverters.push(commitIndex);
revertersByTarget.set(resolvedTarget, reverters);
}

const active = new Map<number, boolean>();
const isActive = (commitIndex: number): boolean => {
const cached = active.get(commitIndex);
if (cached !== undefined) return cached;

const isReverted = (revertersByTarget.get(commitIndex) ?? []).some(
reverterIndex => isActive(reverterIndex)
);
active.set(commitIndex, !isReverted);
return !isReverted;
};

return commits.filter((_, commitIndex) => {
if (!isActive(commitIndex)) return false;
return !revertTargetByCommit.has(commitIndex);
});
}

/**
* Given a list of raw commits, parse and expand into conventional commits.
*
Expand All @@ -417,7 +506,7 @@ export function parseConventionalCommits(
): ConventionalCommit[] {
const conventionalCommits: ConventionalCommit[] = [];

for (const commit of commits) {
for (const commit of filterRevertedCommits(commits)) {
for (const commitMessage of splitMessages(
preprocessCommitMessage(commit)
)) {
Expand Down
30 changes: 23 additions & 7 deletions test/changelog-notes/default-changelog-notes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,13 +374,29 @@ describe('DefaultChangelogNotes', () => {
expect(notes).to.not.include('Test User');
safeSnapshot(notes);
});
// it('ignores reverted commits', async () => {
// const commits = [buildCommitFromFixture('multiple-messages')];
// const changelogNotes = new DefaultChangelogNotes();
// const notes = await changelogNotes.buildNotes(parseConventionalCommits(commits), notesOptions);
// expect(notes).to.is.string;
// safeSnapshot(notes);
// });
it('ignores reverted commits', async () => {
const commits = parseConventionalCommits([
{
sha: '1111111111111111111111111111111111111111',
message: 'feat: some feature',
},
{
sha: '2222222222222222222222222222222222222222',
message:
'revert: feat: some feature\n\nThis reverts commit 1111111111111111111111111111111111111111.',
},
{
sha: '3333333333333333333333333333333333333333',
message: 'fix: some bugfix',
},
]);
const changelogNotes = new DefaultChangelogNotes();

const notes = await changelogNotes.buildNotes(commits, notesOptions);

expect(notes).to.include('some bugfix');
expect(notes).to.not.include('some feature');
});
});
});
describe('pull request compatibility', () => {
Expand Down
121 changes: 102 additions & 19 deletions test/commits.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,25 +263,108 @@ describe('parseConventionalCommits', () => {
expect(commit.type).to.eql('chore');
});

// it('ignores reverted commits', async () => {
// const commits = [
// {sha: 'sha1', message: 'feat: some feature', files: ['path1/file1.txt']},
// {
// sha: 'sha2',
// message: 'revert: feat: some feature\nThe reverts commit sha1.\n',
// files: ['path1/file1.rb'],
// },
// {
// sha: 'sha3',
// message: 'docs: some documentation',
// files: ['path1/file1.java'],
// },
// ];
// const conventionalCommits = parseConventionalCommits(commits);
// expect(conventionalCommits).lengthOf(1);
// expect(conventionalCommits[0].type).to.equal('docs');
// expect(conventionalCommits[0].scope).is.null;
// });
describe('with reverted commits', () => {
it('ignores a commit and its standard Git revert', async () => {
const commits = [
{
sha: '1111111111111111111111111111111111111111',
message: 'feat: some feature',
files: ['path1/file1.txt'],
},
{
sha: '2222222222222222222222222222222222222222',
message:
'revert: feat: some feature\n\nThis reverts commit 1111111111111111111111111111111111111111.\n',
files: ['path1/file1.txt'],
},
{
sha: '3333333333333333333333333333333333333333',
message: 'docs: some documentation',
files: ['path1/file1.java'],
},
];

const conventionalCommits = parseConventionalCommits(commits);

expect(conventionalCommits).lengthOf(1);
expect(conventionalCommits[0].type).to.equal('docs');
expect(conventionalCommits[0].scope).is.null;
});

it('supports an explicit Revert footer with an abbreviated SHA', async () => {
const commits = [
{
sha: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa',
message: 'fix: an unreleased fix',
},
{
sha: 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb',
message: 'revert: undo the fix\n\nRevert: aaaaaaa',
},
];

expect(parseConventionalCommits(commits)).to.be.empty;
});

it('keeps a revert when its target is outside the commit range', async () => {
const commits = [
{
sha: 'bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb',
message:
'revert: undo a released feature\n\nThis reverts commit aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.',
},
];

const conventionalCommits = parseConventionalCommits(commits);

expect(conventionalCommits).lengthOf(1);
expect(conventionalCommits[0].type).to.equal('revert');
});

it('does not match an ambiguous abbreviated SHA', async () => {
const commits = [
{
sha: 'abcdef0111111111111111111111111111111111',
message: 'feat: first feature',
},
{
sha: 'abcdef0222222222222222222222222222222222',
message: 'feat: second feature',
},
{
sha: '3333333333333333333333333333333333333333',
message: 'revert: unclear target\n\nRevert: abcdef0',
},
];

expect(parseConventionalCommits(commits)).lengthOf(3);
});

it('restores a commit when its revert is itself reverted', async () => {
const commits = [
{
sha: '1111111111111111111111111111111111111111',
message: 'feat: some feature',
},
{
sha: '2222222222222222222222222222222222222222',
message:
'revert: feat: some feature\n\nThis reverts commit 1111111111111111111111111111111111111111.',
},
{
sha: '3333333333333333333333333333333333333333',
message:
'revert: restore the feature\n\nThis reverts commit 2222222222222222222222222222222222222222.',
},
];

const conventionalCommits = parseConventionalCommits(commits);

expect(conventionalCommits).lengthOf(1);
expect(conventionalCommits[0].type).to.equal('feat');
expect(conventionalCommits[0].bareMessage).to.equal('some feature');
});
});
});

function assertHasCommit(
Expand Down
24 changes: 24 additions & 0 deletions test/versioning-strategies/default.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,34 @@
import {describe, it} from 'mocha';

import {expect} from 'chai';
import {parseConventionalCommits} from '../../src/commit';
import {DefaultVersioningStrategy} from '../../src/versioning-strategies/default';
import {Version} from '../../src/version';

describe('DefaultVersioningStrategy', () => {
it('does not bump for an unreleased feature that was reverted', async () => {
const commits = parseConventionalCommits([
{
sha: '1111111111111111111111111111111111111111',
message: 'feat: some feature',
},
{
sha: '2222222222222222222222222222222222222222',
message:
'revert: feat: some feature\n\nThis reverts commit 1111111111111111111111111111111111111111.',
},
{
sha: '3333333333333333333333333333333333333333',
message: 'fix: some bugfix',
},
]);
const strategy = new DefaultVersioningStrategy();

const newVersion = await strategy.bump(Version.parse('1.2.3'), commits);

expect(newVersion.toString()).to.equal('1.2.4');
});

describe('with breaking change', () => {
const commits = [
{
Expand Down
Loading