Lefthook does not stash untracked files

j-chmielewski commented 1 month ago

:wrench: Summary

This may be me misunderstanding lefthook's philosophy, but after reading through those issues and PRs:

I was convinced that lefthook stashes the changes before running pre-commit hooks. It seems to me however that this does not include untracked files. This means that if I add new files to the project without staging them, then they will be a part of hook's evaluation unless I explicitly specify the files for the linting / formatting tool with {staged_files} placeholder.

This is an issue for me since in our projects we often prepare npm scripts, like:

    "lint": "pnpm lint:prettier && pnpm lint:eslint && astro check && tsc",
    "lint:eslint": "eslint --max-warnings=0 src",
    "lint:prettier": "prettier 'src/**/*.{js,jsx,ts,tsx,astro,scss,css,json,mdx,md}' --check",
    "fix": "pnpm fix:prettier && pnpm fix:eslint",
    "fix:eslint": "eslint --fix src",
    "fix:prettier": "prettier 'src/**/*.{js,jsx,ts,tsx,astro,scss,css,json,mdx,md}' --write",

Those are pretty elaborate and I'd like to avoid rewriting and duplicating them in lefthook config file. Ideally lefthook would just run the scripts and the scripts would only see staged files. This is achievable by simply stashing all the changes - including new, untracked files, like git stash -a does.

I suspect that this may be in conflict with the {all_files} placeholder. If that's the case then is there an elegant way to achieve what I'm trying to do?

Lefthook version

1.7.18

Steps to reproduce

https://github.com/j-chmielewski/lefthook-unstaged-file

mrexox commented 1 month ago

Hey @j-chmielewski! Thank you for creating this issue. I can describe you the difficulties with hiding the untracked files.

Lefthook uses git diff to save the unstaged changes. But there is no way to add untracked files to this diff (as far as I know), so to hide untracked files on pre-commit hook another mechanism must be used. git stash is probably that mechanism.

This changes the whole implementation of hiding the files. Currently I don't have enough resources to develop this approach. But I'd be glad to support an effort from the community enthusiasts!

j-chmielewski commented 1 month ago

Hi @mrexox, thanks for answering my questions. I'm considering implementing this but fist I'd like to make sure that I won't realize halfway through that there are significant roadblocks to this approach. My concerns:

won't this collide with other features ({all_files}?) and thus make the implementation complex and clumsy
won't it necessitate rewrites of other parts of the tool
will this actually be welcomed by the dev team and users (is this the "correct" behavior for a git hook tool)

I see two ways to do this:

The hard way - rewrite saving to use git stash. This would probably be my initial approach when implementing a tool like this but I bet it's quite naive and you have your reasons to use git diff instead. Perhaps you're trying to avoid messing up user's stash? How much work would this be and would you prefer this approach over the other?
Modify current git diff behavior to include untracked files. This can perhaps be done with git ls-files --others --exclude-standard combined with git diff --no-index.

The second approach seems more doable for me since it uses current code and essentially boils down to appending the two commands I mentioned to current stashing functions.

What are your thoughts on that? Do you see any other issues I didn't mention?

Cheers :vulcan_salute:

mrexox commented 1 month ago

won't this collide with other features ({all_files}?) and thus make the implementation complex and clumsy

I don't think this will be an issue. By hiding all unstaged and untracked changes before executing the {all_files} commands, those changes will be ignored. This approach is likely better than keeping untracked files and executing commands on them.

won't it necessitate rewrites of other parts of the tool

I hope only the part in internal/git/repository.go that saves and applies the diff will need changes.

will this actually be welcomed by the dev team and users (is this the "correct" behavior for a git hook tool)

My inspiration for this feature came from the lint-staged utility. They also face a similar issue (see issue #1473).

It's hard to predict user expectations, but if untracked files remain available and untracked after the pre-commit hook, it shouldn't significantly affect users. I believe hiding untracked files is a more intuitive behavior.

Here’s the PR that introduced hiding unstaged changes, for reference.

The hard way - rewrite saving to use git stash.

My main concern is this issue with stashing partially staged files:

❯ git init
❯ echo A > A
❯ echo B > B
❯ echo C > C
❯ git status --short
?? A
?? B
?? C

❯ git add A
❯ git add B
❯ git status --short
A  A
A  B
?? C

❯ echo BB >> B
❯ git status --short
A  A
AM B
?? C

❯ git stash push --keep-index --include-untracked
Saved working directory and index state WIP on main: eb74add test

❯ git status --short
A  A
A  B

❯ cat B
B

❯ git stash apply
Auto-merging B
CONFLICT (add/add): Merge conflict in B
On branch main
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
        new file:   A

Unmerged paths:
  (use "git restore --staged <file>..." to unstage)
  (use "git add <file>..." to mark resolution)
        both added:      B

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        C

❯ echo $?
1

❯ git status --short
A  A
AA B
?? C

This shows that applying a stash with a partially staged file can lead to merge conflicts. I’m unsure if there’s a way to resolve this.

Modify current git diff behavior to include untracked files.

This sounds promising! I didn’t find a way to append untracked changes to a diff, but it might be possible with a special stash. The flow could look like this:

# git diff ... – existing flow

# special flow for untracked files
❯ files=$(git ls-files --others --exclude-standard)
❯ git stash push --include-untracked -- $files # must be saved in a stash with a specific message

# run pre-commit hook

❯ git stash apply
# apply the diff to revert unstaged chages ...

sanmai-NL commented 1 month ago

@j-chmielewski I don't want Lefthook to stash all files. I have my own Git-related commands that detect some state, which would be influenced by this. Furthermore, this would cause Lefthook to mutate the Git working copy with all the risks for destruction. Moreover, it could be a costly operation if a lot of untracked files are generated in some process, like CI.

adamdicarlo0 commented 1 month ago

I keep running into lefthook running my formatting script on completely-unstaged files, and then seeing stage_fixed: true, and so staging the file... it shouldn't add new files to git.

evilmartians / lefthook