Exit with code 1 when a file gives an error to close #728

src/cli.ts

@@ -506,11 +506,19 @@ function unescapeString(str = '') {
         config.output = true;
     }*/
 
+	// The engine tells how many errors there were, and the CLI turns that into the exit code
+	const run = () =>
+		rexreplace.engine(config).then((errors) => {
+			if (errors) {
+				process.exitCode = 1;
+			}
+		});
+
 	// Like grep, stdin is only read when piped data is expected. An open pipe that nobody writes to would make the run wait forever.
 	const pipeDataExpected = !config.includeGlob.length || config.replacementPipe || config.pipe;
 
 	if (!pipeDataExpected || Boolean(process.stdin.isTTY)) {
-		return rexreplace.engine(config);
+		return run();
 	}
 
 	process.stdin.setEncoding(config.encoding);
@@ -534,6 +542,6 @@ function unescapeString(str = '') {
 			}
 			config.pipedData = pipeData;
 		}
-		rexreplace.engine(config);
+		run();
 	});
 })();

src/engine.ts

@@ -10,6 +10,7 @@ const now = new Date();
 
 import {
 	outputConfig,
+	errorCount,
 	step,
 	debug,
 	chat,
@@ -43,8 +44,13 @@ function validateEncoding(encoding = 'utf8') {
 	}
 }
 
-// Returns a promise that resolves when all files are done
+// Returns a promise that resolves with the number of errors when all files are done
 export async function engine(config: any = {engine: 'default'}) {
+	await run(config);
+	return errorCount();
+}
+
+async function run(config) {
 	outputConfig(config);
 	validateEncoding(config.encoding);
 

src/output.ts

@@ -11,12 +11,18 @@ const lastItemChar = new Map();
 // The char code of the last character printed to each stream
 const lastPrintedChar = new Map();
 
+// The number of errors in this run, so the CLI can give an exit code
+let errors = 0;
+
 export const outputConfig = function (_config) {
 	config = _config;
 	lastItemChar.clear();
 	lastPrintedChar.clear();
+	errors = 0;
 };
 
+export const errorCount = () => errors;
+
 // Everything is printed from here, so the last character of each stream is known
 const write = function (text, stream) {
 	stream.write(text);
@@ -105,6 +111,7 @@ export const error = function (msg, data = '') {
 	if (config.halt) {
 		kill();
 	}
+	errors++;
 	return false;
 };
 

test/cli/run.ai.sh

@@ -95,15 +95,15 @@ reset
 echo assert                "rexreplace foo xxx my.file -q" ""
 # Error run (file not found) should still print error to stderr (tested indirectly via assert_raises failure message)
 # We can't easily assert stderr content with aserta, but we can check it doesn't suppress errors needed for failure
-echo assert_raises         "rexreplace foo xxx non_existent_file.txt -q" 1 # Exit code should still be non-zero
+assert_raises         "rexreplace foo xxx non_existent_file.txt -q" 1 # Exit code should still be non-zero
 
 # -Q, --quiet-total
 reset
 # Successful run should have NO stdout
 echo assert                "rexreplace foo xxx my.file -Q" ""
 # Error run should have NO output at all (stdout or stderr)
 # Check exit code is still non-zero, but no output should be printed by aserta on failure if rexreplace is truly quiet
-echo assert_raises         "rexreplace foo xxx non_existent_file.txt -Q" 1
+assert_raises         "rexreplace foo xxx non_existent_file.txt -Q" 1
 
 # -H, --halt
 reset

test/js/basics.test.js

@@ -229,6 +229,6 @@ describe('piped data', () => {
 	});
 
 	test('leaves nothing to do if neither data nor files are given', async () => {
-		expect(await rr('o', 'x')).toEqual({out: '', err: '0 files found\n', exit: null});
+		expect(await rr('o', 'x')).toEqual({out: '', err: '0 files found\n', exit: 1});
 	});
 });

test/js/fix/683.test.js

@@ -15,7 +15,7 @@ describe('#683 --exclude-re follows the regex flags of the main search', () => {
 	test('the example from the issue', async () => {
 		fs.writeFileSync('a.file', 'x\n');
 		const config = {includeGlob: ['a.file'], excludeRe: ['A'], output: true};
-		expect(await rr('x', 'y', config)).toEqual({out: '', err: '0 files found\n', exit: null});
+		expect(await rr('x', 'y', config)).toEqual({out: '', err: '0 files found\n', exit: 1});
 		expect((await rr('x', 'y', {...config, voidIgnoreCase: true})).out).toBe('y\n');
 	});
 

test/js/fix/705.test.js

@@ -81,7 +81,7 @@ describe('#705 the engine returns a promise that resolves when all files are don
 			const result = await rr('o', 'x', {includeGlob: ['*.file'], ...flags});
 
 			expect(result.err).toContain('ENAMETOOLONG');
-			expect(result.exit).toBe(null);
+			expect(result.exit).toBe(1);
 			expect(content(['a.file', tooLong, 'c.file'])).toEqual(['fxx', 'foo', 'fxx']);
 			expect(fs.readdirSync('.')).toHaveLength(3);
 		});

test/js/fix/708.test.js

@@ -83,7 +83,7 @@ describe('#708 the number of files in work at the same time is limited', () => {
 
 		expect(result.out).toBe('my.file\n');
 		expect(result.err).toContain('EACCES');
-		expect(result.exit).toBe(null);
+		expect(result.exit).toBe(1);
 		expect(await Bun.file('my.file').text()).toBe('fxx');
 	});
 });

test/js/fix/728.test.js

@@ -0,0 +1,50 @@
+// https://github.com/mathiasrw/rexreplace/issues/728
+// An error from a file was printed, but the exit code was 0, so a script could not see that something went wrong.
+import {describe, test, expect} from 'bun:test';
+import fs from 'fs';
+import {rr} from '../helper.js';
+
+// Note that rr() from the helper sets voidAsync per default, so the async flow is `voidAsync: false`
+const flows = [
+	['the async flow', {voidAsync: false}],
+	['--void-async', {voidAsync: true}],
+];
+
+// Make a.file and b.file with the content `foo`, where b.file can not be read
+function oneUnreadable() {
+	fs.writeFileSync('a.file', 'foo');
+	fs.writeFileSync('b.file', 'foo');
+	fs.chmodSync('b.file', 0);
+	return {includeGlob: ['a.file', 'b.file']};
+}
+
+describe('#728 the exit code is 1 when a file gives an error', () => {
+	test.each(flows)('and the other files are still changed with %s', async (name, flags) => {
+		const result = await rr('o', 'x', {...oneUnreadable(), ...flags});
+		expect(result.out).toBe('a.file\n');
+		expect(result.err).toContain('EACCES');
+		expect(result.exit).toBe(1);
+		expect(fs.readFileSync('a.file', 'utf8')).toBe('fxx');
+	});
+
+	test('when the backup can not be made', async () => {
+		fs.mkdirSync('locked');
+		fs.writeFileSync('locked/my.file', 'foo');
+		fs.chmodSync('locked', 0o555);
+
+		try {
+			const result = await rr('o', 'x', {includeGlob: ['locked/my.file']});
+			expect(result.out).toBe('');
+			expect(result.err).toContain('EACCES');
+			expect(result.exit).toBe(1);
+			expect(fs.readFileSync('locked/my.file', 'utf8')).toBe('foo');
+		} finally {
+			// The folder must be writable again so the test folder can be removed
+			fs.chmodSync('locked', 0o755);
+		}
+	});
+
+	test.each(['quiet', 'quietTotal'])('also with --%s', async (flag) => {
+		expect((await rr('o', 'x', {...oneUnreadable(), [flag]: true})).exit).toBe(1);
+	});
+});

test/js/halt.test.js

@@ -16,16 +16,26 @@ function runCli(...args) {
 }
 
 describe('--halt', () => {
+	// a.file can not be read, and the files are handled in the order of the list
+	function lockedFirst() {
+		fs.writeFileSync('a.file', 'foo');
+		fs.writeFileSync('b.file', 'foo');
+		fs.chmodSync('a.file', 0);
+		return {includeGlob: ['a.file', 'b.file']};
+	}
+
 	test('stops the process on the first error', async () => {
-		const result = await rr('o', 'x', {includeGlob: ['nope.file'], halt: true});
-		expect(result.err).toContain('0 files found');
-		expect(result.exit).not.toBe(null);
+		const result = await rr('o', 'x', {...lockedFirst(), halt: true});
+		expect(result.err).toContain('EACCES');
+		expect(result.exit).toBe(1);
+		expect(fs.readFileSync('b.file', 'utf8')).toBe('foo');
 	});
 
-	test('is not the default', async () => {
-		const result = await rr('o', 'x', {includeGlob: ['nope.file']});
-		expect(result.err).toContain('0 files found');
-		expect(result.exit).toBe(null);
+	test('is not the default, so the other files are still handled', async () => {
+		const result = await rr('o', 'x', lockedFirst());
+		expect(result.err).toContain('EACCES');
+		expect(result.exit).toBe(1);
+		expect(fs.readFileSync('b.file', 'utf8')).toBe('fxx');
 	});
 
 	test('changes nothing when all goes well', async () => {

test/js/helper.js

@@ -59,7 +59,10 @@ export async function rr(pattern, replacement, config = {}) {
 			...config,
 		};
 		alias(options);
-		await engine(options);
+		// Like the CLI, an error gives the exit code 1 when the run is done
+		if (await engine(options)) {
+			result.exit = 1;
+		}
 	} catch (e) {
 		if (e !== halt) {
 			throw e;

test/js/replacement-pipe.test.js

@@ -66,7 +66,7 @@ describe('--replacement-pipe', () => {
 
 	test('does not treat the piped data as content to search in', async () => {
 		const result = await rr('o', '_', {replacementPipe: true, pipedData: 'foo'});
-		expect(result).toEqual({out: '', err: '0 files found\n', exit: null});
+		expect(result).toEqual({out: '', err: '0 files found\n', exit: 1});
 	});
 
 	describe('combined', () => {