_has_unstaged_config check reports false negative if the config has been symlinked in place

[comkieffer]

search you tried in the issue tracker

config symlink

describe your issue

If the .pre-commit-config.yaml file is modified, pre-commit reports an error:

[ERROR] Your pre-commit configuration is unstaged.
`git add .pre-commit-config.yaml` to fix this.

This is not the case if .pre-commit-config.yaml is a symlink. In that case, pre-commit only prints out a warning

[WARNING] Unstaged files detected.

To replicate the issue

# Replace the existing config with a symlink
mv .pre-commit-config.yaml real-pre-commit-config.yaml 
ln -s real-pre-commit-config.yaml  .pre-commit-config.yaml
git add .pre-commit-config.yaml real-pre-commit-config.yaml
git commit -m "Make config a symlink" 

# Next change `real-pre-commit-config.yaml`
echo "   " >> real-pre-commit-config.yaml 

# Now run pre-commit - should show 'unstaged files warning'
pre-commit run 

Taking a quick look at the code, the quick-and-dirty option is to resolve symlinks in the config path with os.path.realpath in pre_commit.commands.run:_has_unstaged_config. A more elegant solution would be to resolve the symlink in pre_commit.main:_adjust_args_and_chdir.

The first option seems safer since it will not affect anything outside that check, but the second option feels cleaner. I'm open to implementing either and submitting a PR.

pre-commit --version

git rev: 14fa43d (current HEAD)

.pre-commit-config.yaml

Valid for any config

~/.cache/pre-commit/pre-commit.log (if present)

No response

[asottile]

is there a problem? does it matter?

[comkieffer]

I wouldn't call it a problem, an annoyance at most.

Making unstaged changes to the .pre-commit-config.yml file protects against a footgun (wondering why a hook is not being run correctly when the config is not staged). The current implementation does not protect against this when the config file is symlinked.

[asottile]

ah ok I understand now -- I think I was missing the context initially -- would you be interested in writing a patch for this? should be a pretty easy test as well (there's a similar one for the existing behaviour)

[asottile]

I think the former makes more sense -- the value of the passed in config is "important" for some commands like init-templatedir even with symlinks involved

[comkieffer]

Yes, I'll try to push something out this week.

Thanks for taking the time to read and understand the problem.

[asottile]

the one other thing I thought about is it's going to fail in weird ways if the file is outside the repo -- so might have to think a little bit more about that (since I know some people do wild things like ln -sf /etc/pre-commit-config.yaml .pre-commit-config.yaml)

[HeySlava]

Can I try to write a patch for this issue? I am a newbie; however, I realized the problem and reproduced it locally.

[asottile]

sure!

[HeySlava]

Is it allowed for a config to be a symlink?

[asottile]

I don't think there's anything preventing it today