Should we check for uncommitted/untracked files in pre-commit hook?

hollowverse-archive / hollowverse

The new Hollowverse.com

The Unlicense

9 stars 4 forks source link

Should we check for uncommitted/untracked files in pre-commit hook? #490

Open forabi opened 6 years ago

forabi commented 6 years ago

Uncommitted changes and untracked files could change the result of pre-commit hooks.

Examples:

if I have built the app to /dist, TSLint will try to lint the compiled, compressed files which would produce tons of errors.
if I create a new file that I haven't imported anywhere, it would still be type-checked by tsc and that could fail during pre-commit, even if that files is still untracked in git.

wholesomedev commented 6 years ago

Yeah, I feel our checks should only care about tracked files

forabi commented 6 years ago

One issue that this check would not catch is when we have made changes to a bunch of tracked files but we only want to commit some of those files. In that case, tslint, tsc, eslint... would lint the uncommited changes, because they read files from the filesystem, not from git's staging area.

forabi commented 6 years ago

And if we disallow uncommitted changes when committing, it would be too inconvenient.

wholesomedev commented 6 years ago

One issue that this check would not catch is when we have made changes to a bunch of tracked files but we only want to commit some of those files. In that case, tslint, tsc, eslint... would lint the uncommited changes, because they read files from the filesystem, not from git's staging area.

Can you give an example of what you mean here, I don't fully understand?

forabi commented 6 years ago

Can you give an example of what you mean here, I don't fully understand?

Say I'm working on a new feature that requires changes to many files. I finished implementing the feature but I want to commit some of the changed files, not all of them to give a better git history, and to make it easier to revert changes in case I want to. There are two cases here:

If I leave some files unstaged and run the pre-commit hook, the checks will run on the unstaged files (linters do not distinguish between staged and unstaged files, because they work with the filesystem and have no idea about git). The checks could fail because those uncommitted files contain errors.
If we enforce on pre-commit that no uncommitted files can exist, I would need to stash these files in order to be able to commit. This is inconvenient.

wholesomedev commented 6 years ago

@forabi Most CLI tools, (eslint, tslint, etc) accept a list of files as input. One solution is to create a shell command that retrieves the right files to pass to the CLI tool.