feat: safer handling of partially staged files (with fix) (#33)

* feat: safer handling of partially staged files (#29)

+ Partially staged files are not re-staged

+ Non-zero exit code upon reformatting partially staged file

+ Update README

* Fixes #30 handling of partially staged files

The git command checking for unstaged changes should not have been including a revision.

Author: Mario Pareja

README.md

@@ -74,6 +74,8 @@ In `package.json`'s `"scripts"` section, add:
 
 Pre-commit mode. Under this flag only staged files will be formatted, and they will be re-staged after formatting.
 
+Partially staged files will not be re-staged after formatting and pretty-quick will exit with a non-zero exit code. The intent is to abort the git commit and allow the user to amend their selective staging to include formatting fixes.
+
 ### `--branch`
 
 When not in `staged` pre-commit mode, use this flag to compare changes with the specified branch. Defaults to `master` (git) / `default` (hg) branch.

bin/pretty-quick.js

@@ -9,6 +9,7 @@ const prettyQuick = require('..').default;
 
 const args = mri(process.argv.slice(2));
 
+let success = true;
 prettyQuick(
   process.cwd(),
   Object.assign({}, args, {
@@ -28,10 +29,23 @@ prettyQuick(
       );
     },
 
+    onPartiallyStagedFile: file => {
+      console.log(`✗ Found ${chalk.bold('partially')} staged file ${file}.`);
+      success = false;
+    },
+
     onWriteFile: file => {
       console.log(`✍️  Fixing up ${chalk.bold(file)}.`);
     },
   })
 );
 
-console.log('✅  Everything is awesome!');
+if (success) {
+  console.log('✅  Everything is awesome!');
+} else {
+  console.log(
+    '✗ Partially staged files were fixed up.' +
+      ` ${chalk.bold('Please update stage before committing')}.`
+  );
+  process.exit(1); // ensure git hooks abort
+}

src/__tests__/scm-git.test.js

@@ -11,9 +11,10 @@ afterEach(() => {
   jest.clearAllMocks();
 });
 
-const mockGitFs = () => {
+const mockGitFs = (additionalUnstaged = '') => {
   mock({
     '/.git': {},
+    '/raz.js': 'raz()',
     '/foo.js': 'foo()',
     '/bar.md': '# foo',
   });
@@ -26,8 +27,8 @@ const mockGitFs = () => {
         return { stdout: '' };
       case 'diff':
         return args[2] === '--cached'
-          ? { stdout: './foo.js\n' }
-          : { stdout: './foo.js\n' + './bar.md\n' };
+          ? { stdout: './raz.js\n' }
+          : { stdout: './foo.js\n' + './bar.md\n' + additionalUnstaged };
       case 'add':
         return { stdout: '' };
       default:
@@ -80,6 +81,20 @@ describe('with git', () => {
     ]);
   });
 
+  test('with --staged calls diff without revision', () => {
+    mock({
+      '/.git': {},
+    });
+
+    prettyQuick('root', { since: 'banana', staged: true });
+
+    expect(execa.sync).toHaveBeenCalledWith(
+      'git',
+      ['diff', '--name-only', '--diff-filter=ACMRTUB'],
+      { cwd: '/' }
+    );
+  });
+
   test('calls `git diff --name-only` with revision', () => {
     mock({
       '/.git': {},
@@ -151,19 +166,33 @@ describe('with git', () => {
     expect(fs.readFileSync('/bar.md', 'utf8')).toEqual('formatted:# foo');
   });
 
-  test('with --staged stages staged files', () => {
+  test('with --staged stages fully-staged files', () => {
     mockGitFs();
 
     prettyQuick('root', { since: 'banana', staged: true });
 
-    expect(execa.sync).toHaveBeenCalledWith('git', ['add', './foo.js'], {
+    expect(execa.sync).toHaveBeenCalledWith('git', ['add', './raz.js'], {
+      cwd: '/',
+    });
+    expect(execa.sync).not.toHaveBeenCalledWith('git', ['add', './foo.md'], {
       cwd: '/',
     });
     expect(execa.sync).not.toHaveBeenCalledWith('git', ['add', './bar.md'], {
       cwd: '/',
     });
   });
 
+  test('with --staged does not stage previously partially staged files AND aborts commit', () => {
+    const additionalUnstaged = './raz.js\n'; // raz.js is partly staged and partly not staged
+    mockGitFs(additionalUnstaged);
+
+    prettyQuick('root', { since: 'banana', staged: true });
+
+    expect(execa.sync).not.toHaveBeenCalledWith('git', ['add', './raz.js'], {
+      cwd: '/',
+    });
+  });
+
   test('without --staged does NOT stage changed files', () => {
     mockGitFs();
 

src/index.js

@@ -12,6 +12,7 @@ export default (
     branch,
     onFoundSinceRevision,
     onFoundChangedFiles,
+    onPartiallyStagedFile,
     onWriteFile,
   } = {}
 ) => {
@@ -30,13 +31,28 @@ export default (
     .filter(isSupportedExtension)
     .filter(createIgnorer(directory));
 
+  const unstagedFiles = staged
+    ? scm
+        .getUnstagedChangedFiles(directory, revision)
+        .filter(isSupportedExtension)
+        .filter(createIgnorer(directory))
+    : [];
+
+  const wasFullyStaged = f => unstagedFiles.indexOf(f) < 0;
+
   onFoundChangedFiles && onFoundChangedFiles(changedFiles);
 
   formatFiles(directory, changedFiles, {
     config,
     onWriteFile: file => {
       onWriteFile && onWriteFile(file);
-      staged && scm.stageFile(directory, file);
+      if (staged) {
+        if (wasFullyStaged(file)) {
+          scm.stageFile(directory, file);
+        } else {
+          onPartiallyStagedFile && onPartiallyStagedFile(file);
+        }
+      }
     },
   });
 };

src/scms/git.js

@@ -61,6 +61,10 @@ export const getChangedFiles = (directory, revision, staged) => {
   ].filter(Boolean);
 };
 
+export const getUnstagedChangedFiles = directory => {
+  return getChangedFiles(directory, null, false);
+};
+
 export const stageFile = (directory, file) => {
   runGit(directory, ['add', file]);
 };

src/scms/hg.js

@@ -35,6 +35,10 @@ export const getChangedFiles = (directory, revision) => {
   ].filter(Boolean);
 };
 
+export const getUnstagedChangedFiles = () => {
+  return [];
+};
+
 export const stageFile = (directory, file) => {
   runHg(directory, ['add', file]);
 };