fix: harden pre-push checks and add tests

packages/push/README.md

@@ -2,6 +2,8 @@
 
 Validation for Git push hook.
 
+Rejects pushes to the remote's default branch if the pushed commits contain merge commits.
+
 ## Usage
 
 Follow installation instructions for [husky](https://typicode.github.io/husky/#/?id=usage) and then:

packages/push/cli.test.js

@@ -0,0 +1,16 @@
+const check = require('.');
+
+jest.unmock('./cli');
+
+describe('cli', () => {
+	it('invokes check with the remote argument', () => {
+		const argv = process.argv;
+		process.argv = ['node', 'cli.js', 'upstream'];
+		try {
+			jest.isolateModules(() => require('./cli'));
+		} finally {
+			process.argv = argv;
+		}
+		expect(check).toHaveBeenCalledWith('upstream');
+	});
+});

packages/push/index.js

@@ -1,38 +1,50 @@
 const {readFileSync} = require('fs');
 const {spawnSync} = require('child_process');
 
-const z40 = '0000000000000000000000000000000000000000';
+const isZero = (sha) => /^0+$/.test(sha);
+
+const git = (args) => spawnSync('git', args, {encoding: 'utf8', env: {...process.env, LC_ALL: 'C'}});
 
 const getHeadBranch = (remote) => {
-	const {stdout} = spawnSync('git', ['remote', 'show', remote], {encoding: 'utf8'});
-	const match = /HEAD branch: (.+)/.exec(stdout);
-	return match ? match[1] : 'master';
+	const symbolic = git(['symbolic-ref', '--short', `refs/remotes/${remote}/HEAD`]);
+	if (symbolic.status === 0) {
+		const match = /^[^/]+\/(.+)$/.exec(symbolic.stdout.trim());
+		if (match) return match[1];
+	}
+	const show = git(['remote', 'show', remote]);
+	const match = show.status === 0 && /HEAD branch: (.+)/.exec(show.stdout);
+	if (match) return match[1];
+	process.stderr.write(`Unable to determine the default branch of ${remote}, assuming master\n`);
+	return 'master';
 };
 
 const checkMerges = (range, localRef) => {
-	const {stdout} = spawnSync('git', ['rev-list', '--merges', range], {encoding: 'utf8'});
+	const {status, stdout, stderr} = git(['rev-list', '--merges', range]);
+	if (status !== 0) return `Unable to check commit ${localRef}: ${stderr.trim()}`;
 	return stdout ? `Commit ${localRef} contains merges` : null;
 };
 
-module.exports = (remote) => {
-	const headSuffix = `/${getHeadBranch(remote)}`;
+module.exports = (remote = 'origin') => {
+	const headRef = `refs/heads/${getHeadBranch(remote)}`;
 	const errors = readFileSync(0, 'utf8')
 		.split('\n')
 		.map((line) => {
-			if (!line) return null;
 			const result = /(\S+) (\S+) (\S+) (\S+)/.exec(line);
+			if (!result) return null;
 			const [, localRef, localSha, remoteRef, remoteSha] = result;
-			if (remoteRef.endsWith(headSuffix) && localSha !== z40) {
-				const range = remoteSha === z40 ? localSha : `${remoteSha}..${localSha}`;
+			if (remoteRef === headRef && !isZero(localSha)) {
+				const range = isZero(remoteSha) ? localSha : `${remoteSha}..${localSha}`;
 				return checkMerges(range, localRef);
 			}
 			return null;
 		})
 		.filter(Boolean);
 
-	if (errors.length) {
-		process.stderr.write(`${errors.join('\n')}\n\nFound ${errors.length} problems, rejecting push\n`, () =>
-			process.exit(1)
+	const count = errors.length;
+	if (count) {
+		process.stderr.write(
+			`${errors.join('\n')}\n\nFound ${count} ${count === 1 ? 'problem' : 'problems'}, rejecting push\n`,
+			() => process.exit(1)
 		);
 	} else {
 		process.exit(0);

packages/push/index.test.js

@@ -0,0 +1,170 @@
+const {readFileSync} = require('fs');
+const {spawnSync} = require('child_process');
+
+const check = require('.');
+
+jest.mock('fs');
+jest.mock('child_process');
+jest.unmock('.');
+
+const z40 = '0'.repeat(40);
+const z64 = '0'.repeat(64);
+
+const ok = (stdout = '') => ({status: 0, stdout, stderr: ''});
+const fail = (stderr = '') => ({status: 1, stdout: '', stderr});
+
+const mockGit = ({symbolic = ok('origin/main\n'), show = ok('  HEAD branch: main\n'), revList = ok()} = {}) =>
+	spawnSync.mockImplementation((cmd, args) => {
+		expect(cmd).toBe('git');
+		switch (args[0]) {
+			case 'symbolic-ref':
+				return symbolic;
+			case 'remote':
+				return show;
+			case 'rev-list':
+				return typeof revList === 'function' ? revList(args) : revList;
+			default:
+				throw new Error(`Unexpected ${args}`);
+		}
+	});
+
+describe('check', () => {
+	let exit;
+	let write;
+
+	beforeEach(() => {
+		exit = jest.spyOn(process, 'exit').mockImplementation(() => {});
+		write = jest.spyOn(process.stderr, 'write').mockImplementation((text, cb) => cb && cb());
+	});
+
+	afterEach(() => {
+		exit.mockRestore();
+		write.mockRestore();
+	});
+
+	it('uses origin if no remote is specified', () => {
+		mockGit();
+		readFileSync.mockReturnValue('');
+		check();
+		expect(spawnSync.mock.calls[0][1]).toEqual(['symbolic-ref', '--short', 'refs/remotes/origin/HEAD']);
+	});
+
+	it('uses the local remote HEAD to find the default branch', () => {
+		mockGit({show: fail()});
+		readFileSync.mockReturnValue(`refs/heads/main abc refs/heads/main def\n`);
+		check('origin');
+		expect(spawnSync.mock.calls.map((c) => c[1][0])).toEqual(['symbolic-ref', 'rev-list']);
+		expect(spawnSync.mock.calls[1][1]).toEqual(['rev-list', '--merges', 'def..abc']);
+		expect(spawnSync.mock.calls[0][2].env.LC_ALL).toBe('C');
+	});
+
+	it('falls back to remote show if symbolic-ref fails', () => {
+		mockGit({symbolic: fail()});
+		readFileSync.mockReturnValue(`refs/heads/main abc refs/heads/main def\n`);
+		check('origin');
+		expect(spawnSync.mock.calls[2][1]).toEqual(['rev-list', '--merges', 'def..abc']);
+		expect(write).not.toHaveBeenCalled();
+		expect(exit).toHaveBeenCalledWith(0);
+	});
+
+	it('falls back to remote show if symbolic-ref output is unexpected', () => {
+		mockGit({symbolic: ok('garbage\n')});
+		readFileSync.mockReturnValue(`refs/heads/main abc refs/heads/main def\n`);
+		check('origin');
+		expect(spawnSync.mock.calls[1][1]).toEqual(['remote', 'show', 'origin']);
+		expect(exit).toHaveBeenCalledWith(0);
+	});
+
+	it('assumes master with a warning if the default branch cannot be determined', () => {
+		mockGit({symbolic: fail(), show: fail()});
+		readFileSync.mockReturnValue(`refs/heads/master abc refs/heads/master def\n`);
+		check('origin');
+		expect(write).toHaveBeenCalledWith('Unable to determine the default branch of origin, assuming master\n');
+		expect(spawnSync.mock.calls[2][1]).toEqual(['rev-list', '--merges', 'def..abc']);
+	});
+
+	it('assumes master if remote show has no HEAD branch', () => {
+		mockGit({symbolic: fail(), show: ok('nothing\n')});
+		readFileSync.mockReturnValue('');
+		check('origin');
+		expect(write).toHaveBeenCalled();
+	});
+
+	it('exits with 0 if there is no input', () => {
+		mockGit();
+		readFileSync.mockReturnValue('');
+		check('origin');
+		expect(exit).toHaveBeenCalledWith(0);
+	});
+
+	it('ignores malformed lines', () => {
+		mockGit();
+		readFileSync.mockReturnValue('garbage\n\n');
+		check('origin');
+		expect(exit).toHaveBeenCalledWith(0);
+	});
+
+	it('ignores refs which are not the default branch', () => {
+		mockGit();
+		readFileSync.mockReturnValue(
+			'refs/heads/a abc refs/heads/feature/main def\nrefs/tags/main abc refs/tags/main def\nrefs/heads/a abc refs/heads/other def\n'
+		);
+		check('origin');
+		expect(spawnSync.mock.calls.some((c) => c[1][0] === 'rev-list')).toBe(false);
+		expect(exit).toHaveBeenCalledWith(0);
+	});
+
+	it('ignores deletion of the default branch', () => {
+		mockGit();
+		readFileSync.mockReturnValue(`(delete) ${z40} refs/heads/main def\n(delete) ${z64} refs/heads/main def\n`);
+		check('origin');
+		expect(spawnSync.mock.calls.some((c) => c[1][0] === 'rev-list')).toBe(false);
+		expect(exit).toHaveBeenCalledWith(0);
+	});
+
+	it('checks the whole local history if the remote branch does not exist', () => {
+		mockGit();
+		readFileSync.mockReturnValue(`refs/heads/main abc refs/heads/main ${z40}\n`);
+		check('origin');
+		expect(spawnSync.mock.calls[1][1]).toEqual(['rev-list', '--merges', 'abc']);
+	});
+
+	it('supports sha-256 zero ids', () => {
+		mockGit();
+		readFileSync.mockReturnValue(`refs/heads/main abc refs/heads/main ${z64}\n`);
+		check('origin');
+		expect(spawnSync.mock.calls[1][1]).toEqual(['rev-list', '--merges', 'abc']);
+	});
+
+	it('rejects the push if there are merges', () => {
+		mockGit({revList: ok('abc\n')});
+		readFileSync.mockReturnValue('refs/heads/main abc refs/heads/main def\n');
+		check('origin');
+		expect(write).toHaveBeenCalledWith(
+			'Commit refs/heads/main contains merges\n\nFound 1 problem, rejecting push\n',
+			expect.any(Function)
+		);
+		expect(exit).toHaveBeenCalledWith(1);
+	});
+
+	it('pluralizes the problem count', () => {
+		mockGit({revList: ok('abc\n')});
+		readFileSync.mockReturnValue('refs/heads/a abc refs/heads/main def\nrefs/heads/b ghi refs/heads/main def\n');
+		check('origin');
+		expect(write).toHaveBeenCalledWith(
+			'Commit refs/heads/a contains merges\nCommit refs/heads/b contains merges\n\nFound 2 problems, rejecting push\n',
+			expect.any(Function)
+		);
+	});
+
+	it('rejects the push if rev-list fails', () => {
+		mockGit({revList: fail('fatal: bad object def\n')});
+		readFileSync.mockReturnValue('refs/heads/main abc refs/heads/main def\n');
+		check('origin');
+		expect(write).toHaveBeenCalledWith(
+			'Unable to check commit refs/heads/main: fatal: bad object def\n\nFound 1 problem, rejecting push\n',
+			expect.any(Function)
+		);
+		expect(exit).toHaveBeenCalledWith(1);
+	});
+});

packages/push/package.json

@@ -16,6 +16,7 @@
     "index.js",
     "README.md"
   ],
+  "main": "index.js",
   "bin": "cli.js",
   "publishConfig": {
     "access": "public"