feat: Use async and batch stage commands #151
Conversation
| @@ -1,15 +1,10 @@ | |||
| const mockStream = () => ({ | |||
There was a problem hiding this comment.
Pretty sure this isn't actually used anymore? I think execa is mocked in every test using jest.mock anyways
| '✗ Code style issues found in the above file(s). Forgot to run Prettier?', | ||
| ); | ||
| } | ||
| if (prettyQuickResult.errors.indexOf('STAGE_FAILED') !== -1) { |
There was a problem hiding this comment.
New error state due to the introduction of the "staging" step
| mock({ | ||
| '/.git': {}, | ||
| }); | ||
|
|
||
| prettyQuick('root'); | ||
| await prettyQuick('/'); |
There was a problem hiding this comment.
Okay. You'll see this in a lot of places :(
I develop on windows, and the issue is that the "find-up" module is pulling OS specific root directories in it's results (ex. it returns "C:\" instead of what you would expect from mock-fs: "/")
I changed most of the tests to pull from the root directory .git/ instance. The only test that is relatively unchanged is the test that specifically makes sure to grab the .git/ from the parent folder. I left that one in a failed state for now on my local, but I believe it will pass CI. In the future, the find-up module will probably need a good mock that utilizes the mock-fs
| @@ -90,6 +95,15 @@ export default ( | |||
| onExamineFile: verbose && onExamineFile, | |||
| }); | |||
|
|
|||
| if (filesToStage.length > 0) { | |||
There was a problem hiding this comment.
This is the new "staging" step that was added
| export const stageFile = (directory, file) => { | ||
| runGit(directory, ['add', file]); | ||
| export const stageFiles = async (directory, files) => { | ||
| const maxArguments = 100; |
There was a problem hiding this comment.
Complete transparency: I didn't spend too much time looking into exaca, but I know that some OS/shells limit the number of arguments you can pass. I figured 100 is a reasonable batch size
|
Ah, it does look like this would break compatibility with node 10 and thus would be a breaking change. Let me know if I'm good to remove node 10.* from the test matrix 😮 |
|
@JounQin , Sorry it toke me so long to get back to this. I can certainly attempt this with the new changes |
pretty-quick(er? 🚀 )
Description
We noticed some performance issues with larger repository and environments at my workplace. In particular, we'd notice the most performance issues when pretty-quick runs on merge conflict commits, as it has to scan and prettify all files from the merge.
After forking pretty-quick and running a bunch of
console.time()commands ( 😉 ), most of the performance impact seemed to come from the "git add" step (around 100ms~200ms). This amount of delay is reasonable for a small number of files, but for large commits it starts to add up to many seconds. It's a small, but noticeable delay that I believe goes goes against the spirit of the utility .As a result, I changed most file system calls to their asynchronous equivalents, and more importantly changed staging each file into a single batched "stage" command/step after all of the files are "prettified". I'm seeing some subjective performance gains as a result of these changes, but haven't gotten the time yet to quantify it into numbers.
Let me know what, if any, of these changes you are interested in merging that might make this PR more manageable :)
This also Fixes #100
How Has This Been Tested?
We'll be piloting the forked version in our office of 50 developers or so. We only use git, and don't use some of the advanced flags for this plugin, so the breadth of end-to-end testing will be limited.
Things that should probably be done before merging